The Chris O Show

Last week I had the good fortune of having my colleague Abdel Saleh go through some rspec specs I had written.

“Why do you need this comment?” he asked for a bit of code like this:

before(:all) do 
  # Ensure doesn't actually save to Mogile FS
  MogileFS::MogileFS.any_instance.stubs(:store_content)
end

..and he promptly deleted it..

“No wait!” I said defensively, “That’s there to prevent the tests from actually hitting mogilefs. The comment’s there because the line of code doesn’t describe the intent”

“Well, that’s easily fixed” says Abdel, “Rather than using a comment to describe the intent, put it in a method with a name that describes the intent.”

before(:all) do 
  ensure_doesnt_save_to_mogile_fs
end

def ensure_doesnt_save_to_mogile_fs
  MogileFS::MogileFS.any_instance.stubs(:store_content)
end

Why is this better!??

Comments aren’t maintainable. They don’t grow and change with the codebase. They lay still and live on like strange messages from ancient civilisations.

In the preceding example you no longer NEED a comment to describe the intent of your code, your code does it for you!

Ever find yourself wrapping a method with a conditional:

  def save_to_file(filename)
    unless filename.blank?
      do_something
      do_something_else
    end
  end

Or maybe you subscribe to the one exit from a method strategy.

I write code that looks like this a lot. And you know what, I didn’t think it had a code smell until my colleague Abdel looked over it the other day.

“Why don’t you ever use guard clauses?” he asked.

A guard clause is a conditional statement at the top of a function that bails out as soon as it can. Like this:

def save_to_file(filename)
  return false if filename.blank? #<- Guard clause

  do_something
  do_something_else
  File.write...# blah blah
end

I didn’t have a good reason for not using Guard Clauses, except an idea in the back of my head that there should only be one exit from a method. There’s a bunch of stuff online that promotes this way of writing methods, but I’ve discovered recently that most of the reasons to support one exit are baloney.

Functions with single exit points are handy for languages that require decent memory management like C, but in Ruby where you have a garbage collector this doesn’t make any sense.

Also, if you’re writing uber long functions, then it makes sense to have a single exit point – but if you’re writing uber long functions then you’re a terrible programmer.

Anyway, so Abdel refactored my code by adding in some tasty guard clauses, and I’ve become a die hard fan.

PS: Of course, with all refactoring it’s about the readability and maintainability of your code. Don’t blindly adopt guard clauses if it makes your code harder to read.

PPS: Read more about guard clauses at refactoring.com