Reducing Code Nesting

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.

    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

    # 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

"... if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program." - Linus Torvalds, Linux kernel coding style (partly justifying why the Linux kernel style uses 8-space indentation)

http://www.kernel.org/doc/Documentation/CodingStyle

> Every added level of nesting is another piece of context that your brain has to keep track of.

I disagree entirely. Nesting manifests the conditional-evaluation context in the presentation of the code. Without nesting, the context shifts must be held purely in the mind of the programmer (instead of offloading that storage to the layout of the code.) In dynamic languages especially, I've also found that early returns lead to more test failing while refactoring (or more bugs if the code has low test coverage.)

The issue I have with named functions (as shown in the article) is that context for inner blocks must be passed through the outer blocks, which means your intermediate function signatures have some irrelevant (to the local scope) cruft. There are some rough corner-cases where nested code is far less complicated than juggling context batons.

You can also place blocks of code in a list and recurse through the list until the user is found.

  let cacheUser user =
    cache.set(user.id,user)
    user

  let anonymousUser id =
    Some(new User())
 
  let sourceList = [(fun id -> cache.getUser(id),
                    (fun id -> db.getUser(id)),
                    anonymousUser] 

  let getCachedUser id =
   sourceList
   |> List.pick (fun source -> source id)
   |> cacheUser

    let sourceList = [cache.getUser; db.getUser; anonymousUser]

It's strange that many programmers in this topic feel that multiple return points are bad. It is very clear how nesting makes code logic more complex, but why many return points are a problem at all? They don't require you to do extra work while reading or writing the code, actually it is exactly the reverse IMHO, every return point remove some possible future state.

Also many times early return is an exact translation of the way we think in our natural languages: "if that is this way don't continue at all", "if this precondition is meet return that value", and so forth.

Why couldn't he write the first example like this? Relying on short-circuiting "or", I find this far more readable and maintainable than his final solution:

    def get_cached_user(user_id=None, username=None):
        """
        Returns a cached user object either by their user_id or by their username.
        """
        user = (cache.get_user_by_id(user_id)
                or cache.get_user_by_username(username)
                or db.get_user_by_id(user_id)
                or db.get_user_by_username(username))
        if not user:
            raise ValueError('User not found'))
        cache.set_user(user, id=user.id, username=user.username)
        return user

I didn't realize I instinctively picked up the early return habit until I started doing iOS development, where it seems the convention encouraged by apple is nesting rather than early returning.

I prefer the early return style. I generally try to put all error handling / sanity checking code up top with early returns (ie, the preconditions of the function), so that the "meat" of the code at the bottom of the function can be more concise and easier to grok. However, this requires a reading style of "first read the bottom of the function to get the gist of it, and then read all of the pre conditions above it". Unfortunately I have to explain that to my coworkers, bu when I do, they seem to understand wha I'm going for.

Returning early is, in my experience, a recipe for disaster.

Even in trivial 4 line functions I assign my return values to a variable and then return that at the end of the function.

This may just be my pedantry left over from when I first studied C at university, but more than once I've spent a while trying to figure out why a return value wasn't what I was returning (from some code that had been previously written by someone else) only to find that there was a return statement on the 3rd or 4th line of the function.

One could certainly argue that if a function were concise and readable and commented and all that stuff then this wouldn't be a problem, but that's not always the case and a little return statement sitting someone in the middle of the function can really throw you sometimes

I'm increasingly a fan of local named functions in JS, vs. pulling callbacks entirely out of scope. An example:

    function compile(filename, ready) {
      return fs.readFile(filename, make_fn)

      function make_fn(err, data) {
        if(err) return ready(err)

        ready(null, new Function(data))
      }
    }

Which neatly addresses the desire to retain closures, while avoiding unnecessary nesting.

Also, whenever possible, I like to nix callbacks by using Function#bind:

    res.on('data', accum.push.bind(accum))

    // vs:
    res.on('data', function(data) { accum.push(data) })

I largely try and do the same - use return and continue early and often. Aside from the things that you mentioned, one more benefit is that doing this makes it very easy to step through code using a debugger, and see which condition is failing (rather than having to print out various conditionals, or add log statements in and recompile/rerun).

Another simple way to reduce nesting is to use "continue" rather than wrapping entire loops in if statements.

For instance, this:

  for (file in files) {
    if (isOK(file)) {
      ...  // nested two levels
    }
  }

becomes:

  for (file in files) {
    if (!isOK(file)) continue;
    ...  // nested only one level
  }

Does anyone else hate multiple returns?

A do/while condition would satisfy this situation. or while True: if False: break in python

Cyclomatic complexity has been used for decades as a measure of software quality. ( Less is better. ) Any discussion of "reducing code nesting", should include a mention of it.

http://en.wikipedia.org/wiki/Cyclomatic_complexity

I like the article, but changing languages on me midway without warning me kind of threw me off.

Especially since I didn't notice that I was reading a different language (subconsciously, code is code) and then it kind of hit me in the face that python doesn't have curlys

There is some empirical evidence that excessive nesting correlates with bugs: http://franktheblue.blogspot.com/2011/04/minimize-nesting-of...

The "Maybe" monad solves exactly this problem.

One thing I like about early returns/continues, as opposed to nesting, is that when you're collaborating with people the diffs are much easier to read and make sense of. You practically never re-indent code.

OK, with changes in nesting you could just use a whitespace-free diff, but then you can't just apply/commit it. If you apply it, you have to reindent before committing. (This is tricky in python.) Alternatively, if you get a full diff, you have to apply it and then extract a whitespace-free diff yourself in order to read the actual changes properly.

First time I write in HN, but I really wanted to give my take on this.

I have to say that I really feel that to get readability, over nesting too, you often should refactor a bit the code.

I would in fact write the get_cached_user method by using separate methods and a @cache decorator. Every single function is very readable by itself.

(I'm not fluent in python, pseudo-python follows... but you should get the idea)

    def cache(function_to_cache):
        '''
        A Decorator that caches a function result
        '''
        def wrapper(param):
            cache_key = function_to_cache.name + " on " + param
            if cache.contains(cache_key):
                return cache.get(cache_key)
            else
                value = function_to_cache(param)
                cache.set(cache_key, value)
                return value
        return wrapper
    
    @cache
    def get_cached_user_by_id(id):
        return db.get_user_by_id(id)
    
    @cache
    def get_cached_user_by_username(username):
        return db.get_user_by_username(username)
        
    
    def get_cached_user(user_id = None, username = None):
        user = None
        if user_id:
            user = get_cached_user_by_id(user_id)
        else if username:
            user = get_cached_user_by_username(username)
        
        if not user:
            raise ValueError('User not found')
        
        return user

For Javascript, async.js[1] offers some good tools for different kinds of control flow with callbacks.

[1]: https://github.com/caolan/async

Depth of code nesting can be reduced using many syntactic tricks. You have shown the use of multiple return. This trick is generally considered as a hidden goto (go to the end of the function). 'GOTO considered harmful' is practically biblical law amongst many programmers, but it's worth remembering that he made that statement in the context of an argument with Donald Knuth. Knuth won: (http://pplab.snu.ac.kr/courses/adv_pl05/papers/p261-knuth.pd...) Strict application of usual "good" coding conventions works well in most of the cases, but always results in poor code for the remaining cases. I think a relaxed application of coding conventions would be better, but in case of code review, this requires a lot of efforts to explain why the not compliant code is cleaner. Writing code compliant to coding convention is frustrating, but is often easier than to convince stupid bosses. The compliant way to reduce code nesting is to create small functions (with a single return), even if they are called only once.

I'm surprised Eric didn't mention the Zen of Python:

  >>> import this
  ...snip...
  Flat is better than nested.

I think the last Erlang example should be simplified further. It uses two different values of Resp to signal an error ("Error" and "{timestamp, Start, Error}"), which can be unified for more clarity:

    do_some_file_thing(File) ->
        Resp = case file:open(File, [raw, binary, read]) of
            {ok, Fd} ->
                {timestamp, now(), process_file_data(Fd)};
            Error ->
                {timestamp, now(), Error}
        end,
        case Resp of
            {timestamp, Start, {ok, Processed}} ->
                {ok, Start, now(), Processed};
            {timestamp, Start, Error} ->
                Error
        end.

There is something wrong about this.

Code is a tree, code is about nesting. If you do not like nesting, you do not like code.

Code is not 'text'. You do not read code top to bottom like text. Code has a structure, and you read that structure.

And if an example of 'improvement' doubles the line count, you have a pretty good indication you are doing something wrong.

What seems to have happened is a small piece of advice has been taken too far. The early-return shortcut is reasonable. It is indeed advocated by Fowler and Beck (who deserve some trust) -- they call it 'replace nested conditional with guard clauses'. But that is something very particular. It does not suggest removal of all nesting in general.

Regarding the JavaScript part: I often use small state machines to unroll deep nesting. You get a better state tracking, less nesting and easier error handling :-) You should give it a try....

Perl style tends to use early return and also has the benefit of being able to emphasise the control flow by using a return with a trailing conditional. I.e.

    return $user if $found;

Did anyone else thing that the fact that he needed to do

        cache.set_user(user, id=user.id, username=user.username)

twice in his improved solution a bit of a giant red flag?

This issue comes up a lot between me and my coworker. He is very anti-nesting, and i prefer methods with single points of return. Obviously it is a balance to create readable code.

I wrote two articles with the same goal, using simple rules describing when you should simplify. I also cover cyclomatic complexity, which you seem to want to describe.

Regardless, the two rules I focus on:

* Idents are intents to modularize http://www.jasonlotito.com/programming/blocks/

* Comments indicate future refactoring http://www.jasonlotito.com/programming/comments/

They are both covered here:

Factor (the language) seems to encourage code with very low levels of nesting -- for most words, none at all.

http://factorcode.org

I actually found the second example to be quite nice. Even in my current sleepiness-fuzzed mental state, I was able to follow it easily. It was almost refreshing.

I guess that's one of the dangers of using contrived examples. Or maybe it's just proof that this is as much a matter of taste as anything.

Unfortunately the logic of this article is ass-backward. Multiple return paths involve jumps and make compiler jump and stack optimizations very difficult. What is easier to read and follow for a person does not always produce good instructions for a computer to follow.

The following also mentions something on the similar lines http://www.kernel.org/doc/Documentation/CodingStyle

Code Complete book has a very good section on reducing code complexity and gives various tips and tricks with the analysis of possible outcomes. Must read for developers, I think.

Goto

This is why I've become very fond of a little advertised part of haskell: the where clause. Every function can have its own little library of subfunctions, without polluting the toplevel namespace.

Whenever code starts getting complicated, I break it out into a subfunction in the where clause, which means I give it a name, which makes my code more self-documenting, and incidentally keeps the indentation level sane. Breaking things out into lots of little functions like this often makes it apparent when the function in the where clause is more generic, and does belong at the toplevel, and then the code is already separated into a function that's easy to move out of the where clause (closures do mean additional parameters sometimes need to be added, but the compiler will make sure you get this right).

The other nice thing haskell brings to the table, which would be more helpful in the first example given, is the ability to write your own control flow functions. For the first example, which keeps trying different actions from a list until one succeeds, and returns its value, I would write a generic function to do that. Its type signature would be:

    firstM :: (Monad m) => [m (Maybe a)] -> m (Maybe a)

Of course, I don't need to write that function.. I can just paste the above type signature into Hayoo, and get directed to an existing implementation: http://hackage.haskell.org/packages/archive/darcs/latest/doc...

If I did need to write firstM, I'd feel special to have been the first to think up such a generic and useful function. So it's win-win-win all the way. :)

Anyway, the code to use it would look something like this:

    get_cached_user uid name = fromMaybe nouser <$> find
      where
        cache a = do
          r <- a
          cache_set_user uid name r
          return r
        nouser = error "user not found"
        find = firstM
          [ cache_get_user_by_id uid
          , cache_get_user_by_username name
          , cache $ db_get_user_by_id uid
          , cache $ db_get_user_by_username name
          ]

As another example of this refactoring of control flow, looking at the cache function above I realized I've written those three lines several times before. So I just added this to my personal library:

    observe :: (Monad m) => (a -> m b) -> m a -> m a
    observe observer a = do
      r <- a
      observer r
      return r

(I seem to be the first person to think of this function.. yay!)

With this, the "cache" function can be written as just

        cache = observe $ cache_set_user uid name