Proposed change to WebKit Code Style Guidelines for if-return-else-return

• Component: New Bugs
• Status: NEW ---

[Sam Weinig]

I'd like to propose a change to the WebKit Code Style Guidelines (https://webkit.org/code-style-guidelines/) specifically around this rule: https://webkit.org/code-style-guidelines/#linebreaking-else-if. 

The current rules state that "An else if statement should be written as an if statement when the prior if concludes with a return statement." Additionally, this is generally construed to apply to else statements after a return being elided entirely as well. e.g.

    if (condition)
        return foo;
    else
        return bar;

should be written as:

    if (condition)
        return foo;
    return bar;


I think we should remove this rule and leave it up to the writer to choose how to style this, allowing both else-if after a return and else after a return. My arguments for allowing this are:

- It is often easier to follow logic if you can read out the "if, else-if, else-if, else", rather than having to notice that there is a return in the statement and adding the else in your head.
- For single line conditions like the above example, the lining up of the return statements is pleasing.
- For constexpr if, the elses are required, so we have to break the rule for them anyway.

I'm eager to paint this bike shed, so what do you think?

[Chris Dumez]

I did run into this recently when introducing an "if constexpr" and had to ignore the rule then. That said, unless forced to by constexpr, I actually like that not using an else statement reduces the nesting. It is the same reason we like early returns in WebKit.

I think this would look terrible for e.g.:
```
if (myEarlyReturnCondition)
    return;
else {
    // My
    // long
    // function
    // body.
}
```

For single line conditions, the having the else statement doesn't look bad but I don't find it more pleasing to have the else statement :)

I also personally don't find it confusing or harder to follow-up that we don't have an else after an early return.

Anyway, if others disagree with me fine but my 2 cents is that I prefer the current coding style (except in the very specific case of if constexpr where we don't have any other choice).

[Darin Adler]

I like early return when it’s a true "early exit". I think it can make it much easier to follow the logic of a function.

I don’t think we should insist on it when the two sides of the condition are parallel, especially when both are simple one-liners.

In practice, we are not enforcing an early return style.

[Sam Weinig]

(In reply to Chris Dumez from comment #1)
> I did run into this recently when introducing an "if constexpr" and had to
> ignore the rule then. That said, unless forced to by constexpr, I actually
> like that not using an else statement reduces the nesting. It is the same
> reason we like early returns in WebKit.
> 
> I think this would look terrible for e.g.:
> ```
> if (myEarlyReturnCondition)
>     return;
> else {
>     // My
>     // long
>     // function
>     // body.
> }
> ```
> 

I agree this would be bad, and I don't think we should have a rule to require this. I am very much proposing this. I am just proposing that we call this decision a "styleistic choice". I happen to think something like:

```
if (aCondition) {
   // Do
   // Something
   // here 
   return foo;
} else if (anotherCondition) {
   // Do
   // Something
   // else 
   return bar;
} else {
   // Do
   // a.
   // third thing
   return baz; 
}
```

is quite pleasing. Especially since if you didn't have the early returns, we would write it:

if (aCondition) {
   // Do
   // Something
   // here 
} else if (anotherCondition) {
   // Do
   // Something
   // else 
} else {
   // Do
   // a.
   // third thing
}



> For single line conditions, the having the else statement doesn't look bad
> but I don't find it more pleasing to have the else statement :)
> 
> I also personally don't find it confusing or harder to follow-up that we
> don't have an else after an early return.

I am not really arguing that it is confusing, just that in some cases it is more pleasing to have the else.

        switch (format.pixelFormat) {
        case PixelFormat::RGBA8:
            if (format.alphaFormat == AlphaPremultiplication::Premultiplied)
                return std::make_tuple(8u, 32u, kCGBitmapByteOrder32Big | kCGImageAlphaPremultipliedLast);
            else
                return std::make_tuple(8u, 32u, kCGBitmapByteOrder32Big | kCGImageAlphaLast);

        case PixelFormat::BGRA8:
            if (format.alphaFormat == AlphaPremultiplication::Premultiplied)
                return std::make_tuple(8u, 32u, kCGBitmapByteOrder32Little | kCGImageAlphaPremultipliedFirst);
            else
                return std::make_tuple(8u, 32u, kCGBitmapByteOrder32Little | kCGImageAlphaFirst);

        case PixelFormat::RGB10:
        case PixelFormat::RGB10A8:
            break;
        }

looks more pleasing to me and a bit easier to follow than 

        switch (format.pixelFormat) {
        case PixelFormat::RGBA8:
            if (format.alphaFormat == AlphaPremultiplication::Premultiplied)
                return std::make_tuple(8u, 32u, kCGBitmapByteOrder32Big | kCGImageAlphaPremultipliedLast);
            return std::make_tuple(8u, 32u, kCGBitmapByteOrder32Big | kCGImageAlphaLast);

        case PixelFormat::BGRA8:
            if (format.alphaFormat == AlphaPremultiplication::Premultiplied)
                return std::make_tuple(8u, 32u, kCGBitmapByteOrder32Little | kCGImageAlphaPremultipliedFirst);
            return std::make_tuple(8u, 32u, kCGBitmapByteOrder32Little | kCGImageAlphaFirst);

        case PixelFormat::RGB10:
        case PixelFormat::RGB10A8:
            break;
        }

(this is really a minor thing, it's just something I commonly get wrong due to my desire for parallel construction).

[Ryosuke Niwa]

Yeah, allowing else clause in the case multiple conditions are parallel to one another makes sense to me.

But rather than leaving it to author & reviewed to make arbitrary decision, can we codify that?

I'm afraid some people will ignore this and start arguing / writing code without an early return even in the case where an early return and no-else clause is more appropriate because there is no logical parallelism between the two.

[Radar WebKit Bug Importer]

<rdar://problem/78318197>