{"id":165,"date":"2009-08-31T11:28:56","date_gmt":"2009-08-31T00:28:56","guid":{"rendered":"http:\/\/blog.mozilla.org\/nnethercote\/?p=165"},"modified":"2009-08-31T15:59:00","modified_gmt":"2009-08-31T04:59:00","slug":"no-else-after-return-considered-harmful","status":"publish","type":"post","link":"https:\/\/blog.mozilla.org\/nnethercote\/2009\/08\/31\/no-else-after-return-considered-harmful\/","title":{"rendered":"“No-else-after-return” considered harmful"},"content":{"rendered":"

(Update below)<\/strong><\/p>\n

At the time of writing this, the Mozilla Coding Style guidelines<\/a> have this recommendation under “General C\/C++ Practices”:<\/p>\n

Don’t put an else right after a return. Delete the else, it’s unnecessary and increases indentation level.<\/p><\/blockquote>\n

I can appreciate this in some circumstances, such as when checking for an error case early in a long function:<\/p>\n

void f()\r\n{\r\n    if (errorcase())\r\n        return;\r\n\r\n    a();\r\n    b();\r\n    c();\r\n    d();\r\n}<\/pre>\n
But as a blanket guideline I think it’s misguided because it promotes an impure style of programming.\u00a0 And by “impure” here I mean the opposite of “pure” in this<\/a> sense<\/a>, i.e. “free from side-effects and other such error-prone junk”.\u00a0 Let’s consider a simple example:\u00a0 the max() function.\u00a0 Here’s a way to write it that is quite sensible, in my opinion:<\/p>\n
int max(int a, int b)\r\n{\r\n    if (a > b)\r\n        return a;\r\n    else\r\n        return b;\r\n}<\/pre>\n
But this violates the no-else-after-return guideline.\u00a0 The Mozilla way to write this is like so:<\/p>\n
int max(int a, int b)\r\n{\r\n    if (a > b)\r\n        return a;\r\n    return b;\r\n}<\/pre>\n
(I’m not exaggerating, see this comment<\/a> and this comment<\/a> from Mozilla’s VP of Engineering and CTO respectively, about an example very similar to this.)<\/p>\n
Urk.\u00a0 That’s horrible.\u00a0 Any time you’re using an ‘if’ without a corresponding ‘else’, you’re relying on an impure feature — you must be doing something with a side-effect in the ‘then’ branch, or some kind of impure control flow.\u00a0 Mozilla’s coding guidelines insist that we write max(), a pure function, in an impure way.\u00a0 Also, it’s just harder to read because the two return statements have different indentation levels despite the fact that they are closely related.<\/p>\n
In this case there’s a way around the problem:<\/p>\n
\r\n
int max(int a, int b)\r\n{\r\n    return (a > b ? a : b);\r\n}<\/pre>\n
Even though C’s if-then-else expression syntax is awful, it’s a better way to do it.<\/p>\n
In general, statements suck<\/a>, expressions rule.\u00a0 Even in a language like C\/C++ you can do something approaching functional programming by avoiding statements where possible.\u00a0 In a tiny example like max() the difference may seem trivial, but in bigger examples the difference becomes more important.\u00a0 Consider the following code from Nanojit’s register allocator which picks a register by looking at the free registers, the registers that are allowed to be used, and possibly the preferred registers.\u00a0 You don’t have to understand it closely, but it helps to know that RegisterMask is just a bitfield with one bit per register.<\/p>\n
Register Assembler::registerAlloc(RegisterMask allow)\r\n{\r\n    RegAlloc &regs = _allocator;\r\n    RegisterMask prefer = SavedRegs & allow;\r\n    RegisterMask free = regs.free & allow;\r\n\r\n    RegisterMask set = prefer;\r\n    if (set == 0) set = allow;\r\n\r\n    if (free)\r\n    {\r\n        \/\/ at least one is free\r\n        set &= free;\r\n\r\n        \/\/ ok we have at least 1 free register so let's try to pick\r\n        \/\/ the best one given the profile of the instruction\r\n        if (!set)\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 \u00a0\r\n        {\r\n            \/\/ desired register class is not free so pick first of any class\r\n            set = free;\r\n        }\r\n        NanoAssert((set & allow) != 0);\r\n        Register r = nRegisterAllocFromSet(set);\r\n        regs.used |= rmask(r);\r\n        return r;\r\n    }\r\n\r\n    \/\/ ... more code here ...\r\n}<\/pre>\n
I find this code incredibly hard to understand.\u00a0 There are three ‘ifs’ that lack ‘elses’, so it relies heavily on state updates to do its work.\u00a0 In fact, in order to understand it, I ended up rewriting it in a more pure style, by using a sequence of small refactorings:<\/p>\n
Register Assembler::registerAlloc(RegisterMask allow)\r\n{\r\n    RegAlloc &regs = _allocator;\r\n    RegisterMask allowedAndFree = allow & regs.free;\r\n\r\n    if (allowedAndFree)\r\n    {\r\n        \/\/ At least one usable register is free -- no need to steal. \u00a0\r\n        \/\/ Pick a preferred one if possible.\r\n        RegisterMask preferredAndFree = allowedAndFree & SavedRegs;\r\n        RegisterMask set = ( preferredAndFree ? preferredAndFree : allowedAndFree );\r\n        Register r = nRegisterAllocFromSet(set);\r\n        regs.used |= rmask(r);\r\n        return r;\r\n    }\r\n
    \/\/ ... more code here ...\r\n}<\/pre>\n
There’s now only a single ‘if’ lacking an ‘else’, and it turns out even that can be removed by putting the “… more code here …” part into an else branch, and then the ‘return r;’ can be moved to the end of the function.<\/p>\n
The code is now comprehensible — it picks a register that is free and allowed, and preferred as well if possible.<\/p>\n
Part of the art of programming involves knowing when to avoid using a language feature.\u00a0 Everybody knows that ‘goto’ is occasionally highly useful, but should be avoided whenever something less dangerous (a for-loop, a while-loop, a ‘break’ statement) can be used.\u00a0 And everybody knows it because everybody is taught it.\u00a0 But programmers aren’t taught to avoid unnecessary side-effects… at least, I wasn’t, certainly not via examples such as those I’ve given above.\u00a0 I’ve only come to realise these things as I’ve switched several times between using C\/C++ and some pure functional languages over the years, and come to appreciate how purity helps make programs more readable and less error-prone.<\/p>\n
To come full circle, I’d like to see the no-else-after-return guideline removed from the Mozilla style guide, or at the very least, be qualified in a way that allows the cases I’ve argued for.\u00a0 I’d also love to see a guideline (with examples) added such as “prefer expressions to statements”.<\/p>\n
Update:<\/strong><\/p>\n
(The arguments had the wrong names in the initial post, I’ve fixed them now.)<\/p>\n
All three versions of max() are pure, because it’s possible to combine impure pieces of code (such as an else-less if) into a pure piece of code.\u00a0 So I’ll augment my recommendation: pure code is preferable, and furthermore, <\/em>given that C\/C++ makes it so easy to write impure code, it’s preferable that pure code be obviously pure.<\/em> I argue that the 3rd version of max() with the ?: operator is the most obviously pure, because a ?: is pure if its operands are pure.\u00a0 In comparison, it’s harder to tell if an if-then-else is pure (although this case is still pretty easy thanks to the matching return statements).\u00a0 And an if-without-else almost never results in pure code;\u00a0 the max() example above is a rare exception.<\/p>\n
I also omitted a fourth formulation of max:<\/p>\n
int max(int a, int b)\r\n{\r\n    int m;\r\n    if (a > b)\r\n        m = a;\r\n    else\r\n        m = b;\r\n    return m;\r\n}<\/pre>\n
For this very simple case, such a formulation is overkill.\u00a0 But if the ‘then’ and ‘else’ cases are more complicated it makes more sense.\u00a0 (Furthermore, much of this discussion should be thought of in more complex cases, as these kinds of small decision can make a bigger difference then.)\u00a0 This version also doesn’t violate the no-else-after-return rule.\u00a0 So perhaps that rule isn’t as bad as I thought, if only because it’s easier to work around than I thought… but I still think rewriting the 1st version of max() into the 2nd version is a bad idea.<\/p>\n","protected":false},"excerpt":{"rendered":"
(Update below) At the time of writing this, the Mozilla Coding Style guidelines have this recommendation under “General C\/C++ Practices”: Don’t put an else right after a return. Delete the else, it’s unnecessary and increases indentation level. I can appreciate this in some circumstances, such as when checking for an error case early in a […]<\/p>\n","protected":false},"author":139,"featured_media":0,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[540,528,616,617],"tags":[],"_links":{"self":[{"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/posts\/165"}],"collection":[{"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/users\/139"}],"replies":[{"embeddable":true,"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/comments?post=165"}],"version-history":[{"count":0,"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/posts\/165\/revisions"}],"wp:attachment":[{"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/media?parent=165"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/categories?post=165"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/blog.mozilla.org\/nnethercote\/wp-json\/wp\/v2\/tags?post=165"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}