Refactoring large switch statement [closed]

Question

Lately I have been trying to extract till I drop. Well not necessarily till I drop, but I've been trying to be more strict and look at some metrics of my code.

I have now come along an old class of mine which has a rather large switch case. Metrics say the method lines of code are beyond evil. It goes something like this:

private void startProcessing(Map<MyKeyEnum, String> map) {
    Processor myProcessor = new Processor();
    for (Entry entry : map.entrySet()) {
        switch(entry.getKey()) {
            case KEY1:
                myProcessor.processStuffAboutKey1(entry.getValue());
                break;
            case KEY2:
                myProcessor.processStuffAboutKey2(entry.getValue());
                break;
            case KEY3:
                myProcessor.processStuffAboutKey3(entry.getValue());
                break;
            case KEY4:
                myProcessor.processStuffAboutKey4(entry.getValue());
                break;
            ...
            ...
        }
    }
}

So basically you can gather that it is necessary for me to invoke very different things for each key IF it is in the map. I have therefore already created the Processor class and mind you this is already the absolute shortest and most compact that I was able to come up with. Basically this method does already do only one thing.

But isn't it possible to build this differently so that it is shorter? I currently have 25+ cases to handle

Edit: made clear what Key,Value-Pair is and what they're used for

Answer 1

You can create a KeyProcessor interface and implementation for every key (move the body of the current Processor.processStuffAboutKeyX() methods to these classes):

public interface KeyProcessor {
    void processStuff(String data);
}

Then fill a map with the available implementations:

Map<Key, KeyProcessor> processors = new HashMap<>();
processors.add(key1, new Key1Processor());
...
processors.add(key4, new Key2Processor());

And use that map in the loop:

for (Entry<Key, String> entry: map.entrySet()) {
    Key key = entry.getKey();
    KeyProcessor keyProcessor = processors.get(key);
    if (keyProcessor == null) {
        throw new IllegalStateException("Unknown processor for key " + key);
    }
    final String value = entry.getValue();
    keyProcessor.processStuff(value);
}

With the original 25+ cases the switch-case method is at least 75 lines long (25 case statement, 25 break statement, and at least 1-1 method call line for every case). The separate classes reduces the size and complexity which a developer see at once (+no need to scroll), the code is easier to maintain and test because you can test every case in isolation.

I think it's a lot more easier to have (and handle) a couple of small classes and corresponding *Test classes (with a few test methods for every class) than having a big class (25+ cases) and a lot of tests in one corresponding *Test class or a lot of *Test classes which test separate case branches of the same startProcessing method.

I admit that these arguments might be weak ones but without an actual implementation of the Processor class and the code which creates the input map is hard to say more.

Further readings:

• Refactoring: Improving the Design of Existing Code by Martin Fowler: Replacing the Conditional Logic on Price Code with Polymorphism
• Replace Conditional with Polymorphism

Answer 2

How about adding the functionality of the process methods to the enum itself? This way the enum itself knows how to process, and if you add a value to the enum you don't have to search for all switch statements in your code base to update these.

You can read more about this approach in Effective Java, by Joshua Bloch.

Something like this:

private void startProcessing(Map<MyKeyEnum, String> map) {
    for (Entry entry : map.entrySet()) {
    entry.getKey().process(entry.getValue());
    }
}


public enum MyKeyEnum {
    VALUE1 {
        public void process(String s) {
            // do specific processing for VALUE1
        }
    },
    VALUE2 {
        public void process(String s) {
            // do specific processing for VALUE2
        }
    }
    ...
    ;

    public abstract void process(String s);
}
```

Answer 3

You can replace each case in the switch statement with a virtual function call, as shown in palacsint's answer.

IMO palacsint's answer is little better (and arguably worse: more lines of code) than the original code. For example when you need to add support for a new key value, in your code you need to add a new case statement, and in palacsint's code you need to add a new class (derived from KeyProcessor) and add a new entry to the processors map.

palacsint's strategy is much more worth implementing if you have two switch statements which do various things based on the key value (in which case KeyProcessor has more than one abstract method, a different method to replace each switch statement). Having one such switch statement isn't necessarily a bad idea.

See also Large Switch statements: Bad OOP?

Also, you haven't said what types Key and Value are:perhaps one of these types could be constructed such that it contains an appropriate abstract method.

Answer 4

Your enum should actually be a base class with the virtual method processStuff(string). Each key should be it's own class and should implement the virtual method.

This approach is similar to answer by Paul S but is more true to OO style.

Applying the strategy pattern here is not a good idea in my opinion because it hides the core design problem and creates a lot of boilerplate.

Of course, I'm not a fan of the strategy pattern in general:

The Strategy pattern is beautiful on the surface, but Strategy objects are typically stateless, which means they're really just first-order functions in disguise. (If they have state, then they're Closures in disguise.) Etc. You either get this, because you've done functional programming before, or you don't, in which case I sound like I'm a babbling idiot, so I guess I'll wrap it up.

https://sites.google.com/site/steveyegge2/singleton-considered-stupid

Answer 5

An addition to the strategy and code of @palacsint is to use Reflection to automatically bind the classes that implement KeyProcessor to the dictionary. This way, you don't have to add each KeyProcessor manually and you don't have to remember it.

Make sure that you only 'reflect' once because it is relatively slow. Thus add it to a static block or in the constructor of a singleton.

Answer 6

In line with @M. Mimpen's answer, get all methods of Processor object and store them in Map<MyKeyEnum, Method>, where the key refers to the key from your parameter map.

An alternative to this, is putting an entry to Map every time an entry is added to your map (the one you pass to the method).

//assume there is a populated Map<MyKeyEnum, Method>. Let's say it's named processorMethodMap

 private void startProcessing(Map<MyKeyEnum, String> map) {
    Processor myProcessor = new Processor();
    for (Entry entry : map.entrySet()) {
         //call the appropriate method
         processorMethodMap.get(entry.getValue()).invoke(<pass argument(s) here>);
    }
 }