Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recommend using explicit parentheses in expressions that mix binary boolean operators #22

Closed
TimWolla opened this issue Jun 10, 2022 · 3 comments

Comments

@TimWolla
Copy link
Contributor

While the operator precedence in PHP is clearly defined so that && has higher precedence than || and this precedence matches the vast majority of programming languages in use, expressions mixing those two operators without explicit parentheses still require the reader to know the precedence off the top of their head, adding room for error.

The coding style should require (or recommend) explicit parentheses whenever && and || are mixed within an expression to make the intent clear:

if ($foo && $bar || $baz) { } // no
if (($foo && $bar) || $baz) { } // yes
if ($foo && ($bar || $baz)) { } // yes, but different semantics than the "no".

The "Disjunctive Normal Form Types" RFC that is currently under discussion explicitly requires parentheses for types that mix union and intersection, likely for the same reason: Removing any possible ambiguity.

There's an open pull request for PHP_CodeSniffer that performs and highlights expressions where these operators are mixed without parentheses:

squizlabs/PHP_CodeSniffer#3205

Anecdotally running the Sniff from the PR on our code base exposed a non-trivial number of bugs where $foo && ($bar || $baz) was intended, but $foo && $bar || $baz was used, showing that this is an issue that happens in the real world.

@KorvinSzanto
Copy link
Contributor

I think this is definitely a good idea to do especially if you have complex conditionals but I don't really think it's something we want to require in this doc as it's more of a question of best practices than readability in my opinion. And I want to avoid filling this document with too many recommendations about best practices.

@samdark
Copy link
Contributor

samdark commented Jul 17, 2022

I agree with @KorvinSzanto here. It is not really about just formatting.

@samdark samdark closed this as completed Jul 17, 2022
@TimWolla
Copy link
Contributor Author

Fair enough.

@samdark Would you mind closing this issue as “not planned”? I believe this improves the usefulness of the closed issue list. You should be able to do so using the dropdown right next to the “Reopen” button.

@samdark samdark closed this as not planned Won't fix, can't repro, duplicate, stale Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants