In Ruby, you don’t have to specify an explicit return value from a method. Ruby will just return the last evaluated statement. Similarly, if an explicit return statement will make itself the last evaluated statement – i.e return control to the caller with the specified return value.
However, I’m not a big fan of explicit return statements. In my experience, the only place where they make sense is in the first line of the method, where the control is returned to the caller if the supplied arguments are not valid/expected. Consider the following method :
1 2 3 4 |
def read(file_name, options = nil) return nil unless File.exist?(file_name) .... end |
I think the above is the only case where I feel it’s ok to use an explicit return as it’s much better than the alternative – wrapping the entire method in a big if block. Also, you don’t really need to specify nil. The above can be rewritten as :
1 2 3 4 |
def read(file_name, options = nil) return unless File.exist?(file_name) .... end |
Now the real problem is visible when you look at the full method :
1 2 3 4 5 6 7 8 9 |
def read(file_name, options = nil) return nil unless File.exist?(file_name) if expires_in(options) > 0 return nil end File.open(file_name, 'rb') { |f| Marshal.load(f) } end |
A much simpler version of the above method is :
1 2 3 4 5 |
def read(file_name, options = nil) if File.exist?(file_name) && expires_in(options) <= 0 File.open(file_name, 'rb') { |f| Marshal.load(f) } end end |
Of course there’s no one ring to rule them all. It might be desirable to use multiple returns in a method. But every time you do that, take a moment to make sure it’s making the code easier to read.
Totally agree. And it’s way faster too!! http://gist.github.com/160718
I agree it’s a “code smell”, but it seems less offensively so when the conditional logic isn’t so simple or you aren’t returning nil in the (implied) else clause.
Agreed 100%. Multiple returns are a code smell, while unnecessary usage is a developer smell; not understanding that all ruby statements return a value.
I think guard clauses like that are useful, though, when you have a good bit of intense logic going on, or in this case, where it’s not totally apparent what’s going to happen if the condition isn’t met. I personally prefer obvious code to simple code. :P
I’m afraid I disagree, I prefer two “return if X” and “return unless Y” at the top of my code than to use full (and multi-condition) if as in the last example. When you right “return if this / return unless that / do-this” it’s very straight forward. The “if this and not that then do-this” is much harder and more dense.
However, this is all just a personal opinion point, I think both are OK. I’m just sitting on the other side of the fence.
This makes sense, I too don’t like to have returns flowing around (not only in ruby but in most other languages). And the first line is the only one where return could be used.
Coming from a Java world, I assume it could be better to use exceptions (in the above at least).
The above if this and that looks quite PHPish tome
But as mentioned before,I’m no ruby wizzard (as you probably are).
It depends on the complexity of the method. For a single clause, that’s fine.
It’s more of a problem when you stack several levels:
def read(file_name, options = nil)
return unless File.exist?(file_name)
if expires_in(options) <= 0
File.open(file_name, ‘rb’) { |f| Marshal.load(f) }
else
File.delete(file_name)
end
end
Here I wouldn’t expand the first condition in the method, as that would add more conditional frames you have to keep in mind as you read the code.
I think the problem is, people always take advice the wrong way, just as this will probably be taken to mean “go through great hoops to avoid using ‘return’” by various people.
I generally come across way more code that needs fast exits adding, than being removed, in order to maintain a balance for readability and clarity of conditional cases.
Greetings,
I find that if I’m filling out a variable, and there’s no natural way to have the contents of the variable be the last expression in the function, I very much prefer
over
I hope that formats right; no preview, unfortunately…
Anyhow, I just find that it’s more intention revealing to add the short ‘return’ before the variable at the end. I do usually try to find ways to not need an accumulation variable, though, but I don’t want to tie the logic into pretzels to manage that.
— Morgan
Agreed with raggi and others plus it may be also useful to put return to methods where we 1) explicitely requires the given value (in lots of methods we actually don’t care what’s the return value) 2) it looks ugly (for example we have few longer lines and last line is just “nil” or so)
Has everyone already agreed that it’s better to use one form or the other 100% of the time? I rarely litter methods with returns, but like Morgan I’ll occasionally return the last statement, usually for longer methods. It seems to me that one or the other is going to be more or less readable depending on the code involved.
“simple” is a tricky word that means different things to different people. in your case, the “complex” example is more expressive because it alerts the reader to what is apparently a very important condition. you then have another code chunk, where there is another situation that is important for this method. then, finally you have the real meat of the method, it’s supposed to un-marshal a file.
with having both checks in the same line, a person needs to keep the state of both items in their head. in this example there isn’t much to be confused about, but it can quickly get complex with nested conditionals and !’s. the downside of most nested conditionals is typically the state that you are actually interested in is buried near the bottom of the expression.
to improve your “complex” code further, i would take the second chunk, where it checks expires_in, and hide that in a more expressive method name that keeps each statement at the same level of abstraction.
The preferred solution just looks like procedural, imperative code. I think the Ruby idiom is clearer.
I’m coming to this very late, but nevertheless – I disagree. I’ve been bitten several times by people not realising that a function is supposed to return something, and absent-mindedly adding something at the end to do logging, or whatever. Sure, maybe they should pay more attention, or maybe the function needs a better name. but for me, an explicit return statement at the end of a function isn’t a clue for the compiler/interpreter, it’s a clue for the next poor guy to work on the code that things rely on this.
I don’t like the ‘simpler’ version of that method – I don’t like the way that the main execution flow of the thing is off in a conditional, and I don’t like the way that the thing that it returns is off on the right side of the screen and nested inside two blocks.