David R. MacIver

So, it’s well known that you shouldn’t have code that looks like this (examples in ruby and ruby-like pseudo code, but they’re trivially translatable to any other language):

begin
   do stuff
rescue
end

i.e. you’re swallowing the exceptions because they scared you and you wanted to hide them. This is naughty.

A more subtle trap people fall into is the following example from the rails boot.rb (this isn’t a rails specific problem. I see it everywhere)

    def load_rails_gem
      if version = self.class.gem_version
        gem 'rails', version
      else
        gem 'rails'
      end
    rescue Gem::LoadError => load_error
      $stderr.puts %(Missing the Rails #{version} gem. Please `gem install -v=#{version} rails`, update your RAILS_GEM_VERSION setting in config/environment.rb for the Rails version you do have installed, or comment out RAILS_GEM_VERSION
      exit 1
    end

Let’s consider the results of this rescue block: A generic error message is printed, and we exit with a non-zero status code.

Now let’s consider the results of not having this rescue block: A specific error message is printed, we exit with a non-zero status code and we get a stack trace telling us exactly what went wrong.

So by including this rescue, we have lost information. Often this information doesn’t matter, but as it turns out in this case it does: If you have a gem version clash where rails depends on a different version of a gem that has already been loaded you will get a Gem::LoadError and, consequently, a very misleading error message.

I don’t want to pick on rails. Well, correction. I don’t want to pick on rails here. This post is actually inspired by a similar incident at work: There was a piece of code that basically looked like this:

begin
  connect to server
rescue
  STDERR.puts "Could not connect to server"
  exit
end

And were getting very puzzling errors where we were sure all the details were correct but it was failing to connect to the server. Once we deleted the rescue code it was immediately obvious why it was failing (if you care, the reason was that we weren’t loading the config correctly so it was trying to connect with some incorrect default values).

Which brings me to the point of this article: The best way to handle exceptions is not to handle them. If it’s not an exception you can reasonably recover from, the chances are pretty good that the default behaviour is more informative than the “helpful” code you were going to write in order to catch and log the error. So by not writing it you get to have less code and spend less time debugging it when it inevitably goes wrong.

This contains overtones of a post I’ve been resisting writing for months in the knowledge that the people who already agree with it will find it obvious while the people who don’t will bitch and moan and tell me that I’m stupid and perpetuate the same tired contentless arguments they always use.

So, having explained why this post is useless, I now add the caveat that I am writing it anyway, for the very simple reason that I am so angry at being proven right on the subject time and time again, and in particular at having had an entire morning wasted by this fuckwittery, that I am overriding my internal censor.

DataMapper is a ruby ORM library. It is inherently broken. Why? Because it considers booleans to be a superior solution to exceptions. This means that there is essentially no way to save data without falling into subtle and pernicious traps.

In DataMapper when you save an object, it returns false if it fails to save. You then need to ask it why it failed to save if, for some bizarre reason, you feel the need to care about little things like your data not making it into the database.

So, the code you end up writing all over the place looks like the following:

   my_account = Account.new(:name => "Jose")
   if my_account.save
     # my_account is valid and has been saved
   else
     my_account.errors.each do |e|
       puts e
     end
   end

Before anyone claims this is a contrived and silly example, here’s the source of it.

What’s the problem here?

Well… earlier I was called to help some colleagues. They were seeing really weird behaviour where save was failing but errors was empty. I was sure this must be a bug somewhere – it shouldn’t be possible. We even checked before the safe and valid? was returning true.

Which reminds me, another and sometimes recommended way of writing the above is to instead do…

   my_account = Account.new(:name => "Jose")
   if my_account.valid?
     my_account.save
     # my_account is valid and has been saved
   else
     my_account.errors.each do |e|
       puts e
     end
   end

Warning: If you do this, you are screwed. Your code is broken, even if it happens to work right now, and you will be bitten by it. Change it.

So, why is this code wrong?

Because the errors do not necessarily live on the object you are trying to save.

Suppose I instead had:

my_account = Account.new(:customer => Customer.new(:name => "jose"))

and tried to save this.

Well, it might fail to save, but return no errors.

Why? Because, it first has to save the customer. And that might fail to save. And if it does you won’t see any errors on the object you’re saving and have to hunt around to figure out what’s going wrong.

So, as well as having to explicitly check for errors on everything you attempt to save in order for your code to be correct, you must also explicitly check for errors on anything IT might helpfully decide to save for you.

This is untenable.

Code which fails should fail noisily. I shouldn’t have to work to find out why it went wrong, it should TELL me. If DataMapper did the obvious thing and threw an exception when something failed to validate then it would cut out a great deal of work and debugging effort, it would mean that I could simply handle the creation of a bunch of data and deal with the errors in one place instead of having to check everything for failures. It would mean that one could write code without living in a constant state of paranoia that you might make a mistake and forget to check a return code somewhere and then waste hours debugging when it finally bites you. And it would mean that not all the examples in their documentation would be wrong.