5 ms·

Yikes. Lots of return points have always felt like a code smell to me. I've found that it tends to create surprising and frequently hard-to-maintain code. I do

by cheald 15y ago

Yikes. Lots of return points have always felt like a code smell to me. I've found that it tends to create surprising and frequently hard-to-maintain code. I do use early returns somewhere, but I'm rarely happy about it. For things like the first example, why not something like (in Ruby): # Returns a cached user object either by their user_id or by their username. def get_cached_user(user_id = nil, username = nil) user = cache.get_user_by_id(user_id) || cache.get_user_by_username(username) || db.get_user_by_id(user_id) || db.get_user_by_username(username) || raise ValueError('User not found') cache.set_user(user, user.id, user.username) return user end You just try all your getters, cheapest first, and when the getter returns a nil or false value, the next one will be tried. Once one is found, no more are tried. Then you just set the resulting value in your cache and return the value. One return statement, no ugly nested ifs, and your unnecessary statements never evaluate, which is what you want anyhow.

ericflo 15y ago
Sigh, I knew using that contrived get_cached_user function with all those arguments would detract from my point, which is why I tried to couch it in disclaimers. My point still stands though: nesting is bad. The takeaway from your comment is that you can solve the issue sometimes by factoring out smaller more focused functions instead of returning early, as you've demonstrated.
- mitchty 15y ago
  I have always liked the Linux kernel coding style in this regard: Now, some people will claim that having 8-character indentations makes the code move too far to the right, and makes it hard to read on a 80-character terminal screen. The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program. Yes indentation != nested ifs, but in the end its about the same. I don't mind a bit of nesting, but I think Linus is onto something with >3 meaning: you've done something wrong, step away from the keyboard and think about what you've done.
  - ceol 15y ago
    >if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program. A single if statement in a class method would put the indentation at 3, meaning if you have something like this... class MyClass(object): ... def my_method(self, param): try: self.calculate(param) except MyException: if param == 100: ... else: ... you're screwed? I don't see how you can set such a definitive measure of bad code.
    - jrockway 15y ago
      Linux is written in C, so every "top-level" instruction is going to be one-level deep. In Python, the "top-level" is often the body of a method, which means it's two deep. So to apply Linus' advice to Python code, you can go four levels deep: class Foo(object): def my_method(self): try: if foo is bar: self.quux() except: pass This seems acceptable to me. Factor out the try block if you want more conditionals and use polymorphism instead of conditionals. Rather than writing: class X(object): def quux(self): if self.foo is self.bar: self.quux_foo() else: self.quux_bar() Write: class X(object): def quux(self): self.quux_bar() class FooIsBar(X): def quux(self): self.quux_foo() No conditionals and your intent is encoded in your class hierarchy instead of inside some random method.
      - ceol 15y ago
        Thanks for the thorough reply. I think his argument fits for C, but the linked article uses three different OO languages that would require a deeper "top-level", so I don't think it's fair to copy/paste his quote without any explanation or context.
      - sumeeta 15y ago
        FooIsBar doesn't need to be a subclass of X, does it? The polymorphism comes from the interface (implements `quux`), so inheritance doesn't seem relevant. Feel free to correct me if I'm wrong. Still trying to learn the difference between inheritance and "evil" inheritance :)
- jrockway 15y ago
  Your get_cached_user function is very authentic and is a great example of the kind of hairy code that a programmer should be looking for in his own code. There is no need to backpedal on this choice of example; it's fine. You should go one step further and conclude that nesting is a warning sign often indicative of bad code, and that just fixing the nesting is not the solution to the problem.
nodemaker 15y ago
This is clearly a very beautiful way to do this. But somehow I still havent internalized the fact that when assembly code is generated for such an OR statement it is done in a way such that the if any sub statement is evaluated to be true then all the other sub statements are not evaluated at all.This is true with ANDs as well. Can you please give me a link that proves conclusively that gcc and clang do this?
- marshray 15y ago
  It's guaranteed by the C and C++ standards, and for the ternary operator (x ? y : z) as well. http://en.wikipedia.org/wiki/Short-circuit_evaluation http://en.wikipedia.org/wiki/Short-circuit_evaluation
  - nodemaker 15y ago
    hmm...well I almost got convinced that I should use above technique a lot until I read that. Short-circuiting can lead to errors in branch prediction on modern processors, and dramatically reduce performance (a notable example is highly optimized ray with axis aligned box intersection code in ray tracing)[clarification needed]. Some compilers can detect such cases and emit faster code, but it is not always possible due to possible violations of the C standard. Highly optimized code should use other ways for doing this (like manual usage of assembly code).[citation needed]
    - marshray 15y ago
      Citation needed indeed. It's logically equivalent to an if/else and the compiler probably knows that. That sounds like someone may be thinking about the conditional-move and family on some CPU instructions. That could certainly interact with the branch prediction, but there's nothing about the operators that necessitate it. Sure, it may be that some compilers do generate different code and some runs faster than others, but that's just not the kind of thing you can tell without benchmarking those very specific conditions. If you're writing a ray tracer, you're probably are going to want to hand-tune the assembly but that's not the common case of C and C++ programming.
Someone 15y ago
That looks nicer, but you changed the code; The original code would not call cache.set_user when the data already was in the cache. You would need something in-between like: # Returns a cached user object either by their user_id or by their username. def get_cached_user(user_id = nil, username = nil) user = cache.get_user_by_id(user_id) || cache.get_user_by_username(username) if not user: user = db.get_user_by_id(user_id) || db.get_user_by_username(username) || raise ValueError('User not found') cache.set_user(user, user.id, user.username) return user end
- cheald 15y ago
  I knew someone would catch that. :) It's dependent on your infrastructure and demands, of course, but I probably wouldn't bother optimizing that unless cache sets were prohibitively expensive.
deleted 15y ago
[deleted]
div 15y ago
I really tend to like early returns and use them often. I usually try to make them use the form: return x if y To me, this makes it easier to follow a particular flow when reading code. As soon as I encounter a return that matches it's conditional, I don't have to care about the rest of the method anymore. During debugging, this also quickly lets me test assumptions by putting some log statements right after the return that should be occuring. The only time early returns irk me is when they're deeply nested, but as mentioned by TFA and others in this thread, having deeply nested code is a bit of a code smell in its own right.
- cheald 15y ago
  Those are typically considered to be guard clauses, and I agree that they're one of the more acceptable forms of early returns, but only if they're in the first few lines of the function. Returns in the middle of a 20-line function usually give me the twitches.
vog 15y ago
The "||" idiom is popular in many languages, but keep in mind that it skips not just NULL (nil) values! It also skips otherwise perfectly desirable values like FALSE, 0 (zero), or "" (empty string). In SQL there is the handy COALESCE function [1] which does exactly what I want in this case: It simply returns the first non-NULL value of its arguments, even if it is FALSE, 0 or "". def get_cached_user(user_id = nil, username = nil) user = coalesce( cache.get_user_by_id(user_id), cache.get_user_by_username(username), db.get_user_by_id(user_id), db.get_user_by_username(username) ) if user.nil? raise ValueError('User not found') cache.set_user(user, user.id, user.username) return user end I always wonder why none of the other programming languages provide a handy COALESCE function/operator out of the box. That way, they wouldn't encourage bug-provoking hacks such as abusing the "||" operator. Of course you can always implement your own COALESCE function, but that won't provide the nice short-circuit ability. So this is only feasible in languages with macros (e.g. Lisp), or languages which are lazy-evaluated (e.g. Haskell). All other languages should either provide a COALESCE function/operator, or a sane alternative to "||" which skips only NULL values and nothing else. [1] http://www.postgresql.org/docs/9.1/static/functions-conditional.html#FUNCTIONS-COALESCE-NVL-IFNULL http://www.postgresql.org/docs/9.1/static/functions-conditio...
- skrebbel 15y ago
  C#'s ?? operator does exactly that. `x ?? y` returns y iff x is null. In daily practice, I've hardly ever used it. I use early returns all the time, though, and the code becomes very nice and readable.
- cytzol 15y ago
  > All other languages should either provide a COALESCE function/operator, or a sane alternative to "||" which skips only NULL values and nothing else. In Ruby, only nil and false are false, so you can use || to catch 0s and empty strings. In Perl, 0s and empty strings are false, but there's the // operator (as well as the || or or operators) that catches the first defined value. I'm not sure if this is something specific to the database framework that you're using - sorry if you know all that already.
jonathanwallace 15y ago
I'd love to hear your experiences as to how it leads to surprising and frequently hard-to-maintain code. I've found quite the opposite. Furthermore, I've found that early returns allows one, even in the original post's example, to easily specify a distinct error message for each case.
- cheald 15y ago
  Multiple return points are a signal to me that I'm usually (though not always) trying to encapsulate more functionality into a function than I should, or I'm using returns when it might be more semantically correct to use exceptions. I'm a big fan of the "each function does exactly one thing" rule of thumb, and when I start returning all over the place, it usually means that I'm starting to violate that rule. Except in the simple cases, multiple return statements usually means that I've got a function that's composed of multiple micro-functions, and can/should be refactored. Guard statements are an obvious exception. Sometimes you can't avoid it, especially if you're working with duck typing functions (which are a whole 'nother discussion), and it's not a hard-and-fast rule that multiple returns are evil. There are a lot of legitimate reasons to use them - performance being the primary one - and I would absolutely take them over an 8-deep nested if structure. But, they're often a warning sign that I'm putting too many eggs in that one basket.

by cheald 15y ago

ericflo 15y ago

Sigh, I knew using that contrived get_cached_user function with all those arguments would detract from my point, which is why I tried to couch it in disclaimers. My point still stands though: nesting is bad. The takeaway from your comment is that you can solve the issue sometimes by factoring out smaller more focused functions instead of returning early, as you've demonstrated.

mitchty 15y ago
I have always liked the Linux kernel coding style in this regard: Now, some people will claim that having 8-character indentations makes the code move too far to the right, and makes it hard to read on a 80-character terminal screen. The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program. Yes indentation != nested ifs, but in the end its about the same. I don't mind a bit of nesting, but I think Linus is onto something with >3 meaning: you've done something wrong, step away from the keyboard and think about what you've done.
- ceol 15y ago
  >if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program. A single if statement in a class method would put the indentation at 3, meaning if you have something like this... class MyClass(object): ... def my_method(self, param): try: self.calculate(param) except MyException: if param == 100: ... else: ... you're screwed? I don't see how you can set such a definitive measure of bad code.
  - jrockway 15y ago
    Linux is written in C, so every "top-level" instruction is going to be one-level deep. In Python, the "top-level" is often the body of a method, which means it's two deep. So to apply Linus' advice to Python code, you can go four levels deep: class Foo(object): def my_method(self): try: if foo is bar: self.quux() except: pass This seems acceptable to me. Factor out the try block if you want more conditionals and use polymorphism instead of conditionals. Rather than writing: class X(object): def quux(self): if self.foo is self.bar: self.quux_foo() else: self.quux_bar() Write: class X(object): def quux(self): self.quux_bar() class FooIsBar(X): def quux(self): self.quux_foo() No conditionals and your intent is encoded in your class hierarchy instead of inside some random method.
    - ceol 15y ago
      Thanks for the thorough reply. I think his argument fits for C, but the linked article uses three different OO languages that would require a deeper "top-level", so I don't think it's fair to copy/paste his quote without any explanation or context.
    - sumeeta 15y ago
      FooIsBar doesn't need to be a subclass of X, does it? The polymorphism comes from the interface (implements `quux`), so inheritance doesn't seem relevant. Feel free to correct me if I'm wrong. Still trying to learn the difference between inheritance and "evil" inheritance :)
jrockway 15y ago
Your get_cached_user function is very authentic and is a great example of the kind of hairy code that a programmer should be looking for in his own code. There is no need to backpedal on this choice of example; it's fine. You should go one step further and conclude that nesting is a warning sign often indicative of bad code, and that just fixing the nesting is not the solution to the problem.

nodemaker 15y ago

This is clearly a very beautiful way to do this. But somehow I still havent internalized the fact that when assembly code is generated for such an OR statement it is done in a way such that the if any sub statement is evaluated to be true then all the other sub statements are not evaluated at all.This is true with ANDs as well. Can you please give me a link that proves conclusively that gcc and clang do this?

marshray 15y ago
It's guaranteed by the C and C++ standards, and for the ternary operator (x ? y : z) as well. http://en.wikipedia.org/wiki/Short-circuit_evaluation http://en.wikipedia.org/wiki/Short-circuit_evaluation
- nodemaker 15y ago
  hmm...well I almost got convinced that I should use above technique a lot until I read that. Short-circuiting can lead to errors in branch prediction on modern processors, and dramatically reduce performance (a notable example is highly optimized ray with axis aligned box intersection code in ray tracing)[clarification needed]. Some compilers can detect such cases and emit faster code, but it is not always possible due to possible violations of the C standard. Highly optimized code should use other ways for doing this (like manual usage of assembly code).[citation needed]
  - marshray 15y ago
    Citation needed indeed. It's logically equivalent to an if/else and the compiler probably knows that. That sounds like someone may be thinking about the conditional-move and family on some CPU instructions. That could certainly interact with the branch prediction, but there's nothing about the operators that necessitate it. Sure, it may be that some compilers do generate different code and some runs faster than others, but that's just not the kind of thing you can tell without benchmarking those very specific conditions. If you're writing a ray tracer, you're probably are going to want to hand-tune the assembly but that's not the common case of C and C++ programming.

Someone 15y ago

That looks nicer, but you changed the code; The original code would not call cache.set_user when the data already was in the cache. You would need something in-between like: # Returns a cached user object either by their user_id or by their username. def get_cached_user(user_id = nil, username = nil) user = cache.get_user_by_id(user_id) || cache.get_user_by_username(username) if not user: user = db.get_user_by_id(user_id) || db.get_user_by_username(username) || raise ValueError('User not found') cache.set_user(user, user.id, user.username) return user end

cheald 15y ago
I knew someone would catch that. :) It's dependent on your infrastructure and demands, of course, but I probably wouldn't bother optimizing that unless cache sets were prohibitively expensive.

deleted 15y ago

[deleted]

div 15y ago

I really tend to like early returns and use them often. I usually try to make them use the form: return x if y To me, this makes it easier to follow a particular flow when reading code. As soon as I encounter a return that matches it's conditional, I don't have to care about the rest of the method anymore. During debugging, this also quickly lets me test assumptions by putting some log statements right after the return that should be occuring. The only time early returns irk me is when they're deeply nested, but as mentioned by TFA and others in this thread, having deeply nested code is a bit of a code smell in its own right.

cheald 15y ago
Those are typically considered to be guard clauses, and I agree that they're one of the more acceptable forms of early returns, but only if they're in the first few lines of the function. Returns in the middle of a 20-line function usually give me the twitches.

vog 15y ago

The "||" idiom is popular in many languages, but keep in mind that it skips not just NULL (nil) values! It also skips otherwise perfectly desirable values like FALSE, 0 (zero), or "" (empty string). In SQL there is the handy COALESCE function [1] which does exactly what I want in this case: It simply returns the first non-NULL value of its arguments, even if it is FALSE, 0 or "". def get_cached_user(user_id = nil, username = nil) user = coalesce( cache.get_user_by_id(user_id), cache.get_user_by_username(username), db.get_user_by_id(user_id), db.get_user_by_username(username) ) if user.nil? raise ValueError('User not found') cache.set_user(user, user.id, user.username) return user end I always wonder why none of the other programming languages provide a handy COALESCE function/operator out of the box. That way, they wouldn't encourage bug-provoking hacks such as abusing the "||" operator. Of course you can always implement your own COALESCE function, but that won't provide the nice short-circuit ability. So this is only feasible in languages with macros (e.g. Lisp), or languages which are lazy-evaluated (e.g. Haskell). All other languages should either provide a COALESCE function/operator, or a sane alternative to "||" which skips only NULL values and nothing else. [1] http://www.postgresql.org/docs/9.1/static/functions-conditional.html#FUNCTIONS-COALESCE-NVL-IFNULL http://www.postgresql.org/docs/9.1/static/functions-conditio...

skrebbel 15y ago
C#'s ?? operator does exactly that. `x ?? y` returns y iff x is null. In daily practice, I've hardly ever used it. I use early returns all the time, though, and the code becomes very nice and readable.
cytzol 15y ago
> All other languages should either provide a COALESCE function/operator, or a sane alternative to "||" which skips only NULL values and nothing else. In Ruby, only nil and false are false, so you can use || to catch 0s and empty strings. In Perl, 0s and empty strings are false, but there's the // operator (as well as the || or or operators) that catches the first defined value. I'm not sure if this is something specific to the database framework that you're using - sorry if you know all that already.

jonathanwallace 15y ago

I'd love to hear your experiences as to how it leads to surprising and frequently hard-to-maintain code. I've found quite the opposite. Furthermore, I've found that early returns allows one, even in the original post's example, to easily specify a distinct error message for each case.

cheald 15y ago
Multiple return points are a signal to me that I'm usually (though not always) trying to encapsulate more functionality into a function than I should, or I'm using returns when it might be more semantically correct to use exceptions. I'm a big fan of the "each function does exactly one thing" rule of thumb, and when I start returning all over the place, it usually means that I'm starting to violate that rule. Except in the simple cases, multiple return statements usually means that I've got a function that's composed of multiple micro-functions, and can/should be refactored. Guard statements are an obvious exception. Sometimes you can't avoid it, especially if you're working with duck typing functions (which are a whole 'nother discussion), and it's not a hard-and-fast rule that multiple returns are evil. There are a lot of legitimate reasons to use them - performance being the primary one - and I would absolutely take them over an 8-deep nested if structure. But, they're often a warning sign that I'm putting too many eggs in that one basket.