DEV Community

Don't ALWAYS quick-return from functions

Ben Winding on April 13, 2018

There are two main styles of returning from a function in a program. Both styles are almost diametrically-opposed, but have their uses. ...

Read full post

Idan Arye • Apr 14 '18

RAII - if the language has it - is usually the best way to free resources at the end of a function. But if you have to free them manually, many modern languages have a try...finally construct:

int GetCost(request) {
    Checker checker = new Checker(request);
    try {
        int cost = 0;
        if (!checker.Important) 
            return 20;
        if (!checker.HasDiscount) 
            return 14;
        if (!checker.IsMember) 
            return 12;
        else
            return 10;
    } finally {
        delete checker;
    }
}

This will guarantee that no matter where which return statement ends the function, and even if there was an exception somewhere - delete checker will be invoked.

Andrew Buntine • Apr 13 '18

Nice writeup. In general I am not a fan of multiple returns per function (especially for very simple functions), although I am not sure I am following your argument.

In order for a function to leak memory in this fashion you'd need to be working in a language that lacks a garbage collector. So let's say the above is C or C++. If that's the case, the "request" struct would/should always be passed by reference (a pointer) and not by value. I am sure there are some situations where this type of single return is favourable due to potential memory leaks - but I'm just not sure this example is one of them. :)

I also think perhaps there is a bug in the solution as you always assign "cost=10" before returning it!

Ben Winding • Apr 14 '18

Thanks for the feedback Andrew! I've fixed the last example code in order to better demonstrate what I meant.

Yes, the argument I was trying to make is mainly concerned, but not limited to garbage collection. Actually, any resource may need to be 'freed' at the end of the function; boolean locks, system resources etc...

Also fixed the bug you spotted, thanks!

Andrew Buntine • Apr 14 '18

Yeah, nice one. The last example demonstrates the concept much better now.

Jan van Brügge • Apr 15 '18

The code is not really a good example as you shouldn't use new any more due to the problems outlined. Your code can be written this way:

int GetCost(request) {
    Checker checker = Checker(request);
    int cost = 0;
    if (!checker.Important)
        return 20;
    if (!checker.HasDiscount)
        return 14;
    if (!checker.IsMember)
        return 12;

    return 10;
}

If you want to use pointers, you should always wrap them in a smart pointer, e.g. std::make_shared(new Checker). That way they will also be removed when the scope is exited

Ben Winding • Apr 15 '18

Thanks for the feedback,

Smart pointers make me feel dumb, but they look like a great tool. I have a lot to learn about modern c++...

MTrost • Apr 15 '18 • Edited

In your last example, shouldn't these be else if-statements when you check hasDiscount and isMember?

Because as it now stands it always goes into the else-statement if the request is ~~not ~~a member, therefore always returning 10, even if it is important or has a discount.~~~~

But it's late for me so perhaps I don't get it :D

Anywho: Great write-up anyway, thanks for sharing!

EDIT: just realized the ! before the checks, so ignore my tired rambling :)

fabulous.yap • Apr 15 '18

nice write-up. am guilty of always refactor codes to return only once, it makes the codes cleaner and easier to debug... tho not doing it mainly for freeing of resources, as imho it's better handled with other constructs (try...finally, smart pointers, etc.) if the language supports it...

the last refactored seems to have a small logic issue, assuming the formatting is based on C/C++ language, hope you don't mind that I copy your code to create a small test program here... tpcg.io/JbGrUA

Ben Winding • Apr 15 '18

Thanks man, I don't mind at all, wow that's pretty cool you could do that online

inf3rno • Apr 15 '18 • Edited

One of the advantages of functional programming, that you can write a wrapper, which locks and unlocks the resources automatically, so you won't forget anything to unlock. This approach has it's limitations too ofc. Nice article though, I did not know that was the cause.

Alex Miasoiedov • Apr 14 '18

Single returns is always better unless 0.0000001% cases when you are writing in JS and found a weird bug of V8 JIT.

Bryan Baldwin • Apr 15 '18

Don't use new. That's your problem.

Shalvah • Apr 14 '18

Your final code snippet seems to be missing some braces

Ben Winding • Apr 14 '18

Cheers, is that fixed now?

Shalvah • Apr 14 '18 • Edited

Yes. But I should point out that your final code snippet and the initial one may not do the same thing.

Sppose the request is not Important, has no discount, and is not a member. The initial GetCost() will return 20; the final one will return 12.

The only way for the two to be exactly identical in function is if certain conditions are mutually exclusive, but that isn't stated anywhere.

Ben Winding • Apr 16 '18

Ahh I see, cheers for that. I'm beginning to regret using the embarrassing technique of using the audience as the debugger.