…aka experiments in refactoring

A couple of weeks ago I attended a session at the SPA 2014 Conference run by Sandi Metz (@sandimetz) and Matt Wynne (@mattwynne). I was looking to maintain my balance of coding sessions versus discussion or activity-based workshops so it was a toss-up between two that morning. I must admit that the session title of Replacing Conditionals with Polymorphism didn’t immediately catch my eye although I rate both the presenters. However, as the session on Advanced Testing Techniques for Node.js seemed to require more Node than I know (i.e. more than zero) I decided to give it a go.

When I read the session description, I remembered why I had rated this session proposal when I was helping the conference programme chairs with the proposal review earlier in the year. It involved running an interesting experiment in refactoring style and its effect on a codebase. The basic premise was that by following a simple set of rules you could refactor some simple but gnarly code littered with conditionals in a way that made it very easy to extract objects. This in itself would have been interesting but fairly unspectacular. However, the side effects have some interesting implications.

The Experiment

The session attendees split into two groups: an experimental group and a control group. There was some slicing and dicing based on Ruby experience so that neither group had more Ruby experience than the other. After this, each group split into pairs to perform the refactoring exercise. I found myself in the experimental group pairing with Steve Tooke (@tooky) which I enjoyed a lot.

The rules and guidance for the experimental group were something like:

• Find similar bits of code in the conditionals and make them the same by extracting methods and moving the conditional itself into these methods (i.e. the method has an ‘if’ in it but the original code just has the method call).
• While extracting this commonality, leave in duplication (i.e. don’t try to refactor out common bits between these new methods). This was highlighted as going against the principles of DRY.
• Turn this set of extracted methods into a new class. The methods still have the conditionals in them.
• Create subclasses to remove the conditionals in the superclass methods.
• While doing the refactoring, run the tests after every change and always remain one undo from green. If your tests turn red, undo your change and make a different one.
• Fix bad names as you go.

The point about being one undo from red was that these were to be done as small, mechanical steps.

The control group were essentially given the unit tests and a blank canvas on which to implement their solution.

The Code

The example code was a generator of song lyrics based on the song “99 Bottles of Beer”. You can find the code in Sandi’s GitHub.

The control group were group A so you can see thier instructions in the group A readme

The experimental group were group B so you can see more information on the rules in the group B readme.

The initial code for group B looked something like this:

class Bottles

  def song
    verses(99, 0)
  end

  def verses(upper_bound, lower_bound)
    upper_bound.downto(lower_bound).map { |i| verse(i) }.join("\n")
  end

  def verse(number)
    case number
      when 0
        "No more bottles of beer on the wall, no more bottles of beer.\nGo to the store and buy some more, 99 bottles of beer on the wall.\n"
      when 1
        "1 bottle of beer on the wall, 1 bottle of beer.\nTake it down and pass it around, no more bottles of beer on the wall.\n"
      when 2
        "2 bottles of beer on the wall, 2 bottles of beer.\nTake one down and pass it around, 1 bottle of beer on the wall.\n"
      else
        "#{number} bottles of beer on the wall, #{number} bottles of beer.\nTake one down and pass it around, #{number - 1} bottles of beer on the wall.\n"
    end
  end

end

A set of unit tests were provided to run against the code during the initial refactoring steps.

Extracting Methods and Creating a Class

An example of the type of simple methods extracted is shown below (we got bonus points from Matt for good method naming):

def verse(number)
  ...
    ... Take #{pronoun} down and pass it around ...
  ...
end

def pronoun
  if number == 1
    'it'
  else
    'one'
  end
end

Following the experimental steps heads you towards code that looks like that below:

  def verse(verse_number)
    current_bn = BottleNumber.new(verse_number)
    next_bn    = BottleNumber.new(current_bn.next)

    "#{current_bn.quantity.capitalize} #{current_bn.container} of beer on the wall, " +
    "#{current_bn.quantity} #{current_bn.container} of beer.\n" +
    "#{current_bn.action}, " +
    "#{next_bn.quantity} #{next_bn.container} of beer on the wall.\n"
  end

Because the session was only a couple of hours, once it got to about the half way mark in the session, this code was provided to pairs in the experimental group so that they were starting the second part from the same point. We were not quite there with our implementation but we could see that in about another 20 minutes we would have been.

Subclassing

We then created subclasses to remove the conditionals by overriding methods…

class BottleNumber
  ...

  def pronoun
    'one'
  end

  ...
end

class BottleNumber1 < BottleNumber
  ...
 
  def pronoun
    'it'
  end
 
  ...
end

… and provided a mechanism to instantiate the correct subclass (with some reflection code nicked from the Internet).

class Bottles
  ...

  def verse(verse_number)
    current_bn = create_bottle_number(verse_number)
    next_bn    = create_bottle_number(current_bn.next)

    ...
  end

  def create_bottle_number(number)
    BottleNumber.for(number)
  end

  ...
end

class BottleNumber
  attr_reader :number
 
  def self.for(number)
    bottle_number = available_bottle_numbers.find { |subclass| subclass.handles?(number) }
    bottle_number.new(number)
  end

  ...

  private
  def self.handles?(number)
    true
  end
 
  def self.available_bottle_numbers
    available_bottle_numbers = subclasses
    available_bottle_numbers << self
  end
end

...

module Subclasses
  # return a list of the subclasses of a class
  def subclasses(direct = false)
    classes = []
    if direct
      ObjectSpace.each_object(Class) do |c|
        next unless c.superclass == self
        classes << c
      end
    else
      ObjectSpace.each_object(Class) do |c|
        next unless c.ancestors.include?(self) and (c != self)
        classes << c
      end
    end
    classes
  end
end
 
Object.send(:include, Subclasses)

The final task was to print “Six Pack” when you only had 6 bottles left. This was obviously fairly straight forward as you could add a BottleNumber6 class to provide all the right wording etc.

The Side Effects

Because of the roles I have occupied over the past 6-7 years, I am not a day-to-day coder. This means that I do not have the well honed instincts of some of the reflective practitioners I know who are working in code every day. Given a particular code smell, in most cases I would have to revert to flicking through Martin Fowler’s Refactoring book, digging out Joshua Kerievsky’s Refactoring to Patterns or Googling for some useful advice. Hence, for me, the interesting thing was that following the simple rules took me to the right place without me having to pre-plan. The next change was pretty obvious at all times and each refactoring was a simple step. This made the exercise very stress-free despite us coding against the clock.

All of the pairs lodged their code as gists. This shows up another interesting point about the exercise. Almost all the code for the experimental group ended up looking the same, whereas the code from the control group, although it implemented the same functionality, differed quite markedly. Now, this in and of itself is not too surprising given:

1. all the experimental pairs started from the same place and were following the same rules
2. there are many ways in code to skin the proverbial cat so the output of the control group would reflect their own experience, skills and preferences.

The argument being made was that any given implementation from the experimental group was understandable to all pairs in the experimental group and so could have been maintained by any of them if bug fixing or extension was needed. Similarly, if you could only get part way through a refactoring then this could be finished off by any pair in the experimental group. By extrapolation, if you applied similar rules to refactoring conditionals across your codebase then it would become more familiar to all and less subject to particular preferences. The importance of this is drawn out by examples such as being the person with the pager who has to go in and fix code having potentially consumed several of the bottles of beer.

It could be argued that some of the example solutions from the control group could be seen as more maintainable, such as the one from Duncan McGregor (@duncanmcg) and Seb Rose (@sebrose) which only occupied a single page. Conversely, in some ways it emphasised the point being made in that it was different from the way other control group pairs implemented theirs.

A final thought on this is that the code given to the experimental group to start with was probably “the simplest thing you could possibly do” in terms of an implementation. This means that the pair implementing it could have gotten to the stage of working code and passing tests very quickly but still ended up in a good place after their refactoring.

And finally

I thought that this was as thought-provoking session in the traditions of the SPA conference. The only complaint I would have is that, although it was promised in the session description, nobody sang while they were refactoring but maybe that’s for next time 😉