Bug 63564 - coding-style: Add note about cases where enums are preferred over bools
Summary: coding-style: Add note about cases where enums are preferred over bools
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Pereira
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-28 14:36 PDT by Leandro Pereira
Modified: 2011-06-29 10:57 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.62 KB, patch)
2011-06-28 15:23 PDT, Leandro Pereira
no flags Details | Formatted Diff | Diff
Patch (1.77 KB, patch)
2011-06-29 07:01 PDT, Leandro Pereira
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2011-06-29 09:52 PDT, Leandro Pereira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Pereira 2011-06-28 14:36:08 PDT
coding-style: Add note about cases where enums are preferred over bools
Comment 1 Leandro Pereira 2011-06-28 15:23:19 PDT
Created attachment 98979 [details]
Patch
Comment 2 Darin Adler 2011-06-28 17:15:32 PDT
Comment on attachment 98979 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98979&action=review

> Websites/webkit.org/coding/coding-style.html:606
> +<li>On functions with more than one parameter, prefer enums to bools if callers are likely to be passing constants.

This rule applies to functions with one parameter too. The number of parameters is not the key.

Maybe the reason you though it depended on the number of parameters is that it does not apply to a setXXX function where the name of the function makes clear what the boolean is.

I also think the guideline should explain the reason. The code below does allude to the reason this matters, but it’s worth saying that the named constants are much easier to read at the call site.
Comment 3 Darin Adler 2011-06-28 17:16:08 PDT
Comment on attachment 98979 [details]
Patch

review- because I’d like to remove the mistaken “more than one parameter”
Comment 4 Leandro Pereira 2011-06-29 07:01:34 PDT
Created attachment 99083 [details]
Patch
Comment 5 Darin Adler 2011-06-29 08:19:50 PDT
Comment on attachment 99083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99083&action=review

Thanks. Getting better.

> Websites/webkit.org/coding/coding-style.html:607
> +passing constants, since named constants are easier to read on the call site. 

Should be "at the call site" rather than "on the call site".

> Websites/webkit.org/coding/coding-style.html:608
> +The only exception would be on setters, where the name of the function

Instead of "The only exception would be on setters," I would say, "An exception to this rule is a setter function,"

> Websites/webkit.org/coding/coding-style.html:613
> +doSomething(something, AllowFooBar);
> +paintTextWithShadows(context, ..., textStrokeWidth > 0, isHorizontal());

The right example should include a setter that takes a boolean.

> Websites/webkit.org/coding/coding-style.html:618
> +doSomething(something, false);

The wrong example should include a setter that takes an enum.
Comment 6 Leandro Pereira 2011-06-29 09:52:52 PDT
Created attachment 99107 [details]
Patch
Comment 7 Leandro Pereira 2011-06-29 09:54:19 PDT
(In reply to comment #5)
> 
> Should be "at the call site" rather than "on the call site".
> Instead of "The only exception would be on setters," I would say, "An exception to this rule is a setter function,"
> The right example should include a setter that takes a boolean.
> The wrong example should include a setter that takes an enum.
>

Fixed everything. Thanks.
Comment 8 WebKit Review Bot 2011-06-29 10:55:42 PDT
The commit-queue encountered the following flaky tests while processing attachment 99107 [details]:

http/tests/media/reload-after-dialog.html bug 59901 (authors: annacc@chromium.org, eric.carlson@apple.com, jamesr@chromium.org, and vrk@chromium.org)
The commit-queue is continuing to process your patch.
Comment 9 WebKit Review Bot 2011-06-29 10:57:13 PDT
Comment on attachment 99107 [details]
Patch

Clearing flags on attachment: 99107

Committed r90027: <http://trac.webkit.org/changeset/90027>
Comment 10 WebKit Review Bot 2011-06-29 10:57:18 PDT
All reviewed patches have been landed.  Closing bug.