GIANT ROBOTS SMASHING INTO OTHER GIANT ROBOTS

Written by thoughtbot

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.

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?