save bang your head, active record will drive you mad

Posted by jcarroll

Sep 25

So in my never-ending quest to remove conditional logic from code, I began writing my Rails actions like:

1
2
3
4
5
6
7
8

  def create
    @user = User.new params[:user]
    @user.save!
    redirect_to user_path(@user)
  rescue ActiveRecord::RecordNotSaved
    flash[:notice] = 'Unable to create user'
    render :action => :new
  end

Instead of the traditional way using conditional logic like:

1
2
3
4
5
6
7
8
9

  def create
    @user = User.new params[:user]
    if @user.save
      redirect_to user_path(@user)
    else
      flash[:notice] = 'Unable to create user'
      render :action => :new
    end
  end

I did this until one of my co-workers saw this and said:

exceptions should not be expected

What?

When writing an application you expect invalid input from users. Since we expect invalid input we should NOT be handling it via exceptions because exceptions should only be used for unexpected situations.

Well what’s an unexpected situation?

Now I’m not going to write error handling for those unexpected circumstances because I don’t expect them to happen. If they do then I want the user to see an error page (500) and the developers to be notified via email about the exception (using exception notifier in Rails) (if i cared then yes I’d have to write error handling code but trying to recover from situations such as an out of memory is not going to be easy, if at all possible, so I want my application to fail in such a rare situation).

Let’s look at an example of reading from a file from the pickaxe.

1
2
3
4
5

  File.open('testfile') do |file|
    while line = file.gets
      puts line
    end
  end
Now IO#gets returns a String or nil. That is correct because reaching the end of the file is NOT an unexpected situation and should be handled with conditional logic.

What if ruby raised some EOFError instead:

1
2
3
4
5
6
7

  File.open('testfile') do |file|
    while line = file.gets
      puts line
    end
  rescue EOFError
    return
  end

Raising an EOFError would mean reaching the end of the file would be unexpected. That’s crazy talk.

I think ActiveRecord::Base#save! and ActiveRecord::Base.update_attributes! should be pulled from the public API. All they do is save the object and raise an exception if any validation fails. I doubt there are any web applications out there that do NOT expect invalid input. So there is no point in using #save! and #update_attributes! over their true/false equivalents #save and #update_attributes.

However, there is one case where they both have to be used. In transactions. Apparently the only way to roll back a transaction is to raise an exception. I say there should be some #rollback method so you could do something like:

1
2
3
4

 User.transaction do
   user.save || rollback
   another_user.save || rollback
 end

Following this convention we could also return the ’!’ back to its original meaning of ‘modifying the receiver’. Rails, with its #save! and #update_attributes!, has pushed the ’!’ to mean ‘something dangerous’ in order to justify the exception raising of methods like #save! and #update_attributes!.

Another Ruby idiom that is an abuse of exceptions is something along the lines of:

1
2
3

  user.address.street rescue ''
  # or
  user.address.street rescue nil

Code like this says that an address is optional for a user. Since a user without an address is not an exceptional situation it should be handled with conditional logic instead of exceptions.

1
2
3

  if ! user.address.nil?
    user.address.street
  end

I can’t stand that ‘rescue nil’ hack. Quit being lazy and write the ‘if’ statement.

Comments on this post

Ryan Allen

Sep 26

Ryan Allen said,

Instead of saying if user.save I do the following:

if user.valid?
  user.save!
  # do redirect junk
else
  # do error junk
end

So, an extra line of code… but I think it reads better.

Todd

Sep 26

Todd said,

I generally agree that exception handling for expected cases is not a good idea. I decided to do some benchmarking and it looks like for happy path the inline rescue nil is faster… My tests were really simple… Here’s my results: user system total real Baseline Happy Path Rescue 0.440000 0.010000 0.450000 ( 0.682402) Baseline Happy Path If/Else 0.620000 0.010000 0.630000 ( 0.886856) Not Happy Path Rescue 2.150000 0.150000 2.300000 ( 3.013638) Not Happy Path If/Else 0.240000 0.000000 0.240000 ( 0.394679)

And here’s my test code: require ‘ostruct’ require ‘benchmark’

user = OpenStruct.new({:address => OpenStruct.new({:street => “N st NW”})}) user.address user.address.street user2 = OpenStruct.new({:address => nil })

n = 50000 Benchmark.bm do |x| x.report(“Baseline Happy Path Rescue”) do n.times do user.address.street rescue nil end end x.report(“Baseline Happy Path If/Else”) do n.times do if user.address user.address.street end end end x.report(“Not Happy Path Rescue”) do n.times do user2.address.street rescue nil end end x.report(“Not Happy Path If/Else”) do n.times do if user2.address user2.address.street end end end end

Todd

Sep 26

Todd said,

Sorry my code didn’t paste as I expected… here it is again with the correct filter

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36

require 'ostruct'
require 'benchmark'

user = OpenStruct.new({:address => OpenStruct.new({:street => "N st NW"})})
user.address
user.address.street
user2 = OpenStruct.new({:address => nil })


n = 50000
Benchmark.bm do |x|
  x.report("Baseline Happy Path Rescue") do
    n.times do
      user.address.street rescue nil
    end
  end
  x.report("Baseline Happy Path If/Else") do
    n.times do
      if user.address
        user.address.street
      end
    end
  end
  x.report("Not Happy Path Rescue") do
    n.times do
      user2.address.street rescue nil
    end
  end
  x.report("Not Happy Path If/Else") do
    n.times do
      if user2.address
        user2.address.street
      end
    end
  end
end
Todd

Sep 26

Todd said,

One more try this time with the numbers easy to read:

1
2
3
4
5

      user     system      total        real
Baseline Happy Path Rescue  0.440000   0.010000   0.450000 (  0.682402)
Baseline Happy Path If/Else  0.620000   0.010000   0.630000 (  0.886856)
Not Happy Path Rescue  2.150000   0.150000   2.300000 (  3.013638)
Not Happy Path If/Else  0.240000   0.000000   0.240000 (  0.394679)
sandofsky

Sep 26

sandofsky said,

Exceptions are worse than goto statements, but there are a couple acceptable when validating.

One would be if you’re saving a record you know should be valid.

1
2
3

user = User.find(params[:id])
user.karma += 1
raise ActiveRecord::RecordNotSaved, "Could not save a presumably valid user" unless user.save

That’s a lot clearer than using “save!”.

Another time I throw exceptions is when a user is doing something malicious. I treat SecurityError as an emergency flare to get the hell out of there.

A simplified example:



raise SecurityError, "Javascript detected" if params[:comment] =~ /<script/
Alexandru Nedelcu

Sep 26

Alexandru Nedelcu said,

I do not agree with your statement.

Exceptions where invented to separate error handling from logic.

Your examples are too simple to illustrate the need for such separation, but when your block of code has a complex logic, manually handling errors with if conditions obfuscates the purpose of the code.

I prefer code clarity over anything else, and the first example you gave is a lot clearer for me.

The real problem is … what to exceptions to check and what to ignore ? I think Java answers this with its checked-exceptions, but by abusing the meaning of a checked-exception done by lots of APIs … it turned into feature bloat.

Tammer Saleh

Sep 26

Tammer Saleh said,

JareCare: This is very well put, and I agree that there’s really no place for #save! and the like if you’re following this rule.



@user.save or transaction.rollback!

Is much more explicit, without being much more verbose.

Joe Grossberg

Sep 26

Joe Grossberg said,

Principle #34 of The Pragmatic Programmer: “Use Exceptions for Exceptional Problems”

Then again the Pythonistas say “It’s easier to ask forgiveness than permission”.

P.S. I think you mean this: ;)

unless user.address.nil? user.address.street end

P.P.S. See you guys on Friday.

Joe Grossberg

Sep 26

Joe Grossberg said,

Other alternatives:


user.address.street unless user.address.nil?


user.address.nil? user.address.street : nil
Joe Grossberg

Sep 26

Joe Grossberg said,

Other alternatives:


user.address.street unless user.address.nil?


user.address.nil? ? user.address.street : nil
Rob Sanheim

Sep 26

Rob Sanheim said,

Rails did not “change the meaning” of the bang to mean something dangerous. Bang methods have always meant that in Ruby, see Kernel#exit! for instance. Matz has even commented on the fact that the bang means ‘danger’, and not necessarily modifies its receiver (though thats often the case).

See also David A Black’s post on this.

Tammer Saleh

Sep 26

Tammer Saleh said,

Joe: Or even

user.address.street if user.address

or

user.address and user.address.street

Jon Yurek

Sep 26

Jon Yurek said,

Exceptions where invented to separate error handling from logic.

Quite so! But invalid user input isn’t actually an error. Neither is an EOF. Both are (or at least should be) part of the normal flow of a program. Exceptions are for errors— the times when your program throws up its hands in frustration. It’s a normal thing for your users to type their password confirmation wrong, not a time for panic.

Dan Croak

Sep 26

Dan Croak said,

Rob’s right. Ruby is consistent with “the bang method means something dangerous/unexpected/atypical will happen.”

For instance, you expect many methods on Array and String to return a new Array or String. However, on Array#replace, you expect the array to be modified so there is no bang method.

Joe Grossberg

Sep 26

Joe Grossberg said,

Tammer:

user.address and user.address.street

Uh, forget that. Gives me flashbacks of ugly Zope/ZPT hacks.

Lee O'Mara

Sep 26

Lee O'Mara said,

ActiveRecord is not used exclusively for web application. As such, its API should not be limited by what is common practice in that domain.

Also, you may be interested in sane_transactions which offers an alternate take to the default AR one.

Barry Hess

Sep 27

Barry Hess said,

Several of us in ruby.mn were trying to solve the train wreck problem, but we never did come up with a consensus.

You describe not using this:

user.address.street rescue nil

but rather:

if ! user.address.nil?
  user.address.street
end

That gets to be a big annoying mess when you’re trying to hit, say, user.city.state.name. Just for legibility I’d rather do a rescue nil than:

if !user.city.nil? && !user.city.state.nil?
  user.city.state.name
end

My dislike of that conditional above (and it’s proposed single-line derivatives) is a lot less about lazy than it is about aesthetics. After all, this code is almost exclusively in the view.

We came to one idea that enhanced the Nil and ActiveRecord::Base method_missing definitions to allow a notation like:

user.city._state._name

The leading ’_’ character would result in a nil-safe series of calls, returning nil if anything in the series was nil. Unfortunately the solution seemed a little twitchy, probably because of Ruby’s existing “underscore” methods.

Another member of the group created a safe-getter class that dealt with the issue.

I’d love to see some more alternatives to solving the trainwreck problem. Until then, rescue nil reads better than anything else I can come up with.

crayz

Sep 27

crayz said,

Unless you’re actually planning to run through a save!/rescue loop a few thousand times in your create method, I wouldn’t worry about performance considerations

Jon Yurek

Sep 27

Jon Yurek said,

As such, its API should not be limited by what is common practice in that domain.

There is no problem domain where using #save! is a good method. Exceptions should be used as error control, not normal flow control.

Sorry, comments are closed for this article.

© 2000 - 2009 by thoughtbot, inc.
written by a bushel of tiny robots