views:

433

answers:

6

I am trying to create a web-based tool for my company that, in essence, uses geographic input to produce tabular results. Currently, three different business areas use my tool and receive three different kinds of output. Luckily, all of the outputs are based on the same idea of Master Table - Child Table, and they even share a common Master Table.

Unfortunately, in each case the related rows of the Child Table contain vastly different data. Because this is the only point of contention I extracted a FetchChildData method into a separate class called DetailFinder. As a result, my code looks like this:

DetailFinder DetailHandler;
if (ReportType == "Planning")
  DetailHandler = new PlanningFinder();
else if (ReportType == "Operations")
  DetailHandler = new OperationsFinder();
else if (ReportType == "Maintenance")
  DetailHandler = new MaintenanceFinder();
DataTable ChildTable = DetailHandler.FetchChildData(Master);

Where PlanningFinder, OperationsFinder, and MaintenanceFinder are all subclasses of DetailFinder.

I have just been asked to add support for another business area and would hate to continue this if block trend. What I would prefer is to have a parse method that would look like this:

DetailFinder DetailHandler = DetailFinder.Parse(ReportType);

However, I am at a loss as to how to have DetailFinder know what subclass handles each string, or even what subclasses exist without just shifting the if block to the Parse method. Is there a way for subclasses to register themselves with the abstract DetailFinder?

A: 

As long as the big if block or switch statement or whatever it is appears in only one place, it isn't bad for maintainability, so don't worry about it for that reason.

However, when it comes to extensibility, things are different. If you truly want new DetailFinders to be able to register themselves, you may want to take a look at the Managed Extensibility Framework which essentially allows you to drop new assemblies into an 'add-ins' folder or similar, and the core application will then automatically pick up the new DetailFinders.

However, I'm not sure that this is the amount of extensibility you really need.

Mark Seemann
A: 

You might want to use a map of types to creational methods:

public class  DetailFinder
{
    private static Dictionary<string,Func<DetailFinder>> Creators;

    static DetailFinder()
    {
         Creators = new Dictionary<string,Func<DetailFinder>>();
         Creators.Add( "Planning", CreatePlanningFinder );
         Creators.Add( "Operations", CreateOperationsFinder );
         ...
    }

    public static DetailFinder Create( string type )
    {
         return Creators[type].Invoke();
    }

    private static DetailFinder CreatePlanningFinder()
    {
        return new PlanningFinder();
    }

    private static DetailFinder CreateOperationsFinder()
    {
        return new OperationsFinder();
    }

    ...

}

Used as:

DetailFinder detailHandler = DetailFinder.Create( ReportType );

I'm not sure this is much better than your if statement, but it does make it trivially easy to both read and extend. Simply add a creational method and an entry in the Creators map.

Another alternative would be to store a map of report types and finder types, then use Activator.CreateInstance on the type if you are always simply going to invoke the constructor. The factory method detail above would probably be more appropriate if there were more complexity in the creation of the object.

public class DetailFinder
{
      private static Dictionary<string,Type> Creators;

      static DetailFinder()
      {
           Creators = new Dictionary<string,Type>();
           Creators.Add( "Planning", typeof(PlanningFinder) );
           ...
      }

      public static DetailFinder Create( string type )
      {
           Type t = Creators[type];
           return Activator.CreateInstance(t) as DetailFinder;
      }
}
tvanfosson
Out of sheer curiosity, if Creators was made protected could subclasses of DetailFinder add themselves to Creators during their own static constructor?
DonaldRay
I'd probably provide a Registration method in that case rather than letting each class access the map itself. It would be much easier to make thread-safe with a single Registration method.
tvanfosson
That makes sense. However, is there a guarantee that all of the subclasses will execute their static constructors before the Parse method of the base class is called?
DonaldRay
No I don't think you can guarantee that. The only guarantee is that the static constructor is before the class is used the first time.
tvanfosson
A: 

To avoid an ever growing if..else block you could switch it round so the individal finders register which type they handle with the factory class.

The factory class on initialisation will need to discover all the possible finders and store them in a hashmap (dictionary). This could be done by reflection and/or using the managed extensibility framework as Mark Seemann suggests.

However - be wary of making this overly complex. Prefer to do the simplest thing that could possibly work now with a view to refectoring when you need it. Don't go and build a complex self-configuring framework if you'll only ever need one more finder type ;)

Paolo
A: 

You can use the reflection. There is a sample code for Parse method of DetailFinder (remember to add error checking to that code):

public DetailFinder Parse(ReportType reportType)
{
    string detailFinderClassName = GetDetailFinderClassNameByReportType(reportType);
    return Activator.CreateInstance(Type.GetType(detailFinderClassName)) as DetailFinder;
}

Method GetDetailFinderClassNameByReportType can get a class name from a database, from a configuration file etc.

I think information about "Plugin" pattern will be useful in your case: P of EAA: Plugin

bniwredyc
+2  A: 

You could use an IoC container, many of them allows you to register multiple services with different names or policies.

For instance, with a hypothetical IoC container you could do this:

IoC.Register<DetailHandler, PlanningFinder>("Planning");
IoC.Register<DetailHandler, OperationsFinder>("Operations");
...

and then:

DetailHandler handler = IoC.Resolve<DetailHandler>("Planning");

some variations on this theme.

You can look at the following IoC implementations:

Lasse V. Karlsen
A: 

Like Mark said, a big if/switch block isn't bad since it will all be in one place (all of computer science is basically about getting similarity in some kind of space).

That said, I would probably just use polymorphism (thus making the type system work for me). Have each report implement a FindDetails method (I'd have them inherit from a Report abstract class) since you're going to end with several kinds of detail finders anyway. This also simulates pattern matching and algebraic datatypes from functional languages.

Rodrick Chapman