Reputation: 283
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
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
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