Code Smell – Bad Smell in Code : Understanding Switch Statements

It’s hard to make a small switch statement. And, of course, I include if/else chains in this. Even a switch statement with only two cases is larger than I’d like a single block or function to be.

It’s also hard to make a switch statement that does one thing. By their nature, switch statements always do N things. Unfortunately we can’t always avoid switch statements, but we can make sure that each switch statement is buried in a low-level class and is never repeated. We do this, of course, with polymorphism.

It shows just one of the operations that might depend on the type of employee.

PayrollProcess.java

public Money calculatePay(Employee e) throws InvalidEmployeeType {
 switch (e.type) 
 {
	 case WAGES:
	 return calculateWagesPay(e);
	 case HOURLY:
	 return calculateHourlyPay(e);
	 case SALARIED:
	 return calculateSalariedPay(e);
	 default:
	 throw new InvalidEmployeeType(e.type);
 }
 }

There are several problems with this function. First, it’s large, and when new employee types are added, it will grow. Second, it very clearly does more than one thing.

Third, it violates the Single Responsibility Principle (SRP) because there is more than one reason for it to change. Fourth, it violates the Open Closed Principle (OCP) because it must change whenever new types are added. But possibly the worst problem with this function is that there are an unlimited number of other functions that will have the same structure.

Solution

The solution to this problem is to bury the switch statement in the basement of an ABSTRACT FACTORY, and never let anyone see it. The factory will use the switch statement to create appropriate instances of the derivatives of Employee, and the various functions, such as calculatePay, isPayday, and deliverPay, will be dispatched polymorphically through the Employee interface.

public abstract class Employee {
 public abstract boolean isPayday();
 public abstract Money calculatePay();
 public abstract void deliverPay(Money pay);
}

public interface EmployeeFactory {
 public Employee makeEmployee(EmployeeRecord r) throws InvalidEmployeeType;
}

public class EmployeeFactoryImpl implements EmployeeFactory {
 public Employee makeEmployee(EmployeeRecord r) throws InvalidEmployeeType {
 switch (r.type) {
 case WAGES:
 return new WagesEmployee(r) ;
 case HOURLY:
 return new HourlyEmployee(r);
 case SALARIED:
 return new SalariedEmploye(r);
 default:
 throw new InvalidEmployeeType(r.type);
 }
 }
}

First, most people use switch statements because it’s the obvious brute force solution, not because it’s the right solution for the situation. So this heuristic is here to remind us to consider polymorphism before using a switch.

Second, the cases where functions are more volatile than types are relatively rare. So every switch statement should be suspect.

I use the following “ONE SWITCH” rule: There may be no more than one switch statement for a given type of selection. The cases in that switch statement must create polymorphic objects that take the place of other such switch statements in the rest of the system.