While attending RailsConf a few weeks ago the first non-keynote session I saw was my favorite and was also the reason I decided to go to the conference in the first place. It was the legendary Uncle Bob Martin; if you ever get a chance to attend one of his sessions definitely check it out, very entertaining and someone that would be awesome to work with. His talk wasn’t about Rails, it was about clean code. In it he took an example of some ugly code and refactored it. The refactoring was a demonstration of ‘replace conditional logic with polymorphism’, one of my favorite refactorings because of my hatred towards conditional logic. What you end up with is less ‘if’, ‘elsif’ and ‘else’ statements but more classes. When to add classes is a common debate here at t-bot, one that usually ends up bashing java and quoting some other java haters. I got no problems with classes and will add them to clean up code i.e. i’m fine with all java’s classes and have no trouble following heavy OO code.
Here’s an example:
Lets start with the classic boring employee example with pay depending on employee type.
At first we have only 1 type of employee: hourly.
class Employee < ActiveRecord::Base
def pay
6.00
end
end
And its table:
employees (id, name)
Now we need a salaried employee type, we’ll say they all get 50K. However, now
that we have more than 1 type of employee we need to introduce an employee type
concept. Ok, some simple constants will do for now; there’s no need for an
EmployeeType model.
class Employee < ActiveRecord::Base
HOURLY = 1
def pay
if type == HOURLY
6.00
else
50000.00
end
end
end
And its table:
employees (id, name, type)
Alright now we need a commissioned employee type, we’ll say they get $0. Looks like we’ll need another employee type constant.
class Employee < ActiveRecord::Base
HOURLY, SALARY = 1, 2
def pay
if type == HOURLY
6.00
elsif type == SALARY
50000.00
else
0.0
end
end
end
And its table remains the same:
employees (id, name, type)
Now Uncle Bob referenced his Open-Closed Principle during his talk at RailsConf:
software entities (classes,modules,functions,etc.) should be open for extension, but closed for modification.
In other words, when I add a feature I shouldn’t have to touch any existing code I should only have to add new code. Makes sense. When adding a feature you should only add code and not have to change any existing code. Changing existing code is what you do when you fix bugs.
We violated the Open-Closed Principle when we added our salaried and commissioned employee types. We kept going back and modifying the existing Employee#pay method. This example was simple but we could of introduced a bug into perfectly working existing code. Our new features also resulted in conditional logic making the code harder to read, more complex and a lot uglier than our first iteration that just had 1 employee type.
Let’s step through this again following the Open-Closed Principle.
Iteration 1:
class Employee < ActiveRecord::Base
def pay
6.00
end
end
Ok now lets add a salaried employee type.
Iteration 2:
class Employee < ActiveRecord::Base
def pay
raise NoMethodError,
'Must be implemented by subclasses to return employee pay'
end
end
class HourlyEmployee < Employee
def pay
6.00
end
end
class SalariedEmployee < Employee
def pay
50000.00
end
end
Here we turned Employee into an abstract superclass with 2 subclasses
HourlyEmployee and SalariedEmployee. Both involved not touching any
existing code.
Let’s add the commissioned employee type.
Iteration 3:
class CommissionedEmployee < Employee
def pay
0.0
end
end
Once again we touched no existing code. We added a feature by adding code. Since we didnt touch any existing code, we know we have no chance of breaking any existing features.
Now our first design had conditional logic and 1 class. The second design had no conditional logic and 4 classes. Like everything else in software there are trade offs when writing clean code. Which design would you prefer?