---
title: features and bugs
teaser:
tags: web,rails
author: Jared Carroll
published_on: 2007-06-01
---

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.

```ruby
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.

```ruby
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.

```ruby
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:

```ruby
class Employee < ActiveRecord::Base

  def pay
    6.00
  end

end
```

Ok now lets add a salaried employee type.

Iteration 2:

```ruby
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:

```ruby
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?