Skip Navigation

You're viewing a single thread.

134 comments
  • Uses multiple returns... I'm switching to Windows.

    • You mean you are early-returning to windows, uh? You can't do that by your own rules.

    • What's wrong with multiple returns?

      • Maintainability.

        You can't read a block of code and as quickly and understand its control flow without reading every line, especially in regards to resource cleanup.

        For example say you have:

        ...
        if this:
            something
            or other
            and many more...
            ...
        else:
            yet another thing 
            and some more
            ...
        
        do some cleanup
        return
        ...
        

        Say you aren't exactly interested in what happens inside each branch. If you can assume that there's one return at the end of the block, you can see the if and else, you can reason about what values would trigger each branch, you can also see that no matter which branch is executed, the cleanup step will be executed before returning. Straightforward. I don't have to read all the lines of the branches to ensure the cleanup will be executed. If I can't assume a single return, I have to read all those lines too to ensure none of them jumps out of the function skipping the cleanup. Not having to think about such cases reduces the amount of reading needed and it makes reasoning about the block simpler. The bigger the blocks, the more the branches, the stronger the effect. You have one less foot-shotgun to think about. The easier you make it for your brain, the fewer mistakes it's gonna make. For all those days when you haven't slept enough.

        E: Oh also refactoring blocks of code out into functions is trivial when you don't have multiple returns. Extracting a block with a return in it breaks the parent control flow and requires changes in the implementation.

        E2: Shorter blocks do not obviate this argument. They just make things less bad. But they make almost everything less bad. Shorter blocks and single returns make things even better.

        • Bad advice. Early return is way easier to parse and comprehend.

          if (p1)
          {
              if(!p2)
              {
                  if(p3 || !p4)
                  {
                      *pOut = 10;
                  }
              }
          }
          

          vs

          if(!p1) return;
          if(p2) return;
          if(!p3 && p4) return;
          
          *pOut = 10;
          

          Early out makes the error conditions explicit, which is what one is interested in 90% of the time. After the last if you know that all of the above conditions are false, so you don’t need to keep them in your head.

          And this is just a silly example with 3 predicates, imagine how a full function with lots of state looks. You would need to keep the entire decision tree in your head at all times. That’s the opposite of maintainable.

          • I'm sure you are capable of rewriting that in 3 lines and a single nested scope followed by a single return. In fact in languages that use exceptions you have to use at least one subscope.

            Notice that in my example I didn't even broach the example with error conditions, cause it's trivial to write cleanly either way. Instead I talked about returns inside business logic. You can't unfuck that. 🐞

            • Since my previous example didn't really have return value, I am changing it slightly. So if I'm reading your suggestion of "rewriting that in 3 lines and a single nested scope followed by a single return", I think you mean it like this?

              int retval = 0;
              
              // precondition checks:
              if (!p1) retval = -ERROR1;
              if (p2) retval = -ERROR2;
              if (!p3 && p4) retval = -ERROR3;
              
              // business logic:
              if (p1 && !p2 && (p3 || !p4))
              {
                  retval = 42;
              }
              
              // or perhaps would you prefer the business logic check be like this?
              if (retval != -ERROR1 && retval != -ERROR2 && retval != -ERROR3)
              {
                  retval = 42;
              }
              
              // or perhaps you'd split the business logic predicate like this? (Assuming the predicates only have a value of 0 or 1)
              int ok = p1;
              ok &= !p2;
              ok &= p3 || !p4;
              if (ok)
              {
                  retval = 42;
              }
              
              return retval;
              

              as opposed to this?

              // precondition checks:
              if(!p1) return -ERROR1;
              if(p2) return -ERROR2;
              if(!p3 && p4) return -ERROR3;
              
              // business logic:
              return 42;
              

              Using a retval has the exact problem that you want to avoid: at the point where we do return retval, we have no idea how retval was manipulated, or if it was set multiple times by different branches. It's mutable state inside the function, so any line from when the variable is defined to when return retval is hit must now be examined to know why retval has the value that it has.

              Not to mention that the business logic then needs to be guarded with some predicate, because we can't early return. And if you need to add another precondition check, you need to add another (but inverted) predicate to the business logic check.

              You also mentioned resource leaks, and I find that a more compelling argument for having only a single return. Readability and understandability (both of which directly correlate to maintainability) are undeniably better with early returns. But if you hit an early return after you have allocated resources, you have a resource leak.

              Still, there are better solutions to the resource leak problem than to clobber your functions into an unreadable mess. Here's a couple options I can think of.

              1. Don't: allow early returns only before allocating resources via a code standard. Allows many of the benfits of early returns, but could be confusing due to using both early returns and a retval in the business logic
              2. If your language supports it, use RAII
              3. If your language supports it, use defer
              4. You can always write a cleanup function

              Example of option 1

              // precondition checks
              if(!p1) return -ERROR1;
              if(p2) return -ERROR2;
              if(!p3 && p4) return -ERROR3;
              
              void* pResource = allocResource();
              int retval = 0;
              
              // ...
              // some business logic, no return allowed
              // ...
              
              freeResource(pResource);
              return retval; // no leaks
              

              Example of option 2

              // same precondition checks with early returns, won't repeat them for brevity
              
              auto Resource = allocResource();
              
              // ...
              // some business logic, return allowed, the destructor of Resource will be called when it goes out of scope, freeing the resources. No leaks
              // ...
              
              return 42;
              

              Example of option 3

              // precondition checks
              
              void* pResource = allocResource();
              defer freeResource(pResource);
              
              // ...
              // some business logic, return allowed, deferred statements will be executed before return. No leaks
              // ...
              
              return 42;
              

              Example of option 4

              int freeAndReturn(void* pResource, const int retval)
              {
                  freeResource(pResource);
                  return retval;
              }
              
              int doWork()
              {
                  // precondition checks
              
                  void* pResource = allocResource();
              
                  // ...
                  // some business logic, return allowed only in the same form as the following line
                  // ...
              
                  return freeAndReturn(pResource, 42);
              }
              
        • If your function is so long that keeping track of returns becomes burdensome, the function is too long.

          I'm not a fan of returning status codes, but that's a pretty clear example of early return validation where you can't just replace it with a single condition check. Having a return value that you set in various places and then return at the end is worse than early return.

        • I hate it when some blame early returns for the lack of maintainability.

          Early returns are a great practice when doing argument validation and some precondition checks. They also avoid nested blocks that worsen readability.

          What's being described there is a function that tries to do too much and should be broken down. That's the problem, not early returns.

          • Early returns are very similar to gotos. One level of nesting to take care of validation is trivial in comparison. You're replacing logical clarity for minimal visual clarity. This is true regardless of the size of the function which shows that the size of the function isn't the determinant. You're not alone in your opinion, clearly, and I'm not going to convince you it's a bad practice but I'll just say what I think about it. 😅 This practice doesn't make it my team's codebase.

            • You can say any execution flow controls are like gotos - continue, break, exceptions, switch, even ifs are not much more than special cases of gotos.

              This is true regardless of the size of the function which shows that the size of the function isn’t the determinant

              Logical clarity does tend to worsen as the function grows. In general, it is easier to make sense of a shorter function than a longer one. I don't know how you could even say otherwise.

              Early returns are still great for argument validation. The alternative means letting the function execute to the end when it shouldn't, just guarded by if conditions - and these conditions any reader would have to keep in mind.

              When a reader comes across an early return, that's a state they can free from their reader memory, as any code below that would be unreachable if that condition was met.

              • I never said longer functions are not less clear. I said my argument is valid irrespective of the length of the function which shows that the problems I claim multiple returns bring are independent of function length. 😊

                Any validation you can write with a few early returns you can write with an equivalent conditional/s followed by a single nested block under it, followed by a single return. The reader is free to leave if the validation fails nearly the same, they have to glance that the scope ends at the end of the function. Looks at conditional - that's validation, looks at the nested block - everything here runs only after validation, looks after the block - a return. As I mentioned in another comment, validation is a trivial case to do either way. Returns inside business logic past validation is where the problematic bugs of this class show up which requires more thorough reading to avoid.

                If you gave me a PR with early returns only during validation, I probably won't ask you to rewrite it. If I see them further down, it's not going in.

                • Any validation you can write with a few early returns you can write with an equivalent conditional/s followed by a single nested block under it, followed by a single return. The reader is free to leave the validation behind just the same.

                  And that conditional indents your entire function one level - if you have more validation checks, that's one level of indentation per check (or a complicated condition, depends whether you can validate it all in one place). It's pretty much the case the other user illustrated above.

                  Returns inside business logic past validation is where the problematic bugs of this class show up

                  That much we agree. But again, this is not an early return issue, putting too much logic in a function is the issue. Rewriting it without early returns won't make it much clearer. Creating other functions to handle different scenarios will.

                  • Again, if you can write it with conditionals and returns, you can write it with equivalent number of conditionals and a single nested scope. No further scopes are needed. The conditional will even look nearly identically.

134 comments