Java – How does this switch statement do “multiple things”

clean codejavaswitch statement

This doubt is about Switch Statements from Chapter 3: Functions of the book named Clean Code

Here we have a function:

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

From my inexperienced point of view, I can see that the switch statement inside the function calculatePay is only returning things based on the object Employee. Isn't it doing the "One Thing" mentioned by Uncle Bob?

But Uncle Bob says:

There are several problems with this function. First, it's large, and when new employee types are added, it will grow. Second, it very clear does more than more 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.

How does the switch statement do more than one thing?

Best Answer

Martin's concept of "do one thing" is overly ambiguous to the point I believe it does more harm than good.

In the passage Martin states that a switch does one thing per case and therefore by definition does N things. This implies that any single method call is "one thing". If you follow this thinking, you will quickly realize a program will never be able to do anything!

Martin has a different definition which is that a method does "one thing" when all operations are on the same abstraction level. But the cases here calculateCommissionedPay(), calculateHourlyPay() do seem to be on the same abstraction level, so this contradicts his general criticism of switch-statments as always doing N-things.

That said, there are reasons to avoid this particular use of a switch. The switch check a type-field and then selects the method to call based on that. The idiomatic way to do this in object oriented programming is to use subclasses and overloading. Employee could have subclasses HourlyEmployee, SalariedEmployee etc., which override a calculatePay() method. That way you could avoid the switch altogether and just call e.calculatePay().

But if the input really is an enum value as in the example, then you need a way to get the appropriate Employee-subclass given this enum value. How do you do that? A switch of course! So you end up with code something like this:

public Employee createEmployee(int employeeType)
throws InvalidEmployeeType {
    switch (employeeType) {
      case COMMISSIONED:
        return new CommissionEmployee(e);
      case HOURLY:
        return new HourlyEmployee(e);
      case SALARIED:
        return new SalariedEmployee(e);
      default: 
        throw new InvalidEmployeeType(e.type);
    }
}

public Money calculatePay(int employeeType)
throws InvalidEmployeeType {
    Employee e = createEmployee(e.type);
    return e.calculatePay();
}

You will notice a few things:

  1. We still have a switch which allegedly does "N-things".
  2. The switch will still have to grow when new employment types are added.
  3. We still have a Open/Closed violation, since adding a new subclass will require us to modify the switch, just as before.

But if there are multiple places in the code were we switch on the employee type as in the first example, this refactoring is a clear improvement since we now only need one single switch and can use overloading in the other places.

If it is not clear from the above, I reject the premise that switch statements are bad and should be avoided in general. Sometimes it is the right tool for the job. But certain uses of switch is an anti-pattern, for example using switch as a substitute for overloading, when overloading would be more appropriate.

Related Topic