In Java 1.5 enums were introduced to ease handling of constant values. Enums can improve code quality, because they are types that the compiler can check. But they can also be misused and lower the code quality. In this blog I want to discuss the misuse of enums as type discriminators and show you why this kind of usage is an anti-pattern from an object-oriented perspective.
Type-switches
What I call a “type-switch” is just any conditional statement that does type checks in order to execute type specific code. Here are some examples of type switches.
HumanState humnanState = ...; if(humanState instanceof HappyState){ singHappySong(); } else if(humanState instanceof SadState){ singDirge(); }
HumanState humanState = ...; switch (humanState) { case HAPPY: singHappySong(); break; case SAD: singDirge(); break; }
public static final String STATE_HAPPY = "HAPPY"; public static final String STATE_SAD = "SAD"; ... String humanState = ...; if(STATE_HAPPY.equals(humanState)){ singHappySong(); } else if(STATE_SAD.equals(humanState)){ singDirge(); }
In this blog I want to focus on enums as type switches, but the basics behind it will apply to any type-switch.
Enum as type discriminator
In some tutorials and even production code you will see enum types that are used as type-switches. The example code I want to use here is taken from another blog “Of Hacking Enums and Modifying final static Fields“by Dr. Heinz M. Kabutz. This blog is about testing issues that arise because of the type-switch usage of enums and how to solve them.
Let’s assume you want to design a human beings and let they sing depedent on their state. Some developers might want to do it this way:
public enum HumanState { HAPPY, SAD } public class Human { private HumanState humanState = HumanState.HAPPY; public void somethingBadHappened(){ this.humanState = HumanState.SAD; } public void somethingGoodHappened(){ this.humanState = HumanState.HAPPY; } public void sing() { switch (humanState) { case HAPPY: singHappySong(); break; case SAD: singDirge(); break; default: new IllegalStateException("Invalid State: " + state); } } private void singHappySong() { System.out.println("When you're happy and you know it ..."); } private void singDirge() { System.out.println("Don't cry for me Argentina, ..."); } }
Problems with this design:
- single responsibility principle is violated
The single responsibility principle says that there should only be one reason to change. But the method sing() will change whenever a HumanState is added or removed. - open-close principle is violated
The open-close principles states that we should write our code in a way it is open for extension, but closed for modification. But what will happen if you want to add another HumanState? You have to add a new value to the enum and find and update all HumanState-Switches in your code. Thus you have change existing code instead of adding new one. - cyclomatic complexity increases fast
Cyclomatic complexity is a software metric that indicates the complexity of a program. It is a quantitative measure of the number of linearly independent paths through the source code. Therefore the complexity of the method sing() increases with every new human state . - Hard unit-testing and code coverage
In order to test the complete behavior of the sing() method one must cause the switch statement to fall through to the default case. But this means that one must mock the enum HumanState, because the normal “production” code doesn’t define an illegal value for a human state. Take a look at “Of Hacking Enums and Modifying final static Fields” to see the effort you have to make to get the default case tested. - Default case handling
Most times developers add a default case to be remembered when a new enum value is added and to prevent unexpected behavior. Thus you must also implement a default case even if it is never executed until the enum values change.
Type-Switches in an anemic design
An anemic design inverses the object-oriented way. Instead of calling methods on objects (data structures) you pass data structures into methods.
In this king of design you often have to deal with type-switches as well. E.g.
public class OrderService { public void placeOrder(Order order){ ... if("complete".equals(order.getState())){ .... } ... } }
In a real world application there will be many services that take an order object and you will often see that a lot of services do type checks or in other words… execute logic dependent on some sort of type discriminator.
This leads to the problem that the type-switch will be distributed all over the code base. As a result of this it is hard to estimate the impact of a type-switch change.
An object-oriented way eliminates type-switches
Let us remember the first example of the design of human beings and their state management. If we analyse the code above we will recognize that the human state behavior depends on a state and we can desing the human state as an own class or better an interface.
public interface HumanState { public void sing(); }
Now we can move the sing() logic into dedicated implementations. E.g.
public class Happy implements HumanState { public void sing(){ System.out.println("When you're happy and you know it ..."); } } public class Sad implements HumanState { public void sing(){ System.out.println("Don't cry for me Argentina, ..."); } }
After we have done this the Human class will become much easier.
public class Human { private HumanState humanState = new Happy(); public void somethingBadHappened(){ this.humanState = new Sad(); } public void somethingGoodHappened(){ this.humanState = new Happy(); } public void sing() { humanState.sing(); } }
The difference
- single responsibility principle is respected
- cyclomatic complexity for the sing method is 1 … so the implementations of HumanState
- Every state can be independently unit tested.
- There is no default case anymore. So we don’t have to test it.
Type-switches and the factory method pattern
Also the factory pattern often suffers from type-switches. The next example is taken from a stackoverflow question.
public static Pizza createPizza(String type) { Pizza pizza = null; if(type.equals(PizzaType.Cheese)) { pizza = new CheesePizza(); } else if (type.equals(PizzaType.Tomato)) { pizza = new TomatoPizza(); } else if (type.equals(PizzaType.Capsicum)) { pizza = new CapsicumPizza(); } else { try { throw new Exception("Entered PizzaType is not Valid"); } catch (Exception e) { // TODO Auto-generated catch block e.printStackTrace(); } } return pizza; }
You can also eliminate this type switch by introducing an interface and using a map.
public interface Factory<T> { public T newInstance(); } public class TomatoPizzaFactory implements Factory<TomatoPizza> { public TomatoPizza newInstance() { return new TomatoPizza(); } } public class PizzaFactory { private Map<String, Factory<? extends Pizza>> factories = new HashMap<String, Factory<? extends Pizza>>(); public PizzaFactory(){ factories.put(PizzaType.Cheese, new CheesePizzaFactory()); factories.put(PizzaType.Tomato, new TomatoPizzaFactory()); } public Pizza createPizza(String type){ Factory<? extends Pizza> factory = factories.get(type); if(factory == null){ throw new IllegalArgumentException("Unknown pizza type"); } return factory.newInstance(); } }
You might also want to implement a default constructor factory for the simplest case.
public class DefaultConstructorFactory<T> implements Factory<T> { private Class<T> type; public DefaultConstructorFactory(Class<T> type) { this.type = type; } public T newInstance() { try { return type.newInstance(); } catch (InstantiationException e) { throw new IllegalStateException("Can not instantiate " + type, e); } catch (IllegalAccessException e) { throw new IllegalStateException("Can not instantiate " + type, e); } } }
Conclusion
If you eliminate type-switches you will benefit from
- single responsibility
- easy unit testability
- less cyclomatic complexity
- eventually eliminate default case handling
I like this.
The sing example is unquestionable, the code not using type-switch is much better.
The Factory example:
public PizzaFactory(){
factories.put(PizzaType.Cheese, new CheesePizzaFactory());
factories.put(PizzaType.Tomato, new TomatoPizzaFactory());
}
Creates a bunch of Factory instances that are potentially never used (if a customer orders 1 pizza), whereas the type-switch creates the instances on-demand, how could we improve on this?
Lastly, to stay on point and avoid confusing some readers, the two createPizza implementation must be similar, therefore to make the map based solution conform to the type-switch example, i think we can get rid off the Factory interface, then have the map store actual instances instead of PizzaTypeFactories, that is:
public PizzaFactory(){
pizzaTypes.put(PizzaType.Cheese, new CheesePizza());
pizzaTypes.put(PizzaType.Tomato, new TomatoPizza());
}
public Pizza createPizza(String type){
Pizza pizza = pizzaTypes.get(type);
if(pizza == null){
throw new IllegalArgumentException("Unknown pizza type");
}
return pizza;
}
You’re right. The PizzaFactory creates factories that are potentially never used.
You might want to get around this by introducing a lazy loding proxy and maybe a WeakReferenceMap or LRUMap to release factories that are not needed. But this adds to much complexity and will only be useful if the factory objects would consume a lot of memory. In this example I decided to use the simplest form in order to focus on the main concept.
Your approach of putting pizzas directly into the map can also be used if you see it as a prototype pattern. In the prototyp pattern you create one object that is the prototype for others. usually you make the prototype cloneable. Sometime you might want to make it Serializable. it depends on your needs.
In my mind the PizzaFactory is not made for one customer. I mean that you would not instantiate one PizzaFactory for each order request. It should be used by all order requests.
By the way, I do see the benefit of using the Factories as the Factory.newInstance() will help with safety if customer wants more than one of the same pizza, like 2 Tomato pizzas but 1 without some ingredient