Blog

Friday, January 14, 2011

Never use rescue blindly

On a recent project I was horrified to see various inline rescues. Here are three examples:

# Example 1
def friend_recommended_picture
  friend.recommended_picture rescue ""
end

# Example 2
params[:search][:skip_conversion] = true rescue nil

# Example 3
item_image = Product.find_by_id(product_id).images.find(:first, :conditions => 'attachment_file_name like "%example.jpg"').attachment.url(:mini) rescue ''

In all of these examples I the author is protecting themselves from a NoMethodError with a NilClass.

My preference is to always use the try method (provided in Rails 2.x and standard in Ruby 1.9):

# Example 1
def friend_recommended_picture
  friend.try(:recommended_picture) # returns nil if friend is nil
end

In Rails, this can be cleaned up using the delegate class method:

delegate :recommended_picture, :to => :friend, :allow_nil => true, :prefix => true

For the second example I would prefer this, although there is a little more code involved (clarity over cleverness):

# Example 2
params[:search][:skip_conversion] = true if params[:search]

And for the final example, well this is a bit of a mess. Let's clean up the call:

item_image = item_image(Product.find(product_id))

And encapsulate the various scopings within the correct classes.

# products_helper.b
def item_image(product)
 product.example_image.try(:attachment) {|attachment| attachment.url(:mini) }
end

# models/post.rb
def example_image
  images.examples.first
end

# models/images.rb
named_scope :examples, :conditions => 'attachment_file_name like "%example.jpg"'

This last example illustrates well that the careless use of rescue nil is a symptom of other issues such as poor encapsulation.

There are plenty of examples of using rescue such as on Ruby Inside:

h = { :age => 10 }
h[:name].downcase                         # ERROR
h[:name].downcase rescue "No name"        # => "No name"

This still makes me feel dirty inside. Yes it is working on a Hash, but what if h becomes a more complex object, then that rescue could be obscuring other exceptions. My preference would be:

h[:name].try(:downcase) || "No name"

Jay Fields also has an interesting use for inline rescue. One of the commenters mentions "its important to use them judiciously." I suspect the inline rescue is a victim of cargo culting and 99% of the time there is a better solution.

Please note this blog is no longer maintained. Please visit CivilCode Inc - Custom Software Development.