3

According to the book "Clean Code" on page 38, the following lines of code violate the single responsibility principle. However, I cannot understand how there are "multiple" reasons for it to change?

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

It seems to me that the purpose of this class is singular: to calculate the pay of commissioned, hourly, or salaried employees--and these three functions are at the same level of abstraction and one level below the top-most CalculatePay(). Is he referring to the fact that more employee types might be added?

fibono
  • 139
  • What exactly does the book say about that example? – Ben Aaronson Sep 11 '15 at 20:56
  • Seems like there are at least two reasons to change here. 1) an employee type is added or removed, or 2) the method of calculating a particular pay changes. – 8bittree Sep 11 '15 at 20:56
  • 1
    Voting to close as duplicate of the other question. This one leaves out the vital quote from the book that makes it clear that the claim is that the function itself is what violates the SRP, not the whole class. Both current answers are based on the opposite assumption – Ben Aaronson Sep 11 '15 at 21:04
  • One reason is pointed to by author in the paragraph that immediately follows the listing: "it must change whenever new types are added".

    Given this, it is easy to notice that other reasons for change are removal and rename of existing types. Since function also involves InvalidEployeeType, it would have to change if something is changed in this exception, like rename or change in constructor arguments.

     – gnat Sep 11 '15 at 21:06
  • 1
    Suppose you add an Intern employee type, a Volunteer employee type, etc. The class is changing for multiple reasons from a use case point of view (support Interns, support Volunteers, etc). Each is its own reason to change. – ravibhagw Sep 11 '15 at 21:07

1 Answers1

1

There are five concerns here:

  • Determine which type of pay to calculate.
  • Calculate commissioned pay.
  • Calculate hourly pay.
  • Calculate salaried pay.
  • Validate the employee type.

What might compel us to modify the class containing this method?

  • Perhaps we add a new type of employee. Now we need to add new calculation logic.
  • Perhaps we modify an existing type of employee. Now we need to modify an existing employee type's logic.

Multiple types of employees have their calculations in the same class. Each type of employee is a reason for this class to change. Hence, it violates the Single Responsibility Principle.

Moving each pay type to its own class (think: Strategy pattern) would localize the reasons to change to one class per employee type, giving each one reason to change.

Finally, using dynamic dispatch to select the pay calculation further separates the concerns by offloading the selection and validation logic itself to the compiler.

  • What if the private methods call into other classes? – Ben Aaronson Sep 11 '15 at 21:02
  • @BenAaronson irrelevant. The selection logic is still in that class, increasing its responsibilities. The issue is with that conditional, and the solution is to use dynamic dispatch by calling a method on the enum itself. –  Sep 11 '15 at 21:04
  • I agree this isn't the right design, but the question is how this violates the SRP. If three of your concerns are actually contained in other classes, then that leaves 1, doesn't it? – Ben Aaronson Sep 11 '15 at 21:05
  • Still irrelevant. There are private methods on that class which the code structure calls. If they delegate, it does not matter: the class in question still has too many responsibilities. What if a new type is added? Now we need a new method, and it may or may not delegate. No matter what, it still has the first and last responsibility listed in the bullet list in my answer. –  Sep 11 '15 at 21:10
  • If the private methods were delegating to other classes, they wouldn't change if the calculation changed. And again, yes, I agree there are good reasons that this isn't the right design. Your last paragraph, for example, accurately describes why this violates DRY. But that's not what's being asked – Ben Aaronson Sep 11 '15 at 21:13
  • @BenAaronson do they delegate? You assume they do, but that is not specified. All we know is there are multiple methods in the class related to calculating different types of pay. Please show me in the book where that class delegates to other classes. –  Sep 11 '15 at 21:15
  • There are one, maybe two responsibilities in your bullet points: calculation of pay and validation of employee type. If validation of employee type is only done relative to pay, then there is a single responsibility here. – Robert Harvey Sep 14 '15 at 15:32