Elegant Code » Refactoring the Refactored Switch Statement

Refactoring the Refactored Switch Statement

January 31st, 2009

Semantic Competence and Disquotational Knowledge

In her book, Relevance and Linguistic Meaning: The Semantics and Pragmatics of Discourse Markers, Diane Blakemore speaks, among other things, of incoherent interpretation and contextual assumptions. While pondering these phrases, I was reminded of Chris Brandsma’s recent article on Refactoring a switch statement.

You see, it’s not that Chris Brandsma is wrong, per se… it’s that I believe that his advice in this matter applies, at best, to a very small number of people who better already understand what they are doing. For the large majority of developers who still misuse the word “refactoring”, the article merely builds on their disquotational knowledge.

Disquotational knowledge describes the concept of knowing something without understanding it. There is wealth of this knowledge in our industry. For me, it’s more important to firmly establish the 90% case and allow for discovery of the 10% case in time. To that end…

Sean Timm’s rules of proper switch statement etiquette:

If your code has multiple switch statements working with a common subset of case instances, the switch must go away.
If you find yourself about to write a switch statement:
- Stop, and mentally confirm that:
  - You’re not already doing another switch on a subset of these same case instances.
  - You don’t plan on doing another switch on a subset of these same case instances.
  - You can’t for the life of you figure out why anyone else would ever do another switch on a subset of these same case instances.
  - You are willing to get rid of this switch statement when you discover that you actually do need to switch on a subset of these same case instances.
- If any of the above are not true, the switch must go away.
If you can’t articulate precisely why a switch must stay, the switch must go away.
If you’re not working in a language that supports polymorphic behavior, ignore all of this advice, and do something like Chris suggested.

A Brief Analysis of the Problem

Let’s take a quick look at the original problem:

public string ReturnSomething(string name, string value)
{
    switch (name)
    {
        case "Chris":
            return _valueProcessor.Chris(value);
        case "David":
            return _valueProcessor.David(value);
 
        ...
    }
}

Can anyone tell me what will return when you pass “Chris” and another value into this function? Of course not… we’ve lost all semantic meaning in this code. We could fix this with something like the following:

public string ReturnSomething(string name, string value)
{
    switch (name)
    {
        case "Chris":
            return _valueProcessor.PrependLastNameOfChrisToValue(value);
        case "David":
            return _valueProcessor.PrependLastNameOfDavidToValue(value);
 
        ...
    }
}

Ok, that’s a bit awkward, but we’ll run with it. Since we wrote this code, we’ve discovered that we need to add some new functionality, so we stick to the code:

public string ReturnSomethingElse(string name, string value)
{
    switch (name)
    {
        case "Chris":
            return _valueProcessor.LookupNoteToChris(value);
        case "David":
            return _valueProcessor.LookupNoteToDavid(value);
 
        ...
    }
}

Pretty soon, your interface is starting to look like this:

public interface IValueProcessor
{
    string PrependLastNameOfChrisToValue(string data);
    string LookupNoteToChris(string noteKey);
    byte TheHandThatFeedsChris();
    bool AskChrisIfItIsReallyStillAGoodIdeaThatYouAreGoingDownThisPath();
    void AllKnowledgeOfWhatYouThoughtWasReallyAGoodIdeaThatYouGotFromChris();
 
    string PrependLastNameOfDavidToValue(string data);
    string LookupNoteToDavid(string noteKey);
    byte TheHandThatFeedsDavid();
    bool AskDavidIfItIsReallyStillAGoodIdeaThatYouAreGoingDownThisPath();
    ...
}

Dealing with Code Tumors

When you’re facing uncontrollable growth like this, it’s time to get in and do some surgery. It should have been dealt with back when it was easier to operate on because now it has spread throughout your code.

First, we introduce a test because we want to remove this malignant growth without interfering with the rest of the code’s ability to function:

[TestFixture]
public class SwitchRefactorDemoTest
{
    [Test]
    public void DoSomethingWithChrisTest()
    {
        var switchDemo = new SwitchDemo(new ValueProcessor());
        var result = switchDemo.ReturnSomething("Chris", "Hey");
        Assert.That(result, Is.EqualTo("BrandsmaHey"));
    }
 
    ...
}

ValueProcessor is being used directly in this test because we’re attempting to get rid of it (oh, I didn’t mention that?). Among other things, ValueProcessor is clearly violating the Single Responsibility Principle. This class is doing way too much! For the purposes of this discussion, I won’t get into details on why this is important, but feel free to add this to your disquotational knowledgebase.

For this example, we’ll be looking at the refactoring known as Replace Conditional with Polymorphism. You may also want to spend some time investigating Replace Conditional with Visitor as a solid combination of both these techniques can be required at times.

We’re refactoring, so we’ll go step by step and avoid breaking functionality. We should be able to stop at any time and get back to work on that new defect we just got assigned without worrying about the state of the code.

First, we’ll work with the Chris method of ValueProcessor which currently looks like:

public class ValueProcessor : IValueProcessor
{
    public string Chris(string data)
    {
        return "Brandsma" + data;
    }
 
    ...
}

We do an Extract Method on the logic, and things now look like:

public string Chris(string data)
{
    return PrependLastNameTo(data);
}
 
private string PrependLastNameTo(string data)
{
    return "Brandsma" + data;
}

We run the tests, and everything still passes. This will be our new method name, but we have a lot more work to do before the conditional is gone. For now, we’ll just Move Method and delegate our call to the new ChrisProcessor class that we create:

public string Chris(string data)
{
    var processor = new ChrisProcessor();
    return processor.PrependLastNameTo(data);
}

We run the tests, and everything still passes. Now we go ahead and do this for each of the other conditionals. We run the tests after each step, and everything continues to pass. At last, all of the logic has been delegated to these new classes, and the code in the functions looks the same other than which processor we’re instantiating.

We change ChrisProcessor to derive from ValueProcessor, add the PrependLastNameTo() method to the IValueProcessor interface and add a default implementation to ValueProcessor that throws an exception:

public class ValueProcessor : IValueProcessor
{
    public virtual string PrependLastNameTo(string data)
    {
        throw new System.NotImplementedException();
    }
    ...
}

This method is currently not called, and as we modify our conditional to call it, we shouldn’t get the exception because each subclass will override the method. ChrisProcessor is now ready to be fully responsible for its behavior, but we have an interesting situation with this code.

An IValueProcessor implementation is currently being passed in to the SwitchDemo class. This interface was being used by the switch statement to call into the correct conditional logic, but that can now happen automatically. Most likely, we aren’t going to need to pass the IValueProcessor in at all (since the whole SwitchDemo class will likely disappear).

For now, we’ll introduce a factory method that instantiates the correct ValueProcessor subclass (yes, I appreciate the irony of introducing a new switch method, but this is temporary and may disappear depending on the external callers… if not, use Chris’ dictionary for this ):

private class ValueProcessorFactory
{
    public static IValueProcessor GetValueProcessor(
        string processorKey,
        IValueProcessor valueProcessor)
    {
        switch(processorKey)
        {
            case "Chris":
                return new ChrisProcessor();
            default:
                return valueProcessor;
        }
    }
}

Note that we’re doing these one at a time, so we start with just replacing the ChrisProcessor and add the others as we go:

public string ReturnSomething(string name, string value)
{
    IValueProcessor processor = ValueProcessorFactory.GetValueProcessor(
        name, _valueProcessor);
    switch (name)
    {
        case "Chris":
            return processor.PrependLastNameTo(value);
        case "David":
             return _valueProcessor.David(value);
        ....
    }
}

Run the tests. Everything passes. Do the same thing with each of the other conditionals, and you finally end up with removing the conditional statement altogether:

public string ReturnSomething(string name, string value)
{
    IValueProcessor processor = ValueProcessorFactory.GetValueProcessor(
        name, _valueProcessor);
    return processor.PrependLastNameTo(value);
}

At this point, it’s likely that you don’t even need the ReturnSomething method, so you Inline Method and move the factory instantiation to wherever it makes sense. Now you go and do the same to all your other switch statements that you need to push into these subclasses. Lather, rinse, repeat.

Afterthoughts

When you’re disciplined about these things, you’ll find yourself realizing you could have taken a different path and gotten to the same place (or perhaps a better place) a lot sooner. While working through this example, I realized that rather than delegating the PrependLastNameTo method to the new ChrisProcessor subclass when I was getting started, I should have created a ChrisProcessor class that took IValueProcessor in its constructor. This would have allowed me to call into the old Chris() function in ValueProcessor while working to modify the conditional logic. In real code where things are more complicated than this, I probably would have needed access to the IValueProcessor beyond its use in the ReturnSomething method, so keeping it around for delegation purposes makes a lot more sense. Perhaps next time… – Sean Timm

Sean Timm Architecture, Unit Testing

Comments (2)

Chris Brandsma

January 31st, 2009 at 10:34 | #1

Reply | Quote

I’m feeling a slight need to defend myself here.

The main point of “Refactorring a switch statement was that a switch statement can be replaced with a Dictionary. Even if the switch is done correctly (with polymorphism), like you did here, can still contain a rather nasty switch statement in the guts.

For example: If you are writing a tag processor (you are taking in a file with custom, distinct, tags in that that indicated a need to perform an action), the list of possible tags can get quite large. One commenter on my post was talking about having 1500 case statements in a single switch because of this.

Everything was kept simple in that post to highlight one point: a Dictionary is an effective alternative. Even all the suggestions that people put on the blog ignored one fact: they had done everything but remove the switch. I wanted it GONE.

You moved the switch to another class, which had merely migrated an ugly piece of code to a new home — which is what you are supposed to do with code that is becoming unmanageable, but I wanted to make it less ugly from the start. No polymorphic trick will remove a switch statement (but it will make it easier to remove).

Part of this could just be me. I don’t hate the switch statement, I use them from time to time, but I think they should be used much more rarely than they are.

But, effective refactoring is a multi-step process. Thank you for showing one of the other steps.
Sean Timm

January 31st, 2009 at 11:02 | #2

Reply | Quote

It’s good to bring out the apologist in you, Chris. I completely agree with your desire to eliminate the switch statements, and it’s rare that I run across one that wouldn’t be better off in a different form.

My main concern called out in this article is that the majority of switch statements I’ve run into in production code involve multiple switches with multiple responsibilities and invariably would be better off in polymorphic form.

In these cases, your dictionary refactor is an effective addition to the steps called out in this article (in the case where creation of the appropriate subclass needs handled by a switch), but the main problem in the code isn’t the existence of a switch but the use of it.

Comments are closed.

Life With My Geek – Sexy Bag Setting up a FubuMVC Project from Scratch

Refactoring the Refactored Switch Statement

Semantic Competence and Disquotational Knowledge

Sean Timm’s rules of proper switch statement etiquette:

A Brief Analysis of the Problem

Dealing with Code Tumors

Afterthoughts

Recent Comments

Our Community

Disclaimer

Administrivia