Question About Example In Robert C Martin's _Clean Code_

Question

This is a question about the concept of a function doing only one thing. It won't make sense without some relevant passages for context, so I'll quote them here. They appear on pgs 37-38:

To say this differently, we want to be able to read the program as though it were a set of TO paragraphs, each of which is describing the current level of abstraction and referencing subsequent TO paragraphs at the next level down.

To include the setups and teardowns, we include setups, then we include the test page content, and then we include the teardowns. To include the setups, we include the suite setup if this is a suite, then we include the regular setup.

It turns out to be very difficult for programmers to learn to follow this rule and write functions that stay at a single level of abstraction. But learning this trick is also very important. It is the key to keeping functions short and making sure they do “one thing.” Making the code read like a top-down set of TO paragraphs is an effective technique for keeping the abstraction level consistent.

He then gives the following example of poor code:

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);
}
}

and explains the problems with it as follows:

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 Principle7 (SRP) because there is more than one reason for it to change. Fourth, it violates the Open Closed Principle8 (OCP) because it must change whenever new types are added.

Now my questions.

To begin, it's clear to me how it violates the OCP, and it's clear to me that this alone makes it poor design. However, I am trying to understand each principle, and it's not clear to me how SRP applies. Specifically, the only reason I can imagine for this method to change is the addition of new employee types. There is only one "axis of change." If details of the calculation needed to change, this would only affect the submethods like "calculateHourlyPay()"

Also, while in one sense it is obviously doing 3 things, those three things are all at the same level of abstraction, and can all be put into a TO paragraph no different from the example one: TO calculate pay for an employee, we calculate commissioned pay if the employee is commissioned, hourly pay if he is hourly, etc. So aside from its violation of the OCP, this code seems to conform to Martin's other requirements of clean code, even though he's arguing it does not.

Can someone please explain what I am missing?

Thanks.

Answer 1

There appears to be two reasons for calculatePay to change:

1. Changes in employee types
2. Changes in pay calculations

Two different axes of change. However, calculatePay method's responsibility is pay calculation. It should only change if there is a change in the pay calculation formula. I think this is why the author states that the method violates SRP.

In the book, the author provides a solution where he defines classes for each employee type derived from a common Employee abstract base class. He moves calculatePay method to Employee base class and defines an Employee factory class that creates appropriate employee objects given an employee type. This way each pay calculation is encapsulated in a specific employee type class and therefore not affected by changes in employee types. Also the employee factory class in this simple solution is only affected by changes in employee types. So the new solution makes SRP happy.

In the new solution, you ask an employee to calculate his/her pay which I don't like because this does not reflect the reality. You can actually argue that this too is a violation of SRP. This calculation is the responsibility of the payroll department. I like it where the model in software represents the real world domain as much as possible but we usually have to make compromises. In this case, I would say that changes in employee types are not going to happen on regular bases. In fact, they will most likely occur very rarely. So I would keep things where they make sense business domain wise such as asking payroll object to calculate employee pay. At the same time, I would have and keep an extensive suit of unit tests as one should have to ensure that when a change in employee type occurs everything continues to work as expected.