How to solve the "Growing If Statement" problem?

Question

I've been doing some reading about design patterns and wanted some perspective. Consider the following:

Dim objGruntWorker as IGruntWorker

if SomeCriteria then
   objGruntWorker = new GoFor()
else if SomeOtherCriteria then
   objGruntWorker = new Newb()
else if SomeCriteriaAndTheKitchenSink then
   objGruntWorker = new CubeRat()
end if

objGruntWorker.GetBreakfast()
system.threading.thread.sleep(GetMilliSecondsFromHours(4))
objGruntWorker.GetLunch()

The above code grows each time a new Criteria arises. I've seen code like this all over the place and in ignorance wrote some of it myself. How should this be solved? Does this kind of anti-pattern have a more "formal" name? Thanks for your help!

Edit: Another consideration is I want to avoid having to recompile the existing implementations of IGruntWorker simply to add a new implementation.

Answer 1

I think as long as your most likely criteria are ordered first to allow the runtime to jump the rest of the cases, this is fine.

If your concern is just readability you could use the ternary operator or if the criteria evaluations are just ==, you could use a switch statement.

Answer 2

That sort of logic is often encapsulated using the Factory method pattern. (See the ImageReaderFactory example under Encapsulation.)

Answer 3

The type of pattern that would suit the above solution would be the Factory Pattern. You have a situation where you don't need to know the concrete type of object you require, it just has to implement IGruntWorker. So you create a factory which takes in a criteria and based on that criteria you would return the specific IGruntWorker object. It is usually a good idea to map the criteria to some identifier i.e. an enumeration or constant for readability e.g.

public enum WorkerType
{
    Newbie,
    Average,
    Expert
}

public class WorkerFactory
{
    public static IGruntWorker GetWorker(WorkerType type)
    {
        switch (type)
        {
            case WorkerType.Newbie:
                 return new NewbieWorker();
            case WorkerType.Average:
                 return new AverageWorker();
            case WorkerType.Expert:
                 return new ExpertWorker();
        }
    }
}

So in your case you could have a small helper method that works out the correct type of Worker required based on the criteria. This could even be wrapped up in a read-only property which you just pass into the factory.

Answer 4

You could create Factories for each object type, and those factories could have a function that takes criterias as parameter and returns a IGruntWorker if the parameters are satisfied (or null otherwise).

You could then create a list of those factories and loop through them like (sorry I'm a c# guy):

Dim o as IGruntWorker;
foreach (IGruntWorkerFactory f in factories)
{
    o = f.Create(criterias);
    if (o != null)
        break;
}

When a new criteria is needed, you only add it to the list of factories, no need to modify the loop.

There are probably some more beautiful ways

My 2 cents

Answer 5

I think this pattern is fine, so long as your criteria and operations are single lines/method calls. This is easy to read and accurately reflects your logic:

   if (ConditionOne())
   {
     BuildTheWidget();
   }
   else if (ConditionTwo())
   {
     RaiseTheAlarm();
   }
   else if (ConditionThree())
   {
      EverybodyGetsARaise();
   }

Even if there are 20 different conditions, it probably is an accurate reflection of some complex business logic of your application.

On the other hand, this is a readability disaster

if (  ((A && B) || C &&
      (D == F) || (F == A)))
{
   AA;
   BB;
   //200 lines of code
}
else if ( (A || D) && B)
{
  // 200 more lines
}

Answer 6

If you are using .NET you could build it with reflection instead. For example, if you were creating a plugin system then you would have a folder to drop plugin DLLs into. Then your factory would look at the available DLLs, examine each one for the appropriate reflection attributes, then match those attributes against whatever string was passed in to decide which object to select and invoke.

This keeps you from having to recompile your main app, though you'll have to build your workers in other DLLs and then have a way to tell your factory which one to use.

Here's some really fast and dirty pseudo code to get the point across:

Assuming you have a DLL assembly called Workers.DLL

Set up an attribute called WorkerTypeAttribute with a string property called Name, and the constructor to be able to set that Name property.

[AttributeUsage(AttributeTargets.Class, AllowMultiple=false)]
public class WorkerTypeAttribute : Attribute
{
    string _name;
    public string Name { get { return _name; } }
    public WorkerTypeAttribute(string Name)
    {
        _name = Name;
    }
}

You'd then apply this attribute to any worker class that you've defined like:

[WorkerType("CogWorker")]
public class CogWorker : WorkerBase {}

Then in your app's worker factory you'd write code like:

 public void WorkerFactory(string WorkerType)
    {
        Assembly workers = Assembly.LoadFile("Workers.dll");
        foreach (Type wt in workers.GetTypes())
        { 
            WorkerTypeAttribute[] was = (WorkerTypeAttribute[])wt.GetCustomAttributes(typeof(WorkerTypeAttribute), true);
            if (was.Count() == 1)
            {
                if (was[0].Name == WorkerType)
                { 
                    // Invoke the worker and do whatever to it here.
                }
            }
        }
    }

I'm sure there are other examples of how to do this out there, but if you need some more pointers, let me know. The key is that all of your workers need to have a common parent or interface so that you can invoke them the same way. (I.e. all of your workers need a common "Execute" method or something that can be called from the factory, or wherever you use the object.

Answer 7

If you can define an object with a checkCriteria method, then you can make this code table-driven. I don't know C#, so bear with me on the syntax:

public class WorkerFactory {
    IGruntWorker makeWorkerIfCriteria(criteria_parameters parms);
}

extern WorkerFactory worker_factories[];  /* table with factories in order */

IGruntWorker makeJustTheRightWorker(criteria_parameters actual_critera) {
  for (i = 0; i < worker_factories.length(); i++) {
    IGruntWorwer w = worker_factories[i].makeWorker(actual_criteria);
    if (!null(w)) return w;
  }
  --- grim error --- /* table not initiailized correctly */
}

Then some of the objects in the table look like this

public class MakeGoFor(critera_parameters cp) {
   if SomeCriteria then
      return new GoFor();
   else
      return NULL;
}

You can recompile the table in a separate module without having to recompile the selection code. In fact, if you get ambitious, you could even build the table at run time based on command-line arguments or the contents of a file...

Answer 8

I think a lot of it depends on how predictable your 'conditions' are. Your 'growing IF' IS essentially a factory, and perhaps refactoring it out to its own method or class would help, but it may STILL be a growing-IF. If your conditions are things you cannot predict, like "if joe.is.on.fire" or "if x==2" or "if !shuttle.is.launched" then you're stuck with IF's.

One bad thing about these uber-IF's is the scope they may have over your application. That is, what all do you need to call/touch/check to determine which 'if' should be true? You might end up having tons of global cruft or lots of parameters to pass to your 'factory'. One thing I did a little while ago to help with this was implement a factory of sorts that contained an array of boolean-delegates (Func) and types. I would register boolean delegates and types at initialization-time, and iterate thru the list in the factory calling each delegate until I got a 'true' and then instantiated that type. That worked well for me becuase I was able to 'register' new conditions without editing the factory.

Just an idea

Answer 9

Could you use a variant of the visitor pattern instead? Call it the factory visitor (perhaps)

excuse the pseudo-code, but my VB is rusty

Dim objGruntWorker as IGruntWorker

objGruntWorker = null

// all your objects implement IFactoryVisitor
Dim factory as IFactoryVisitor
while objGruntWorker == null
    factory = factoryCollection.GetNext 
    objGruntWorker = factory.TryBuild(...)
end

objGruntWorker.GetBreakfast()
system.threading.thread.sleep(GetMilliSecondsFromHours(4))
objGruntWorker.GetLunch()

Answer 10

I know your .NET but this is how I do something similar in a Java web application, where my 'if-thens' were growing....still requires a recompile but easy to add other actions or in your case grunt workers.

private HashMap actionMap = new HashMap();

actionMap.put("cubeRat", new CubeRatAction());
actionMap.put("newb", new NewbAction());
actionMap.put("goFor", new goForAction());
actionMap.put("other", new otherAction());

String op = request.getParameter("criteria");  // not sure how your criteria is passed in but this is through a parameter in my URL.
ControllerAction action = (ControllerAction) actionMap.get(op);
if (action != null) {
     action.GetBreakfast();
     action.Sleep();
     action.GetLunch();              
} else {
     String url = "views/errorMessage_v.jsp";
     String errMessage = "Operation '" + op + "' not valid for in '" + request.getServletPath() + "' !!";
     request.setAttribute("message", errMessage);
     request.getRequestDispatcher(url).forward(request, response);
}

Answer 11

You can use reflection to find a constructor of a given type and create the instance by the constructor. Of cause, constructors must follow certain pattern. In your example above, all are default constructors.