16

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.

  • 4
    This seems like a textbook case for polymorphism. – wchargin Mar 8 '15 at 23:53
  • This is very intersting topic. Is there a chance you add the following solution to this problem? I would seggust that one put a calculatePay fucntion in each employee class but that would be a bad becouse now each employee class can be changed becouse of :1. The payment calculations. 2. adding more properties to the class etc.. – Stav Alfi Sep 7 '16 at 23:32
12

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.

  • 1
    I tried to watch the linked youtube video, but after 10 minutes into it with no mention of grouping by use-case actors, I gave up. The first 6 minutes are all rambling about entropy, for no apparent reason. If you gave a location in the video where he begins to discuss this, it would be helpful. – Michael Shaw Mar 8 '15 at 22:21
  • @MichaelShaw Try watching from 10:40 on. Uncle Bob mentions that code will "change for different reasons, because of different people". I think that might be what MichelHenrich is trying to point us to. – Enrique Mar 8 '15 at 22:49
  • Finished watching the entire 50 minute youtube video, the majority of which was not about what it was supposed to clarify. I noticed at the 16:00 mark that he'd already moved on from that topic, and he never returned to it. The "explanation" essentially boils down to this: "responsibility" in SRP doesn't mean that, it means "different reasons for change", which really means "changes at the request of different people", which really means "changes at the request of different roles that people play". The "explanation" doesn't clarify anything, it replaces one vague word with another. – Michael Shaw Mar 9 '15 at 1:11
  • 2
    @MichaelShaw like in the quote from the book, if you need to introduce different employee types, you have to change the Employee class. Different roles may be responsible for the payment of different employee types, so in this case, the Employee class has to be changed for more than one role, hence the violation of SRP. – MichelHenrich Mar 9 '15 at 22:31
  • 1
    @MichaelShaw yes, you're right - SRP does depend on how the organization is organized. That is exactly why I add "may" or "might" to all my comments :). However, even in those cases, while the code may not violate SRP, it surely does violate OCP. – MichelHenrich Mar 9 '15 at 22:45
2

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.

2

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.

-2

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
    Not sure how you can list 2 things the function does, but say it's not a SRP violation. – JeffO Mar 9 '15 at 1:29
  • @JeffO: That's not 2 things, that's 2 parts of one thing: calling the appropriate function based on the type. – Michael Shaw Mar 9 '15 at 1:56

protected by gnat Jan 23 '16 at 14:00

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.