features and bugs

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.

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.

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:

classEmployee<ActiveRecord::Basedefpay6.00endend

Ok now lets add a salaried employee type.

Iteration 2:

classEmployee<ActiveRecord::BasedefpayraiseNoMethodError,'Must be implemented by subclasses to return employee pay'endendclassHourlyEmployee<Employeedefpay6.00endendclassSalariedEmployee<Employeedefpay50000.00endend

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:

classCommissionedEmployee<Employeedefpay0.0endend

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?

Want to level up your testing game?
Learn about testing Rails applications and TDD
in our new book
Testing Rails.
The book covers each type of test in depth,
intermediate testing concepts,
and anti-patterns that trip up even intermediate developers.