[Webkit-unassigned] [Bug 225817] Proposed change to WebKit Code Style Guidelines for if-return-else-return

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=225817

--- Comment #3 from Sam Weinig <sam at webkit.org> ---
(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).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210514/6fe9bb7d/attachment.htm>