Code Smell: Switch Statements

posted on 11/01/07 at 09:37:54 pm by Joel Ross

I've never really thought too much about switch statements as being a sign of code smell, but after reading Jeremy Jarrell's post about refactoring switch statements with the Strategy Pattern (the Wikipedia entry has C# code in it!), I'm starting to see the light. That, and I'm working with some code I wrote a few years ago, where I have the same switch statement (or in some cases, the same set of if / else if / else statements) over and over.

Jeremy says the main reason to do this is in case you end up adding new values to the switch statement down the line, but as I started looking at what he was writing and what I was seeing, I think I can benefit from it even though I know there won't ever be another value in these switch statements. I know - I shouldn't say "ever" but in this case, I'm very sure there won't be. There hasn't been a new value since the original three were added, and the vertical business isn't changing in that way.

Want an example where the values won't change? Primary colors: Red, Green and Blue. So let's use those as an example. You have three operations that need to occur. OperationOne is color specific. OperationTwo is generic, but relies on OperationOne already running. OperationThree is also color specific, and relies on OperationTwo completing. Using switch statements, it would look something like this:

 1: public void DoColorWork(ColorObject color, ColorManager manager)
 2: {
 3:  switch(color.PrimaryColor)
 4:  {
 5:  case PrimaryColor.Red:
 6:  manager.OperationOneRed(color);
 7:        break;
 9:        case PrimaryColor.Green:
 10:  manager.OperationOneGreen(color);
 11:        break;
 13:        case PrimaryColor.Blue:
 14:  manager.OperationOneBlue(color);
 15:        break;
 16:  }
 18:  manager.OperationTwo(color);
 20:  switch(color.PrimaryColor)
 21:  {
 22:  case PrimaryColor.Red:
 23:  manager.OperationThreeRed(color);
 24:  break;
 26:  case PrimaryColor.Green:
 27:  manager.OperationThreeGreen(color);
 28:  break;
 30:  case PrimaryColor.Blue:
 31:  manager.OperationThreeBlue(color);
 32:  break;
 33:  }
 34: }

It's not horrible, but it can definitely be done better. Using the strategy pattern (and the Factory pattern), this can be done cleaner:

 1: public void DoColorWork(ColorObject color)
 2: {
 3:  ColorManager manager = GetColorManager(color);
 4:  manager.OperationOne(color);
 5:  manager.OperationTwo(color);
 6:  manager.OperationThree(color);
 7: }

In the above, I use a factory method (GetColorManager) to return an instance of ColorManager.

 1: public ColorManager GetColorManager(ColorObject color)
 2: {
 3:  switch(color.PrimaryColor)
 4:  {
 5:  case PrimaryColor.Red:
 6:  return new RedColorManager();
 8:  case PrimaryColor.Green:
 9:  return new GreenColorManager();
 11:        case PrimaryColor.Blue:
 12:  return new BlueColorManager();
 13:  }
 14: }

The ColorManager would be virtual and define OperationOne, and OperationThree to be abstract, while providing the implementation for OperationTwo. Each concrete class (RedColorManager, GreenColorManager, and BlueColorManager) would implement OperationOne and OperationThree with color specific code.

In the example, I still have a switch statement, but I'm down to just one, and that code 1.) is easier to maintain the way it is, and 2.) could be refactored further. Since it's only in one place, it wouldn't be a huge deal to leave it as a switch statement, and as long as the functionality is the same, refactoring can be done later if needed.

Of course, you could argue that going from two switch statements to one isn't a huge savings. That's true. But in some code I'm working with, I could have a Manager factory that I get once, and I could literally change 50 or more switch statements throughout the application. Now that would actually be worth it!

Categories: Development, C#