Coding is like gardening...

Archive for the ‘refactoring’ Category

Under the hood: RSpec as your early warning system

This article is part of our ‘under the hood’ series, where we examine coding concepts and techniques related to how we build our products and services for our clients. If you’re of a technical bent, then do check out other articles as they appear in our ‘under the hood’ category.

One of the most useful aspects of RSpec and BDD is how it can highlight areas of your code that are obvious targets for improvement.

When we write our specs before our code we are effectively writing the API, the contract that our code must fulfill. It’s often the case that if this contract is hard to write, or just looks clumsy, then there may be a better way to express it.  This makes RSpec our first way of detecting code smells.

Perhaps the easiest type of code smell to detect with RSpec is Law of Demeter violations. Imagine a simple Ruby on Rails task management application. Tasks can be owned by Users. There may be many places where we would want to show who owns what Task. The simplest way to do this in Rails would be to use a belongs_to association to expose the attributes of the User to the Task. This makes it trivial to get the name of the user who is assigned to a task:

class Task < ActiveRecord::Base
  belongs_to :owner, :class_name => "User"
end

# to get the owner name:
task.owner.name

While this is easy to write, it would make our specs more complex. Anywhere we wished to mock the name of the owner we would have to write something like the following:

task.stub!(:owner).and_return(mock_model(User, :name => "Bob Smith"))

There’s a lot of noise in this line. We’re creating a mock object just to produce the name of the user. Typing and maintaining test code like the above just seems unnecessary — we’re mocking more than we need. Perhaps worse, it doesn’t express our intentions clearly. We’re asking the task to return its owner, then asking that owner to give us its name. Instead of this approach, we can define the contract as we go and we can actually express our intention:

task.stub!(:owner_name).and_return("Bob Smith")

This makes our tests look tidier, but it actually has a more beneficial side effect. Even though we now have to write more code (speccing then adding the owner_name method to the Task class), it’s now the Task’s job to find the name of its owner. This makes our code more maintainable. For instance, imagine that not every task has an owner. With our original code, anywhere we explicitly used the owner association we would have to change the code to:

(task.owner.nil? ? "No Owner" : task.owner.name)

If this was used many times throughout our code base, we’ve suddenly got a large number of places to find and test this code. By defining the owner_name method early we avoided this problem because there’s only one place that now has to deal with the messy business of finding the name of the owner. Our other specs and stories aren’t broken because they can still happily call the owner_name method without worrying about how this is actually handled.

By using RSpec to help us separate out what information we want from how we get this information, we’ve made our codebase just a little bit cleaner and more maintainable.

Personal victories

I love it when you can refactor a bulky, ugly function down into one line. It always makes me very happy indeed. Here is my solution for outputting a person’s name in a nice format, taking title, initials, first name, surname, suffix. Initials take precedence over first name.

This is how i did it originally:

  def contact_display_name
    returning String.new do |output|
      output < < title + ' ' unless title.blank?
      if initials
        output << initials + ' ' unless initials.blank?
      else
        output << first_name + ' ' unless first_name.blank?
      end
      output << surname + ' ' unless surname.blank?
      output << suffix unless suffix.blank?
      output.strip!
    end
  end

Rather too long-winded for my liking. I managed to refactor it down to this:

def contact_display_name
    [title, (initials? ? initials : first_name),
      surname, suffix].delete_if(&:blank?).join(' ')
  end

Of course, it is another advantage of behaviour-driven development, that i had already specced out what i wanted to happen, and i could use the specs to ensure that my two versions of the function were equivalent. See below for the specs that verify the behaviour.

(more…)