225817 2021-05-14 10:54:57 -0700 Proposed change to WebKit Code Style Guidelines for if-return-else-return 2021-05-21 10:55:29 -0700 1 1 1 Unclassified WebKit New Bugs Other Unspecified Unspecified NEW InRadar P2 Normal --- 1 sam webkit-unassigned cdumez darin rniwa webkit-bug-importer oldest_to_newest 1760142 0 sam 2021-05-14 10:54:57 -0700 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? 1760146 1 cdumez 2021-05-14 11:04:51 -0700 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). 1760167 2 darin 2021-05-14 12:18:00 -0700 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. 1760201 3 sam 2021-05-14 14:05:30 -0700 (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). 1760238 4 rniwa 2021-05-14 15:06:05 -0700 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. 1762489 5 webkit-bug-importer 2021-05-21 10:55:29 -0700 <rdar://problem/78318197>