# Refactoring large switch statement [closed]

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:
break;
case KEY2:
break;
case KEY3:
break;
case KEY4:
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

-

## closed as off-topic by RubberDuck, Phrancis, Mast, Jamal♦Aug 16 at 16:04

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

You're passing in Map<Key, Value> map but you're only using the map.keySet(). Are you using the Values in the Map for anything, or passing them into the Processor subroutines? –  ChrisW Feb 19 '14 at 10:16
BEEP - BEEP - BEEP! Strategy Pattern Alert! –  Simon Forsberg Feb 19 '14 at 10:24
More fundamentally, see if you can't just have one processStuffAboutKey(...) and simply parameterize it based on a string/int, like @dss539. (Are the actions actually key-specific?) Unless yes, then Strategy Pattern is overkill. –  smci Feb 20 '14 at 0:11
They are very much key-specific and will do very different things –  avalancha Feb 20 '14 at 7:20
@SimonAndréForsberg BEEP BEEP BEEP you can also use the visitor pattern for more flexibility but added complexity! –  Adam Gent Feb 20 '14 at 18:08

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<>();
...


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.

-
Exactly what I was going to say, you beat me to it. StrategyPattern FTW! –  Simon Forsberg Feb 19 '14 at 10:24
since Keyis an enum, EnumMap should be used instead of HashMap. It's way more efficient (Effective Java, Item 33) –  Sean Patrick Floyd Feb 19 '14 at 14:00
@dss539 google.com/search?q=java+delegate suggests Java doesn't support such delegates. –  ChrisW Feb 19 '14 at 16:27
I might be missing something, but how is this any better than the switch solution? You just moved the 25 "cases" from one function to another (the one which initializes the map), plus you introduced 25 new classes. This is certainly "more object-oriented" than a switch, but that doesn't mean it's better. –  fredoverflow Feb 19 '14 at 17:34
You could even go ahead and save the corresponding key in the class itself (getProcessedKey()). Thus the creation of all those classes and the filling of the map could be "automated". I used this approach once for a similar problem and found it to be quite maintainable. –  Carsten Hoffmann Feb 19 '14 at 20:25

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 dont 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);
}

-
This is a good idea for small enums, but it is going to get messy when the enum grows. I'd prefer the EnumMap solution –  Sean Patrick Floyd Feb 19 '14 at 14:25
With the EnumMap solution you still have the boilerplate code to add all the processors to the map, and 25+ Processor classes. I think it's a matter of taste. –  Paul S Feb 19 '14 at 15:11
Yeah for this answer I'd say so, too. But could you give the exact item number in Effective Java you are referring to please? –  avalancha Feb 19 '14 at 16:27
This solution is the cleanest. You have identified the core problem being that MyKeyEnum really should be a base class with derived types KEY1, KEY2, etc. IMO, this enum shouldn't be an enum. Switch statement's like the OP's are a code smell that indicates an opportunity for polymorphism. –  dss539 Feb 19 '14 at 16:30
@avalancha Item 30 in Effective Java 2nd edition. Look for the example with enum Operator, and onwards. –  Paul S Feb 19 '14 at 19:46

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.

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.

-
The key is an enum I built and the value is String. Edited the question accordingly –  avalancha Feb 19 '14 at 11:48
You can put processStuff method in the enum using strategy pattern. –  abuzittin gillifirca Feb 19 '14 at 11:59
@avalancha How do you build the enum values? Instead of creating enum values, can you create instances of subclasses of an abstract class instead (or, select a flightweight singleton of a subclass)? –  ChrisW Feb 19 '14 at 16:59

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.

-
+1. It does not have two similar object inheritance trees (which is a smell); encapsulates the data and operation in the same class; if you have a new key you just have to create a new subclass and don't have to modify the code in several places, you won't forget to add a case to the switch, etc. (single responsibility principle, DRY); and you don't have a long class, just small ones. (But I'd use an interface, not an abstract parent class. :) –  palacsint Feb 20 '14 at 21:59

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.

-
I don't think using reflection would be a good advise –  gonzalon Feb 19 '14 at 14:23
@GonzaloNaveira: Please elaborate. IMO instead of remembering, I implement rules via Reflection. –  M. Mimpen Feb 19 '14 at 14:25
@Mimpen IMO in this case it doesn't worth it and I don't even mention the problems regarding the performances issues, refactoring and error prone it could be. By the other hand, yes, somethimes reflection is great; but not always. –  gonzalon Feb 19 '14 at 14:45

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>);
}
}

-