Getting Rid of the Case Statement From Hell

In most commercial applications that I have worked with, I have found a curious pattern that continues to emerge – the Case Statement From Hell pattern.  This pattern can be found in any language which has a switch/case, select/case or other form of case statement and is often found in a method which is handling GUI code.  The Case Statement From Hell usually emerges in an event handler which has to process parameters.  These parameters may be in the form of an enum, but in most cases are usually strings.  The Case Statement From Hell has a variable length, but can quite often stretch for thousands of lines of code, violating the “avoid writing long methods” principle of good object oriented design.

If you have seen the Case Statement From Hell pattern, you know what I mean.  If you have not seen the Case Statement From Hell pattern then either you aren’t a developer, or you are a developer and I really, really want to work where you’re working.  Nevertheless, these giant case statements can usually be re-factored easily enough.  They key to doing so is to think a little bit about what the case statement is trying to accomplish. Consider the following class:

class Commands
{  public static void DoCommand(string command)
  {
    switch (command)
    {
      case "Command1":
        Console.WriteLine("Command1");
        Console.WriteLine("Does this");
        break;
      case "Command2":
        Console.WriteLine("Command2");
        Console.WriteLine("Does this");
        break;
      case "Command3":
        Console.WriteLine("Command3");
        Console.WriteLine("Does this");
        break;
    }
  }
}


This class is rather simple and only has one method, but it is an example of the kind of case statement that I am talking about. The DoCommand method takes a string parameter and executes some code based on what the string is. In a commercial application this may have fifty different strings, which each have twenty or thirty lines of code that get run for each command, which looks a lot nastier than this one. At its core though, it’s the same concept. The first step towards simplifying this code is to shorten the case statement by compressing each case’s code into a separate method.

class Commands
{
    public static void DoCommand(string command)
    {
        switch (command)
        {
            case "Command1":
                Command1();
                break;
            case "Command2":
                Command2();
                break;
            case "Command3":
                Command3();
                break;
        }
    }

    private static void Command3()
    {
        Console.WriteLine("Command3");
        Console.WriteLine("Does this");
    }

    private static void Command2()
    {
        Console.WriteLine("Command2");
        Console.WriteLine("Does this");
    }

    private static void Command1()
    {
        Console.WriteLine("Command1");
        Console.WriteLine("Does this");
    }
}


This is actually more code than before, but the code is much better structured. This refactoring has helped us separate our concerns. Now our DoCommand method is only concerned with calling the right method, based on the input. Before we did this refactoring, the DoCommand method was concerned with doing the commands and deciding which commands to do. It was responsible for doing four different things – choosing the right command code and executing three separate commands. Note now that each of these commands are very easily testable. We can write a separate unit test for each one.

Our DoCommand method is still a bit messy though. Not only that, but it has a cyclomatic complexity of 3 (there are three different code branches it can execute). This isn’t bad, but the problem with this is that for each command we add, the complexity grows. So what I will do instead is create a dictionary lookup for the commands and get rid of the case statement altogether:

class Commands
{
  private ConcurrentDictionary<string, Action> commands;

  public Commands()
  {
    commands = new ConcurrentDictionary();
    commands.TryAdd("Command1", this.Command1);
    commands.TryAdd("Command2", this.Command2);
    commands.TryAdd("Command3", this.Command3);
  }

  public void DoCommand(string command)
  {
    commands[command]();
  }

  private void Command3()
  {
    Console.WriteLine("Command3");
    Console.WriteLine("Does this");
  }

  private void Command2()
  {
    Console.WriteLine("Command2");
    Console.WriteLine("Does this");
  }

  private void Command1()
  {
    Console.WriteLine("Command1");
    Console.WriteLine("Does this");
  }
}


Now we have a class (which I could refactor to make a singleton – but that’s not important) which has the same way of executing commands as before (calling DoCommand and passing a string). The only difference is that I now have some constructor code which needs to execute to set up my class. There may also be a marginal increase in memory usage, though it will be very minor and scale linearly.

We could leave the code right there and we have solved our problem. There is one final step that I would like to take though. Right now, to add a command, the developer needs to add the method and then add the method mapping to the dictionary. This means double entry, after a fashion. It’s always easier to remember to do things in one place in code, rather than force developers to remember that they have to add some code in one place and some in another place. So what we will do is create a custom attribute to decorate our code.

class CommandAttribute : Attribute
{
  public string CommandName { get; private set; }

  public CommandAttribute(string commandName)
  {
    CommandName = commandName;
  }
}


Now we can decorate our methods with the command attribute and load all of the commands via reflection. Unfortunately this does get a little bit messy. I am not by any means an expert on reflection, so I am almost certain that there is a nicer way to write the code in the constructor. But the code below works. In essence, the code is getting all of the methods from the assembly that are decorated with the Command attribute. It is then adding to our commands dictionary an anonymous delegate to that method. Note that the closure bug no longer exists in c#, but without the code in place to handle it, anyone compiling to .NET 4 or below will run into trouble if they don’t have the extra line in there.

using System.Collections.Concurrent;
using System.Reflection;
using System.Runtime.CompilerServices;

class Commands
{
  private ConcurrentDictionary<string, Action> commands;

  public Commands()
  {
    commands = new ConcurrentDictionary<string, Action>();

    var assembly = Assembly.GetAssembly(typeof(Commands));
    var assemblyTypes = assembly.GetTypes();
    foreach (var commandType in assemblyTypes)
    {
      var methods = commandType.GetMethods();

      foreach (var method in methods
        .Where(x => x.GetCustomAttributes(typeof(CommandAttribute)).Any()))
      {
        var attributeData = method.GetCustomAttributes(typeof(CommandAttribute))
                              .Cast<CommandAttribute>().Single();

        // to avoid the closure bug in c#
        var closedMethod = method;
        commands.TryAdd(
          attributeData.CommandName, 
          () => closedMethod.Invoke(null, null));
      }
    }
  }

  public void DoCommand(string command)
  {
    commands[command]();
  }

  [Command("Command3")]
  public static void Command3()
  {
    Console.WriteLine("Command3");
    Console.WriteLine("Does this");
  }

  [Command("Command2")]
  public static void Command2()
  {
    Console.WriteLine("Command2");
    Console.WriteLine("Does this");
  }

  [Command("Command1")]
  public static void Command1()
  {
    Console.WriteLine("Command1");
    Console.WriteLine("Does this");
  }
}


The real beauty of this code is that to create a new command, all I have to do is create a static method and decorate it with the Command attribute. There are several variations on this concept that you could try in your own code. If you want to, you could make the command attribute apply to a class, and force that class to implement a method called “Execute()” (or something similar). This may mean that you end up with a large number of one-method classes, but as long as they are organised neatly, this is preferable to having one large class with thousands of methods in it. As always, development is a game of trade-offs.

I really like using reflection and the command pattern to solve this problem as it makes developing new commands very easy to teach other developers. Even though my example is not simple (the code is quite complex, particularly for a novice programmer) the simplicity comes when adding new commands using this pattern. We have effectively empowered other users to be able to add new commands faster and with less merge conflicts. It also encourages good design as the developer will never be tempted to write code around the case statement (which isn’t there any more). It also encourages a separation of concerns between the different commands.

So there you have it, with a little reflection and clever use of delegates we can entirely eliminate Case Statements from Hell.

Advertisements

5 thoughts on “Getting Rid of the Case Statement From Hell

  1. Please remove this article; you’ve gone from a simple switch statement to a hideously over-engineered mess. Any inexperienced (or just plain bad) developer would happily read this and shoe-horn it into his/her next project expecting to receive positive comments but would no doubt be told to refactor it into something must simpler to read and maintain.

    Awful article, awful coding.

    • Thank you for your comment. I disagree with you obviously.

      The thrust of your argument seems to be:

      you’ve gone from a simple switch statement to a hideously over-engineered mess

      And I kind of agree with you *if and only if* the original code (the “simple switch statement”) would only ever deal with three cases. This was an oversimplified example for teaching purposes, and not indicative of most real-world examples. In the real world, I’ve seen many switch statements that follow this anti-pattern that have thousands of lines of code and have cyclomatic complexities in the hundreds. Not only does this make for difficult to follow code, it also makes for impossible-to-test code.

      The second of the SOLID principles is the Open/Closed principle, which states that “software entities should be open for extension but closed for modification”. The initial code in this example was a very bad violator of the open/closed principle, while the refactored code very nicely follows the principle. It is very easy to create new commands (simply create a method and decorate it with an attribute) and there is no need to change the calling code.

      Yes, the reflection code is a bit messy but this solution is extremely easy to scale and all of the code is testable. The “simple switch statement” is very difficult to test, difficult to follow and is not scalable at all.

  2. But, to the substance… I would remove the last bit =) Or introduce a sub-heading; this final section is a digression from the (excellent) meat of the article.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s