Skip to content

Instantly share code, notes, and snippets.

@mattdenner
Created July 5, 2011 09:56
Show Gist options
  • Select an option

  • Save mattdenner/1064583 to your computer and use it in GitHub Desktop.

Select an option

Save mattdenner/1064583 to your computer and use it in GitHub Desktop.
ActiveRecord is missing a middle ground between 'save' and 'save!'

I recently read Exceptional Ruby written by Avdi Grimm and, whilst I was generally impressed with it (it's worth a read, it made me think about things I do) there's one bit that I still struggle to approve of: using ActiveRecord::Base#save rather than ActiveRecord::Base#save!. It's not that I disagree with Avdi's sentiments, that exceptions should only be used in exceptional circumstances, but that ActiveRecord doesn't provide the middle ground that I think is needed.

My problem with save is that it relies on the developer calling it to check the return result which, from bitter experience, doesn't always happen.

What I want to see is something like this:

my_record.save do
  # Something went wrong, clean up!
  return 
end
# Successful save, continue as normal

I'm suggesting that the definition of save could be modified like this (although I don't agree with alias_method_chain but that's a whole other story!):

class ActiveRecord::Base
  def save_with_error_callback(&block)
    yield unless save_without_error_callback
  end
  alias_method_chain(:save, :error_callback)
end

If the developer doesn't pass the block then the yield will raise an exception, otherwise the error handling block will be called.

I might be completely mad with this but, until there's a way to force checking of the return result from save, I'll always advocate save! in preference. What do people think?

@cbrunnkvist
Copy link

Can't you check whether your code checker checks for unchecked #save calls? :)

@mattdenner
Copy link
Author

Because of the dynamic nature of Ruby, and the very fact that I've just shown how to modify the behaviour of a method, the best thing a code checker could do is warn that the "return value" isn't being checked and, from bitter experience, people ignore that kind of stuff. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment