krn krn - 4 months ago 10
Ruby Question

Ruby flow control: throw an exception, return nil or let it fail?

I am thinking of the best practices of flow control. Which way should I go?

1) Don't check anything and let the program fail (cleaner code, natural error messages):

def self.fetch(feed_id)
feed = Feed.find(feed_id)
feed.fetch
end


2) Fail silently by returning nil (however, "Clean Code" says, that you should never return null):

def self.fetch(feed_id)
return unless feed_id
feed = Feed.find(feed_id)
return unless feed
feed.fetch
end


3) Throw exceptions (because it's exceptional not to find a feed by id):

def self.fetch(feed_id)
raise ArgumentError.new unless feed_id
feed = Feed.find(feed_id)
raise ArgumentError.new unless feed
feed.fetch
end


To put it in other words: should I be using guard conditions actively, or is it better to rely on Ruby / Rails methods and let them throw an exception, if something wrong happens?

Answer

1) Don't check anything and let the program fail (cleaner code, natural error messages):

It's ok to "let the program fail" with known, documented exceptions, but getting an unpleasant NoMethodError because you tried to use a nil object is just careless. In your particular example, ActiveRecord#find raises a documented ActiveRecord::RecordNotFound exception, so IMO this is the way to go:

def self.fetch(feed_id)
  Feed.find(feed_id).fetch
end

2) Fail silently by returning nil (however, "Clean Code" says, that you should never return null):

That's fine as a general advice, but Ruby is crammed with methods that return nil; and that's ok (again, as long as it's documented), it just means "Nothing" (and allows the very compact pattern something_that_can_be_nil || another_value). In this case I'd write it concisely using Ick's maybe:

def self.fetch(feed_id)
  Feed.find_by_id(feed_id).maybe.fetch
end

3) Throw exceptions (because it's exceptional not to find a feed by id):

Yes, but then let the method raise the well-known RecordNotFound exception, not a custom one (unless you want to abstract the fact that you are working with AR, which can be very cumbersome).

Comments