Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

share|improve this question
1  
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 at 10:16
18  
BEEP - BEEP - BEEP! Strategy Pattern Alert! –  Simon André Forsberg Feb 19 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 at 0:11
    
They are very much key-specific and will do very different things –  avalancha Feb 20 at 7:20
1  
@SimonAndréForsberg BEEP BEEP BEEP you can also use the visitor pattern for more flexibility but added complexity! –  Adam Gent Feb 20 at 18:08

6 Answers 6

up vote 33 down vote accepted

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:

share|improve this answer
5  
Exactly what I was going to say, you beat me to it. StrategyPattern FTW! –  Simon André Forsberg Feb 19 at 10:24
6  
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 at 14:00
2  
@dss539 google.com/search?q=java+delegate suggests Java doesn't support such delegates. –  ChrisW Feb 19 at 16:27
6  
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 at 17:34
3  
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 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);
}
share|improve this answer
1  
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 at 14:25
3  
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 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 at 16:27
3  
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 at 16:30
    
@avalancha Item 30 in Effective Java 2nd edition. Look for the example with enum Operator, and onwards. –  Paul S Feb 19 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.

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.

share|improve this answer
    
The key is an enum I built and the value is String. Edited the question accordingly –  avalancha Feb 19 at 11:48
1  
You can put processStuff method in the enum using strategy pattern. –  abuzittin gillifirca Feb 19 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 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.

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

share|improve this answer
    
+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 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.

share|improve this answer
1  
I don't think using reflection would be a good advise –  gonzalon Feb 19 at 14:23
    
@GonzaloNaveira: Please elaborate. IMO instead of remembering, I implement rules via Reflection. –  M. Mimpen Feb 19 at 14:25
2  
@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 at 14:45

I can't see where the problem is. Your function may be long, but it does what it is supposed to do in a way that is very straightforward and easy to understand. You can probably find some clever way to do it differently, but "clever" is not a desirable attribute for code.

share|improve this answer
2  
It violates DRY, Dont Repeat Yourself, that is the problem. –  konijn Feb 19 at 23:37
    
@konijn Isn't the only repetition, the repetition of entry.getValue()? –  ChrisW Feb 19 at 23:45
1  
@ChrisW: No! the yukky part is for n keys there will be n functions 'processStuffAboutKey*', which are probably all/mostly largely/completely unnecessary. Object bloat for no reason. –  smci Feb 20 at 0:14
1  
They need to be n functions indeed, they all do very different things depending on the key at hand, some will invoke DB handling, some will just print stuff etc... I find it near impossible to imagine that I can simply use the same function and parameterize it well –  avalancha Feb 20 at 7:19
1  
Not that I'm recommending it but in the accepted answer you would set the breakpoint on the Key3Processor.processStuff method. –  ChrisW Feb 20 at 11:04

protected by rolfl Feb 20 at 11:16

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.