Home
About
Contact
Sunday, July 19, 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.

Sunday, July 19, 2009 3:17:06 AM (W. Europe Daylight Time, UTC+02:00)
Unless your real code use SystemMessenger to do something slightly more, you should get rid of it, because the GetSystemMessage() is simply doing:

public string GetSystemMessage(ISystemStrategy system)
{
//return new SystemMessenger(system).GetMessage();
return system.GetMessage();
}

More important, this refactoring forces all clients to change. Now they have to pass the correct instance of ISystemStrategy instead of the enum.

You forgot to show us how that happens.
Sunday, July 19, 2009 10:03:48 AM (W. Europe Daylight Time, UTC+02:00)
Thomas,

You're right in the example code I used. However, given a situation where ISystemStrategy was not given in to the method (an example I should have used) SystemMessenger makes sense. This to not have direct dependency between the actual SystemX class.

Using it from a different place would look something like this:

var systemMessenger = new SystemMessenger(new System3());
string message = systemMessenger.GetMessage();
The responsibillity of SystemMessenger is to access the different systems. One needs to make sure not to violate the YAGNI principle though, which was probably the case in my previous example...

Regarding breaking the contract by requiring ISystemStrategy instead of the enum, this is something I would have done no matter what, since I hate enums. And since I didn't show you the code that use GetSystemMessage() I leave it up to you to figure that one out :-)
Monday, July 20, 2009 12:15:15 AM (W. Europe Daylight Time, UTC+02:00)
Well if you get rid of the enums anyway then you could improve the Dictionary a little bit by using Dictionary<type, Func<string>> The good thing about using the Dictionary solution vs the custom classes is that now your custom classes don't need to know anything about a message, this can be outside their responsibility.

So depending whether or not the ISystemStrategy should know about what it should do in the re-factored switch I would go for either of the solutions.

Does that make sense?

-Mark



Monday, July 20, 2009 12:23:21 AM (W. Europe Daylight Time, UTC+02:00)
Mark,

You have a point, but most of all because of my trivial example (which I regret :-)). Since my classes only has the responsibility of returning a message as string, it's not much behavior. Though in a real scenario the system classes would probably have more stuff in it, making a dictionary a bad candidate.

There is of course a reason why I showed the dictionary example (except from it being a common solution to the switch refactoring). The reason for why I'm argue strongly for using polymorphism though, is I find that to be better in the long run if that's a valid option.
Monday, July 20, 2009 12:29:31 AM (W. Europe Daylight Time, UTC+02:00)
I am just saying that even if your classes have much more behavior, I could still go for a Dictionary if I am looking for different behavior depending on the type that doesn't belong in the class. Sort of your visitor pattern.
OpenID
Please login with either your OpenID above, or your details below.
Name
E-mail
(will show your gravatar icon)
Home page

Comment (Some html is allowed: a@href@title, b, blockquote@cite, em, i, strike, strong, sub, sup, u) where the @ means "attribute." For example, you can use <a href="" title=""> or <blockquote cite="Scott">.  

Live Comment Preview
RSS RSS - Comments Twitter LinkedIn
         
SEARCH
 
 
         
TOP POSTS
   
         
NAVIGATION
   
         
CATEGORIES
  .Net (61) ADFS (3) Agile (30) Ajax (5) Architecture (20) Articles (1) ASP.NET (6) ASP.NET-MVC (1) Blogging (12) Books (2) BPEL (1) CleanCode (1) CloudComputing (7) Community (4) CSharp (11) DasBlog (5) Database (2) DDD (5) Deployment (16) DSL (1) Events (38) ExtremeProgramming (6) Fun (6) Gadgets (4) IIS (10) InfoQ (4) Java (2) Lean (3) Linq (2) MemoryLeaks (5) Microsoft (37) MVC (1) NDC (2) NNUG (36) Other (10) Patterns (9) Performance (3) Scrum (17) Security (7) ServiceBus (1) Silverlight (4) Software (19) TeamManagement (11) TechEd (7) Testing (4) Tools (25) TvGuide (1) WCF (8) Web (15) WebDeploy (1) WIF (3) Windows (10) Vista (15) VisualStudio (16) WiX (9) Work (16) Workflow (3)  
         
ARCHIVE
   
         
BLOGROLL
   
         
ON THIS PAGE...