Thinking in Ruby
If you've been programming in another language for a long time before learning Ruby, you'll probably go through several stages of improvement before you feel totally comfortable with it. At first, you'll learn the keywords, you'll slowly catch on to the awesomeness of the block syntax, and you might even start using begin...rescue...end. And at that point you'll get to a plateau of sorts, where you can pretty much do what you need to do in Ruby without referring to ruby-doc.org too much.
But you're probably still writing your control structures like you did in your old language. You're still thinking the old way. You need to think in Ruby to get to that next stage of productivity.
Here's a simple example. Today, we were writing some Rails code when we realized "there must be a Ruby way to do this." Skipping the irrelevant details, here's what the original code looked like:
before_filter :init
def init
if params[:id]
@user = User.find(params[:id])
elsif params[:name]
@user = User.find_by_name(params[:name])
end
end
If this were C# code, I would have declared victory and moved on. But after you program with Ruby a while, you realize that this code has an aroma not unlike the well-known refactoring-needed code smell. But, simple refactoring here is not the whole story. First, we realized that we will always have an :id or a :name in the params hash. So I wanted to do this:
def init @user = User.find(params[:id]) || User.find_by_name(params[:name]) end
Awesome, right? One line of code, using the nifty || operator in Ruby. For those of you not familiar with it, the || only allows the right-hand side to be evaluated if the left-hand side evaluates to false (or nil).
But, this did not work, because ActiveRecord::find throws an exception if you pass a nil value into it. So the left-hand side will throw an exception before the right-hand side gets a chance to run. For a moment I was about to do this:
@user = User.find(params[:id]) if params[:id] @user ||= User.find_by_name(params[:name])
Now, this uses the even niftier ||= operator, which only evaluates the right-hand side if the left-hand side of the assignment (@user in this case) is nil. So if @user already has a value, the right-hand side won't even execute, which is what we want.
Then a small light went on, a soprano voice could be heard faintly in the distance, and I realized I could do this instead:
@user = User.find(params[:id]) rescue User.find_by_name(params[:name])
This is the very nifty inline rescue syntax (it probably has an official name, but that's what I call it). If the first part of the expression blows up, like the User.find() will if params[:id] is nil, then the exception is handled right then and there, and then the second part is then executed (the find_by_name part).
So we put the 80% case first (find by id), and if that fails, we find by name. One line, still very readable.
Using Rails is one thing; learning Ruby and really starting to "think" in Ruby can be another. This is one area we'll try to cover in our workshop, because it's really important for those of us switching to Rails from languages like VB or C#.
That's awesome! I've had to do something similar previously and never found a solution I really liked the look off — mainly because of the issue of AR raising an error as you point out.
Now… I'm sure there is some code I can refactor with this.
You could also swap out vanilla find for find_by_id which will return nil allowing your || version to work.
Some would argue, exceptions are meant for handling exceptional conditions (i.e., conditions you didn't anticipate), and not for typical functioning of the method.
Instead of utilizing rescue, what about doing this:
@user = params[:id] ? User.find(params[:id]) : User.find_by_name(params[:name])Greoff: my thoughts precisely. Te code should not throw exceptions as part of normal operations.
It's strange, in any other language the more code you write, the more lines in your file. Since moving to Ruby, I do the same thing that you mention, keep re-factoring code as you go and your code gets shorter!!? - I love Ruby ;)
I'm with Geoff and Gleb. Exceptions are not for flow control!
I actually (gasp) like the original code best. No, it's no one line. But it's concise and clear. It is also extensible. What happens if you want to add a third query parameter in the future? In the first version, you add two lines of trivial ruby code. You could technically extend your final version by chaining multiple rescue statements, or nesting the ? : operator (using Geoff's suggestion) - but that is clearly a bad idea.
Another reason I like the first option: it is more specific. It explicitly tests for the existence of params[:name], which none of the one-line solutions do. In this case that may not be a big deal. But I like to make the fewest possible assumptions about the input of my functions. What if you want to do something special if none of the params are present? You'd need to add a rescue or test for nil. Might as well throw it in a catch-all "else" clause at the end of your if..elsif chain, where it fits neatly.
What if User.find(params[:id]) throws an exception other than the :id = nil case?
I wouldn't use rescue for flow control either. I could be wrong, but I belive you'll also pay a performance penalty for using that rescue. I would stick with the original or a slighly modified version of your 1st refactoring.
If you just have to have a one liner I would go with the following, although you'll needlessly incur an extra database hit if there if params[:id] is nil:
@user = User.findby_id(params[:id]) || User.findby_name(params[:name])
"Premature optimization is the root of all evil." - Donald Knuth
this isn't "thinking in ruby", its just trying to needlessly eliminate LOC instead of thinking ahead for maintainability and scalability.
Keith: the version I gave above will avoid the extra database hit.
Gabe: actually, the situation where no id or name are provided in the params might be a good place to raise an exception, if this is an unexpected condition (for example, if the querystring is trimmed.)
How about this?
@user = User.find(:first, :condition => "id = #{params[:id]} or name = '#{params[:name]}'")
Wow.. you guys are harsh on the guy. In a good way though =)
Yeah... I think using exception handling for this case is bad. I'd vote for Geoff's solution @user = params[:id] ? User.find(params[:id]) : User.find_by_name(params[:name])
I guess I'll throw my $.02 in there now...
I think the point of Jeff's article is to illustrate just what everyone is talking about. There's more than one way to do it. It depends on your style and the goals you're trying to accomplish.
@Chad: I think I like your way best when it comes to readability. Although you incur the extra database hit, if in fact the id field is not nil (80% of the time according to Jeff's example), then it doesn't matter.
@Geoff: I like your approach too. No extra database hit. Although some people find the ? : syntax to be less conversation-like... one of the appealing things about Ruby.
@Keith (#2): Sorry, missing your point. You quote that "premature optimization is the root of all evil" then go on to say that he's not "thinking ahead for maintainability and scalability."
As for not using exception handling for flow control - ok, I'll buy that. Probably some minor performance concerns too. That said, for this particular example, does it accomplish the goal? Is is readable? Does anything else matter? Probably not.
I suppose that the fact that the debate is so lively is good thing. Don't think we'd all be discussing this at all if we were talking ASP.NET.
Aside from the idea that using exception handling for code like this, my question is whether an exception is "expensive" as I've been told it is in .NET?
Good discussion.
Just a quick update to say that I ran some benchmarks, and on my system, using rescue is about 9% slower than the if-else statement. The difference isn't as big as I was expecting.
Using exceptions for flow-control is pretty high up there on my code smells list. Add the aroma of hitting the DB when you don't need to and it's rank :) Two maxims that come to mind are:
@Keith Morrison: To me 9% is a HUGE difference. Imagine that logic was in a key routine for fetching tags on a page that gets Dugg and I bet 9% would seem bigger.
@Brian Eng: Keith's quote that "premature optimization is the root of all evil" actually goes very well with talking about maintainability and scalability. Maintainable code can be quickly looked at by another developer or even yourself say a year later and you instantly get what's going on. Scalable code can be quickly extended without having to rewrite a bunch of the old code. I've spent hours trying to unravel code that people have made "more elegant" in the name of optimization, thus taking away from maintainability.
As for your questions "does it accomplish the goal? Is is readable?" that depends on the version you're talking about. I prefer Geoff's because it's the most readable and it expresses intent while the rescue or find_by_id() versions feel disjointed. Actually, using your criteria of "does it accomplish the goal? Is it readable?" the original "if params[]..." version meets both of those very well with the added bonus of being the better performer.
Lastly, I've seen discussions very similar to this by C# folk (I'm assuming you mean C# since C# is to ASP.NET what Ruby is to Rails). In fact by Delphi and C++ and Lisp and JavaScript folk as well. I know it's hard to believe but C# and ASP.NET really do excite a lot of people just as much as Ruby or Rails does :)
@Shawn:
Totally in agreement with you. If I was writing an application that was going to potentially get dugg or the next Amazon or eBay, certainly 9% is significant and Geoff's solution is best. I actually think any of the solutions we're discussing are pretty understandable and maintainable; again, the point of the article is that these are the types of things that can be up for discussion, thanks to Ruby.
And I'm definitely not saying that there aren't any passionate people in the Softies world. Jeff and I were among them for many years. But it was the overwhelming excitement shown by those in the Ruby/Rails community that drew us to the technology in the first place, and led us to start this blog and quit our jobs to do Ruby full-time :)
"Don't think we'd all be discussing this at all if we were talking ASP.NET."
I like the original example the best too, or Geoff's. Its the same in .net, actionscript, javascript, etc...I don't see how this is Ruby specific. In C# I could write:
try{User = User.FindById(Request.Params["id"]);}catch{User = User.FindByName(Request.Params["name"])....but I wouldn't, obviously for the aforementioned reasons.
John Walker:
An Exception is ALWAYS more expensive than not throwing an exception. It doesn't matter what the actual cost is, it is always more than not throwing one at all. Using the ternary operator to check the :id param is probably the best solution in this case. Provided you never try to find on another param (like birthdate or email or first name vs last name).
Premature complexity is the root of all evil.
The point of this refactoring experiment is not how minimal the result became (or whether it was good style). The point is that jeff pushed the code through several permutations, each with the potential to catch his eye.
The great thing about flexible languages in general, and Ruby in particular, is that a good refactor will arrive at a well-formed English sentence. That, in turn, makes the code's style and efficiency easier to review, either now or at tune-up time.
I like Jeff's solution. Why?
Because imagine if you have the following:
if @user.votes.findby_commentid(params[:comment_id]) # the user has voted, so do one thing else # the user hasn't voted end
@user being nil is an exceptional situation as far as this little piece of flow control is concerned, which is why I like the idea of using an exception:
dosomething = true rescue @user.votes.findbycommentid(params[:comment_id]) if do_something # then do it else # don't end
I think it looks fairly straightforward visually, and it's clear what the code is going to do both if the rescue happens and if not.
The alternative is to do several nested tests. The more chained method calls you use, the more Jeff's syntax simplifies the code.... and I'd imagine the 9% fades into nothingness.
For Phill Kenoyer: Please don't! Inlining the params[:id] and params[:name] in an explicit database query just opens you to SQL injection. Which is very bad.
If doing the one line, should it be something like:
@user = ... rescue RecordNotFound nil
as other exceptions should just be allowed to propagate out?
However, the above syntax doesn't work and I haven't been able to find how in Ruby to do that. Does anybody know?
Hi, I'm a beginner. I've read about Schema Manipulation Outside Migrations and I dont think i understand this topic. Can anyone help me further understand this?
Thanks