gears
within
gears

At ThoughtWorks, everyone submits a code sample as part of their interview process. The comment I received from my interviewer about mine struck me, though, and it sticks with me to this day. “The code looks fine,” he said, “but what’s with all the comments?”

My introductory CS course at Northeastern University taught me a lot of things that have resonated in my experience in the industry, but we were taught one thing that, as it turns out, almost none of my colleagues agree with: that all of your methods, classes, and modules should be preceded by a simple, one-line comment that states the purpose of code unit in question. Far be it from me to gainsay Neal Ford, who believes that code comments are a smell, but there are days when I disagree: Even developers with the best intentions and training in the world can’t always write clear code.

A Brief Re-Introduction to Class Re-opening

The problem manifests itself most acutely through the Ruby mechanism of class re-opening (sometimes dismissively referred to as “monkey-patching”). You can go about it in a number of ways, and the codebase I’m working on these days has employed each and every one of them at some point or another. The advantage you gain from being able to reopen classes is that you can patch the framework you’re using unobtrusively in order to fix bugs or improve performance. The downside is that you don’t get to choose the names of the methods you’re overriding: they may make perfect sense in the context of the class you’re overriding, but none whatsoever as an isolated patch.

A reopened class looks something like this:

class String
  def to_widget
    Widget.new self.gsub(/foo/, "bar")
  end
end

String.class_eval do
  def to_widget
    Widget.new self.gsub(/foo/, "bar")
  end
end

module StringExtensions
  def to_widget
    Widget.new self.gsub(/foo/, "bar")
  end
end

String.send :include, StringExtensions
In the first example, we’ve patched String directly; in the second, class_eval’d it for an added layer of complexity; and finally we’ve defined our own module and added it to String after the fact, which is fact the recommended way of going about it. We can use IRB and the method method to try to figure out where our patch is coming from:

irb(main):003:0> "".method(:to_widget)
=> #<Method: String#to_widget>

irb(main):003:0> "".method(:to_widget)
=> #<Method: String#to_widget>

irb(main):003:0> "".method(:to_widget)
=> #<Method: String(StringExtensions)#to_widget>

Notice that only the last one gives us a helping hand when we’re trying to figure out where this method was defined.

Overriding, not extending

Now imagine there’s a bug in the String class: a colossally poorly-designed method, something that’s significantly impacting your ability to get something done in the language. (While this doesn’t happen in Ruby very often, it’s like to have occurred at least once in the framework of your choice.) In the bad old days, you’d have had to patch Ruby directly; if you were good about it, you’d submit your patch to Matz, and it’d get integrated as part of some future release. But the language itself gives you an escape valve: override the method, and your implementation gets used.

On my current project, we do this all the time.

But what happens when the framework gets upgraded? Is this extension still necessary? Why did anyone put it there in the first place? What’s with this Subversion check-in comment? Why didn’t anyone say anything in the code?

Enter alias_method_chain

alias_method_chain is a Rails method patched into Ruby that allows you to extend a method without blowing away the old version. You can read Marcel’s post on it on the Rails blog from back in the dawn of time, two years ago, when they added it to the framework. In action, it looks something like this:

module BetterToInteger
  def self.included(string_class)
    string_class.alias_method_chain :to_i, :logging
  end

  def to_i_with_logging
    returning(to_i_without_logging) do |new_value|
      logger.log "Converting from #{self} to #{new_value}"
    end
  end
end

String.send :include, BetterToInteger

Suddenly our extended method has an identity again: it has a name that clearly indicates both the method it’s trying to extend and the feature we’re adding on top of it. Hallelujah!

Why does this matter?

The ability to re-open classes is an insanely powerful tool. I love it, not least because it never means having to write another XUtil (XmlUtil, DateUtil, StringUtil…) class again to make up for the deficiencies of someone else’s API. I could never tell you, in good conscience, not to use it.

But cleaning up becomes a bit of a mess when you have to upgrade frameworks (or language versions, for that matter) as we’ve had to do recently on our project. We carry a copy of Rails around in our vendor directory, like most projects do, but we’ve tried to be good about making our extensions externally, through class re-opening, rather than patching the Rails code directly as men and women did in days of yore. But because, out of habit, none of these extensions carry any comments, we’ve had a devil of a time trying to figure out not what they do (we can always read the code) but why they’re there, and whether we can finally, for the love of God, delete them now that we’re upgrading to Rails 2.2.

But take it from me, one developer to another, who hath had to maintain thine extension, yea, even unto multiple new Rails versions: using the tips above will help me, and those like me, figure out what the dickens you were trying to do. And for Heaven’s sake, when you don’t have the luxury of naming your methods to make it clear what they do or why they’ve changed, leave a quick comment.