Recently I was about to refactor some code Crap4j pointed me to. When I realized most of that code was some kind of switch-case or if-else-cascade, I remembered Daniel´s post and decided to obey those four rules.
This post is supposed to give some inspiration on how to get rid of code like:
switch (value) {
case SOME_CONSTANT:
//do something
break;
case SOME_OTHER_CONSTANT:
//do something else
break;
...
default:
//do something totally different
break;
}
or an equivalent if-else-cascade.
In a first step, let’s assume the constants used above are some kind of enum you created. For example:
public enum Status {
ACTIVE,
INACTIVE,
UNKNOWN;
}
the switch-case would then most probably look like:
switch (getState()) {
case INACTIVE:
//do something
break;
case ACTIVE:
//do something else
break;
case UNKNOWN:
//do something totally different
break;
}
In this case you don’t need the switch-case at all. Instead, you can tell the enum to do all the work:
public enum Status {
INACTIVE {
public void doSomething() {
//do something
}
},
ACTIVE {
public void doSomething() {
//do something else
}
},
UNKNOWN {
public void doSomething() {
//do something totally different
}
};
public abstract void doSomething();
}
The switch-case then shrinks to:
getState().doSomething();
But what if the constants are defined by some third-party code? Let’s adapt the example above to match this scenario:
public static final int INACTIVE = 0; public static final int ACTIVE = 1; public static final int UNKNOWN = 2;
Which would result in a switch-case very similar to the one above and again, you don’t need it. All you need to do is:
Status.values()[getState()].doSomething();
Regarding this case there is a small stumbling block, which you have to pay attention to. Enum.values() returns an Array containing the elements in the order they are defined, so make sure that order accords to the one of the constants. Furthermore ensure that you do not run into an ArrayOutOfBoundsException. Hint: Time to add a test.
There is yet another case that may occur. Let’s pretend you encounter some constants that aren’t as nicely ordered as the ones above:
public static final int INACTIVE = 4; public static final int ACTIVE = 7; public static final int UNKNOWN = 12;
To cover this case, you need to alter the enum to look something like:
public enum Status {
INACTIVE(4),
ACTIVE(7),
UNKNOWN(12);
private int state;
public static Status getStatusFor(int desired) {
for (Status status : values()) {
if (desired == status.state) {
return status;
}
}
//perform error handling here
//e.g. throw an exception or return UNKNOWN
}
}
Even though this introduces an if (uh oh, didn’t obey rule #4), it still looks much more appealing to me than a switch-case or if-else-cascade would. Hint: Time to add another test.
How do you feel about this technique? Got good or bad experiences using it?
Schneide Blog 
Sounds good. Will try it when I get the opportunity to do this.
Looks very cool. I’ve not been able to use this pattern much because our use of enums have been to label information that is processed differently as it passes into and out of our middleware stack. To put processing code in the enum would remove it from the class that “owns” the transformation and passing of the objects.
We used a switch. 🙂
Great idea, but I would make:
public Status getStatusFor(int desired)
a static method.
Hi Wim,
you’re absolutely right. The method should be static. Missed that when writing the post.
Thank you.
I don’t think this is a particularily useful design.
Mixing “state” information and the control/decision logic into the same class/enum defeats the single reponsibility principle.
You will then have to start passing in arguments to the doSomething() method to apply the business/domain logic depending on the enum.
What’s wrong with a switch statement anyway?
Exactly!
Nice tip! I would modify the Enum slightly. I use the following pattern to index and lookup Enums by index value. This works more reliably than ordinal values and the for loop is only needed once.
static public enum IndexedEnum {
CASE_0(0), CASE_1(1), CASE_2(2);
private IndexedEnum(final int indexValue) {
this.indexValue = indexValue;
}
public final int indexValue;
static private Map map =
new HashMap();
static {
for (IndexedEnum item : IndexedEnum.values()) {
IndexedEnum.map.put(item.indexValue, item);
}
}
static public IndexedEnum lookup(final Object key) {
return map.get(key);
}
}
You know I really don’t buy that pattern of avoiding switch.
Why ? Because when you have a huge code base and when you read a switch, it is crystal clear to understand what the code do.
When your enum is usen is dozen places, not all switch will do the same thing (otherwise you just have a single method not a dozen dealing with the enum).
You ENUM become bloated and illisible :
doSomething1, doSomething2… doSomethingN. And where doSomething1… doSomething10 are totally differents methods dealing with totally differents matter. No a good design practice.
If you use several external system, you’ll even have something like :
INACTIVE(2, 8, 2)
ACTIVE(1,7, 3)
UNKNOWN(3,0,1)
In fact trying to get rid of your switch make the code more complex most of the time, less maintenable, transform the ENUM in a doEverything class (violating the one object do one thing well principle).
I do admit that you pattern does have it’s usage. But ONLY when the abstracted doSomething method is a logical part of the ENUM behaviour/state.
Basically if you abtract an internal behaviour for the particular state, it is great. If you abstract an external behaviour but that depend of the state, you should avoid it at all cost.
It is likely anyway that if you add real behaviour to an enum, you don’t want an enum, but a classical object instance. Then it more logical to use polymorphism as needed.
nice first-step refactoring. the next one might be, replace conditional with polymorphism 🙂
Can’t you use the third library’s constants? Like:
public enum Status { INACTIVE(OTHER_LIB_INACTIVE), ACTIVE(OTHER_LIB_ACTIVE), UNKNOWN(OTHER_LIB_UNKNOWN); // ...You just discovered the good old GoF State design pattern, admittedly in an interesting syntactic form.
Another common way of getting rid of switch statements is to use the Strategy pattern, as per Fowler’s “Replace Conditional with Polymorphism” refactoring.
Does not sound good to me. In a big project with different layers, one use the enums just as a type, not doing some business logic. And enum are hard to test and mock. And, lets say you want to inject them, it doesn’t make any sense to inject them.
Using it locally for a code fragment could make sense.
Cheers.
The problem with placing logic in the enum is that it is going to know about the different ways it is going to be used, so you are creating cycles in your software and that is a bad thing. So I really would watch out for replacing switch case statements for virtual method calls.
it’s hard to change extend or override behaviour when it’s in the enum you might want to dispatch the logic to an other class
What’s wrong with good old switch? If I want action based on a key, I would use a Map where the values are anonymous implementations of an interface. It’s far more clear than the convoluted use of enums shown here, IMHO.
How about this design:
public enum Status {
INACTIVE {
public void compileSafeSwitch(CompileSafeSwitch dispatcher) {
dispatcher.inactive();
}
},
ACTIVE {
public void compileSafeSwitch(CompileSafeSwitch dispatcher) {
dispatcher.active();
}
},
UNKNOWN {
public void compileSafeSwitch(CompileSafeSwitch dispatcher) {
dispatcher.unknown();
}
};
public interface CompileSafeSwitch{
public void inactive();
public void active();
public void unknown();
}
public abstract void compileSafeSwitch(CompileSafeSwitch dispatcher);
public static class StatusTest{
public static void main(String[] args) {
final String inactiveText = “closured inactive text”;
final String activeText = “closured active text”;
final String unknownText = “closured unknown text”;
for (Status status : Status.values()) {
status.compileSafeSwitch(new CompileSafeSwitch(){
@Override
public void inactive() {
System.out.println(inactiveText);
}
@Override
public void active() {
System.out.println(activeText);
}
@Override
public void unknown() {
System.out.println(unknownText);
}
});
}
}
}
}
So you’ve replaced a switch statement with something else that looks just like a switch statement except it’s somewhere else. If it’s a single occurrence, what’s wrong with it being in line? I first encountered the switch-case type statement in Pascal some 35 years ago and found it to be one of the most useful features of the language. Anybody remember Cobol Level-88? A language that is about as cumbersome as you can get implementing a boolean function. It was way ahead of its time, yet instructors said “Don’t use 88s, they’re confusing”. So now you’re saying not to use one of the most useful and concise features of the language, in favor of something nearly identical embedded in an enum def – probably in a different source file? Next time I’m stepping code and have 39 source files open, I’ll think of you.
The approach is absolutly right, if it is logic that belongs to the enum. The problem with the switch approach is, if you want to edit your enum you have to change all switch statements this enum is used. This is not OOP. In OOP you want to use Polymorphism. In the shown approach you only change the enum and everything works as expected. Yes you only put the logic at a different location but thats the absolutly correct location. Everyone who does not understand this approach does not understand OOP.
There is still a problem.If the enum is used at many places and at each place a different operation is to be performed based on the enum values,then we need to have several methods in enums.
This would make the enum fat.
How do you suggest to solve this case?
Recent versions of java do very performance on String switch – they are basically hashmap implementations – so unless readability is a concern – switch case would be fine in newer versions of java