According to Robert C. Martin, the SRP states that:
There should never be more than one reason for a class to change.
However, in his book Clean Code, chapter 3: Functions, he shows the following block of 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 then states:
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. [emphasis mine]
Firstly, I thought the SRP was defined for classes, but it turns out it’s also applicable to functions. Secondly, how is that this function has more than one reason to change? I can only see it changing because of a change on Employee.
2
One often missed detail of the Single Responsibility Principle is that the “reasons for change” are grouped by use-case actors (you can see a full explanation here).
So, in your example, the calculatePay method will need to be changed whenever new types of Employees are required. Since one type of employee may have nothing to do with another, it would be a violation of the principle if you keep them together, since the change would affect different user-groups (or use-case actors) in the system.
Now, about whether the principle applies to functions: Even if you have a violation in only one method, you are still changing a class for more than one reason, so it is still a violation of SRP.
10
When Mr. Martin applies the SRP to a function, he’s implicitly extending his definition of SRP. As the SRP is an OO-specific wording of a general principle, and since it’s a good idea when applied to functions, I don’t see a problem with that (although it might have been nice if he’d explicitly included it in the definition).
I don’t see more than one reason to change either, and I don’t believe that thinking of the SRP in terms of “responsibilities” or “reasons to change” is helpful. Essentially what the SRP is getting at is that software entities (functions, classes, etc.) should do one thing and do it well.
If you take a look at my definition, it’s not any less vague than the usual wording of the SRP. The problem with usual definitions of the SRP is not that they’re too vague, but that they try to be too specific about something that is essentially vague.
If you look at what calculatePay does, it is clearly doing a single thing: dispatch based on type. Since Java has built-in ways of doing type based dispatch, calculatePay is inelegant and non-idiomatic, so it should be rewritten, but not for the stated reasons.
On page 176, Chapter 12: Emergence, in the section titled Minimal Classes and Methods the book provides somewhat of a correction, by stating:
In an effort to make our classes and methods small, we might create
too many tiny classes and methods. So this rule suggests that we also
keep our function and class counts low
and
High class and method counts are sometimes the result of pointless dogmatism.
Obviously, he is talking about dogmatism in following the SRP to break down perfectly fine innocent little methods like calculatePay() above.
You are right @Enrique. No matter if it’s a function or a method of a class, the SRP means that in that code block you only do one thing.
The ‘reason to change’ statement is sometimes a bit misleading, but if you change calculateSalariedPay or calculateHourlyPay or the enum of Employee.type you have to change this method.
In your example the function:
- checks the type of the employee
- calls another function which calculates the money according to the type
In my opinion it’s not directly a SRP violation, since the switch-cases and the calls cannot be written shorter, if you think of Employee and the methods already exists. Anyway it’s a clear open-closed principle (OCP) violation as you must append ‘case’ statements if you add employee types, so it’s a bad implementation: refactor it.
We don’t know how the ‘Money’ should be calculated, but the easiest way is to have Employee as interface and some concrete implementations with getMoney methods. In that case the whole function is needless.
If it’s more complicated to calculate it, one could use the visitor-pattern which is also not 100% SRP but it’s more OCP than a switch case.
2