18. July 2009
One of the code smells I see most often in C# and other OO languages is the misuse of switch/case statements. When I was a young Jedi (some might argue I still am :-)) I didn’t see anything wrong with using the switch statement. Actually there still are a few cases where it’s still applicable, but most usages I come across should have been solved differently. So why is this? What is so bad with the switch statement?
As Dan North tweeted me when I asked for the perfect refactoring solution for switch statements :-) :
A switch is often a sign of behaviour that should live elsewhere – so where should the code in the cases live?
Functions should only do one thing, the one thing which they state. Like CalculateInterest and ConvertDateToString should do exactly that; calculate interest and convert a date to string. Since switch always reside inside functions it almost always violates that principle.
Let’s look at what Uncle Bob (Robert C. Martin) says in his Clean Code book about switch statements:
Even a switch statement with only two cases is larger than I’d like a single block or function to be. It’s also hard to make a switch statement that does one thing. By their nature, switch statements always do N things.
My answer to Dan’s question above was using polymorphism. The cases should live in their own classes. By doing that you don’t have to modify existing code when introducing new behavior, you just create a new state (read class). Btw Dan is really good at asking questions to questions where you end up answering your own question :-)
Enough with the coding theory. Below is a method using a switch statement for getting messages from different systems. Typically these are systems that another system depends on (the system containing the below code). The messages might be if the system is available or not, or if there are any maintenance planned.
public string GetSystemMessage(System system)
{
switch (system)
{
case System.System1:
return GetSystem1Message();
case System.System2:
return GetSystem2Message();
case System.System3:
return GetSystem3Message();
case System.System4:
return GetSystem4Message();
default:
throw new ApplicationException("System not found");
}
}
This is actually quite a good switch. It uses return instead of break and it only has one line per case. If you can’t avoid writing a switch, this is how you should do it.
Now let’s pull it apart and make it better and extendable. Time for refactoring.
Polymorphism by using Strategy Template Pattern
Here’s my code using the Strategy pattern:
public interface ISystemStrategy
{
string GetMessage();
}
public class System1Strategy : ISystemStrategy
{
public string GetMessage()
{
return "System1";
}
}
public class System2Strategy : ISystemStrategy
{
public string GetMessage()
{
return "System2";
}
}
public class System3Strategy : ISystemStrategy
{
public string GetMessage()
{
return "System3";
}
}
public class System4Strategy : ISystemStrategy
{
public string GetMessage()
{
return "System4";
}
}
public class SystemMessenger
{
private readonly ISystemStrategy _system;
public SystemMessenger(ISystemStrategy system)
{
_system = system;
}
public string GetMessage()
{
return _system.GetMessage();
}
{
}
First I define an interface to be used by the different systems (ISystemStrategy) and next each system (1-4) implements that interface, resulting in a class for each case in the switch.In addition I have a class called SystemMessenger that is responsible for accessing the system and get a message.
The switch can now be replaced by this code:
public string GetSystemMessage(ISystemStrategy system)
{
return new SystemMessenger(system).GetMessage();
}
Note that the "client" only relates to the SystemMessenger class and not to the different implementations of ISystemStrategy.
The benefit of this code might not be obvious. Actually it might give the impression that this was a lot of code for almost nothing. Well, don’t be fooled. The strongest aspect of the above solution is that you don’t have to change the GetSystemMessage later when/if a new system is added, you just implement a new class.
To prove the above, here’s what I need to do to introduce a new system:
public class System5 : ISystemStrategy
public string GetMessage()
{
return "System5";
}
}
To use the new system, an instance of System5 will be sent into GetSystemMessage just as before, and SystemMessenger handles the rest as before. No code change!
Dictionary
Another refactoring option is using some type of lookup table. In my example I’ll use a dictionary together with the Func<TResult> delegate. However, I don’t recommend doing this unless you have a really good reason to do so. Stick with the Strategy pattern if you can.
public class SystemMessengerDictionary
{
private readonly Dictionary<System, Func<string>> _systems;
public SystemMessengerDictionary()
{
_systems = new Dictionary<System, Func<string>>
{
{System.System1, GetSystem1Message},
{System.System2, GetSystem2Message},
{System.System3, GetSystem3Message},
{System.System4, GetSystem4Message}
};
}
public string GetSystemMessage(System system)
{
return _systems[system]();
}
//GetSystemX Methods
...
}
I decided to show this example even if I don’t like it. The reason is that it is better than the switch. The worst about this in my eyes is that the enum is still there. I just hate enums! When reading the code they look like classes with interesting behavior, but then you discover they’re just glorified int’s :-|
Conclusion
Hopefully this example will help you understand why switch is something you should avoid and also give you an example of how it should be done. But before you consider doing any refactoring of existing code, consider a second question Dan asked (answered?) me:
How would it evolve if you added a few more cases? And how likely are a few more cases?
This is an important question, because you don’t want to refactor just for the sake of refactoring. Keep this in mind before making use of any of the suggested solutions above.
