CODEWITHSUNDEEP
oopdesign-patternscoding-stylesingle-responsibility-principle
Mostafa Darwish
Mostafa Darwish

Reputation: 283

understanding single responsibility principal SRP in clean code example

currently i'm reading the clean code Book of Uncle Bob,in the function section when looking to the following example:-

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

Uncle Bob said:-

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

he states a solution as following:-

public abstract class Employee {
    public abstract boolean isPayday();
    public abstract Money calculatePay();
    public abstract void deliverPay(Money pay);
}
-- -- -- -- -- -- -- -- -
public interface EmployeeFactory {
    public Employee makeEmployee(EmployeeRecord r) throws InvalidEmployeeType;
}
-- -- -- -- -- -- -- -- -
public class EmployeeFactoryImpl implements EmployeeFactory {
    public Employee makeEmployee(EmployeeRecord r) throws InvalidEmployeeType {
        switch (r.type) {
            case COMMISSIONED:
                return new CommissionedEmployee(r);
            case HOURLY:
                return new HourlyEmployee(r);
            case SALARIED:
                return new SalariedEmploye(r);
            default:
                throw new InvalidEmployeeType(r.type);
        }
    }
}

I can not fully understand the idea from the example and there is some question in my head i can not find answers to :-

1- in first code when new employee is added it will grow. true but also this occurs in the solution so what is the difference ?

2- how the first example does more than one thing . it only calculate payment "functions at the same level of abstraction" notice that if we consider throw error is another thing to do , the solution does it too

Upvotes: 3

Views: 1010

Answers (2)

Floegipoky
Floegipoky

Reputation: 3273

First, it’s large, and when new employee types are added, it will grow.

You're right, the solution doesn't really make the overall code size shorter and when a new employee type is added, it will still grow overall.

Second, it very clearly does more than one thing.

The original both handles dispatching to the correct payment calculation function and calculates the payment. The proposed solution addresses this. HourlyEmployee.calculatePay() now calculates pay only for an HourlyEmployee, etc. EmployeeFactoryImpl handles the dispatch based on the Employee implementation it returns.

Third, it violates the Single Responsibility Principle (SRP) because there is more than one reason for it to change.

The original calculatePay needs to change if the pay calculation logic needs to change. It also needs to change if a new employee type is added. The solution does not require a change to calculatePay when a new employee type is added. Thus there is only a single responsibility and a single reason to change.

Fourth, it violates the Open Closed Principle (OCP) because it must change whenever new types are added

Going back to 1, the overall code length will still change when a new employee type is added. However, only the part related to dealing with employee types has to change. The code devoted to calculating pay doesn't need to change at all. Thus the part that needs to be extended is open, and the part that is unrelated to the extension is closed.

Upvotes: 3

David Osborne
David Osborne

Reputation: 6821

1) More code is harder to understand, debug and maintain. Sure, the example isn't that big, but you get the point. Keeping classes small and to the point aids maintainability, extensibility and re-usability. What's more, if you change the code, adding a new section for the new employee type, you potentially break the old code.

2) The logic in each calculation is going to be different. Therefore, the class is doing three different things. If you split each calculation into its own class, you get the benefits mentioned in 1 plus the solution is much more testable.

Upvotes: 0

Related Questions