11

I am quite confused on how to determine if a single method has one responsibility being done just like the following code from the book Clean 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);
        }
    }

As the author stated on this code snippet: "...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.". On a first glance at the code I was thinking that how come that the method violates SRP since if there is a change in the code it would be the switch statement only if there will be an added employee type but as I try to understand the method further I came up with a hypothesis on why it violates the said principle.

My hypothesis is that since the name of the method is calculatePay(Employee e) then the only responsibility of this method is for the payment computation as the method's name suggest but since inside the method there is a switch on filtering the type of Employee this filtering is now a different or another responsibility thus violates SRP. I don't know if I got it right.

Dave Schweisguth
  • 36,475
  • 10
  • 98
  • 121
anathema
  • 947
  • 2
  • 15
  • 27

2 Answers2

12

"...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."

There lies your answer. Every time a new Employee.Type is added that method will have to change. Also, the calculation for each Employee.Type might change too.

A better solution would be to create an Abstract Factory for Employee and each derivative of Employee having it's own implementation of CalculatePay. This way, only ONE class needs to changed when the Calculation changes or when a new Employee.Type is added.

Here is another extract from Clean Code that explains in more detail -

The solution to this problem (see Listing 3-5) is to bury the switch statement in the basement of an ABSTRACT FACTORY,9 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. My general rule for switch statements is that they can be tolerated if they appear only once, are used to create polymorphic objects

Janus Pienaar
  • 1,083
  • 7
  • 14
  • thank you for the input, I would like to as on the `Also, the calculation for each Employee.Type might change too`. How does the calculation for the employee would affect the method? Since each type is just calling a method to perform the calculation, I think, and also based on your answer and on the statement given by the book can I assume that my hypothesis on how I understand the principle is correct? – anathema Oct 23 '15 at 00:25
  • 2
    The [Single Responsibility Principal](https://en.wikipedia.org/wiki/Single_responsibility_principle) should be seen in the context of a Class and not necessarily on Method level. This is why I'm saying that if any calculation changes you would need to update this class. If you moved this Method to a derivative of Employee, then only that class will have to change if the calculation needs to change. – Janus Pienaar Oct 23 '15 at 05:50
  • @anathema also, if e.g. we want to calculate hourly payment differently (let's say working on weekend has an effect on the salary) then we might change calculateHourlyPay function signature (adding a boolean flag or a datetime), hence the calculatePay function is also need to be changed. – Robert F. Mar 29 '17 at 10:22
6

I generally apply the SRP at the class level. It prevents classes becoming too big and taking on too many roles.

I treat the 'responsibilities' as conceptual. Therefore, I would argue that your method has only one responsibility: to calculate pay.

I try and follow Google's guidelines on this and look for these warning signs that suggest you're wandering away from the SRP:

  • Summarising what the class does includes the word ‘and’.
  • Class would be challenging for new team members or inexperienced developers to read and quickly ‘get it’.
  • Class has fields that are only used in some methods.
  • Class has static methods that only operate on parameters.

On another note, however, your code contains a switch statement that suggests you might be wandering away from the OCP...

David Osborne
  • 6,436
  • 1
  • 21
  • 35