10000 [CS] [Proposal] Replace calls to `0 === count(array())` with `!array()` · Issue #19182 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[CS] [Proposal] Replace calls to 0 === count(array()) with !array() #19182

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.

Sign up for GitHub

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

Jump to bottom
Closed
phansys opened this issue Jun 25, 2016 · 19 comments
Closed

[CS] [Proposal] Replace calls to 0 === count(array()) with !array() #19182

phansys opened this issue Jun 25, 2016 · 19 comments

Comments

@phansys
Copy link
Contributor
phansys commented Jun 25, 2016

Since empty arrays always evaluates to false, these checks can be simplified.
An example from Symfony\Component\Routing\Annotation\Route::setRequirements():

// current check
if (0 === count($this->methods)) {
    // ...
}

// proposed check
if (!$this->methods) {
    // ...
}

NOTE: I've found 54 exact matches for 0 === count( at 2.7 branch.
@nicolas-grekas
Copy link
Member

Dont forget countable objects

@Simperfit
Copy link
Contributor

I agree when these are pur array since it can be faster.
8000

@phansys
Copy link
Contributor Author
phansys commented Jun 25, 2016

Thanks for pointing that @nicolas-grekas. Are we widely using this notation to allow (now or later) either an array or an instance of Countable for these checked collections, or we can apply this change making the distinction in each replacement?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jun 25, 2016 via email

@phansys
Copy link
Contributor Author
phansys commented Jun 25, 2016

I'll await for more feedback then, but I don't know if this change should be against 2.7 or master.

@nicolas-grekas
Copy link
8000 Member

2.7 if any

@Tobion
Copy link
Contributor
Tobion commented Jun 25, 2016

Is there a performaance difference considering counting is not necessary?

@phansys
Copy link
Contributor Author
phansys commented Jun 25, 2016

I haven't any benchmark @Tobion, but I can do something about that.

@phansys
Copy link
Contributor Author
phansys commented Jun 25, 2016

The results shows that the proposed syntax is much faster than the original: https://3v4l.org/GETTr

@javiereguiluz
Copy link
Member

I like this proposal and @phansys showed that it's 4 or 5 times faster than the existing code. But at the same time I don't like this proposal because the code is way harder to read.

If I see this:

if (0 === count($this->methods)) {    }

I instantly understand it as "if the array has no elements".

But if I see this:

if (!$this->methods) {    }

I understand it as "if this is null", "if this doesn't exist", "if this is false", etc. And "if this has no elements" comes much later.

So, if understanding the code as "this has no elements" is important, this change shouldn't be made.

@ro0NL
Copy link
Contributor
ro0NL commented Jun 26, 2016

Im very 👍 on this, basicly we can write faster. @javiereguiluz with a good object domain and understanding of it this should be not so much a problem, ie i read;

"If the object has no methods available". (With methods being a bad example perhaps, but you get the point).

@jvasseur
Copy link
Contributor

Maybe we could use if ([] === $this->methods) instead, it is faster than the count method (https://3v4l.org/m9CNV) and still convey the meaning "this array is empty".

@ro0NL
Copy link
Contributor
ro0NL commented Jun 26, 2016

It's still boilerplate code, but definitely better than count imo.

8000

@wouterj
Copy link
Member
wouterj commented Jun 26, 2016 •
edited
Loading

👎:

  • The time saved is nearly nothing. I mean even after 100.000 calls, the count one still takes just 0.004 seconds.
  • I agree with @javiereguiluz here. Using count is way more readable and clear. (for instance, I thought !array(null) would be true as well)
  • As illustrated by @nicolas-grekas, this fix wouldn't be as trival as just running PHP CS fixer, as we have to take care of not breaking BC. Also, this means inconsistency is introduced, people will start submitting PRs to change other count usages as well, which we then have to close because of BC breaks, etc. I don't think it's worth it, let's focus on some parts of the app that can reduce way more response time instead :)

@ro0NL ro0NL mentioned this issue Jun 26, 2016
Closed
@ro0NL
Copy link
Contributor
ro0NL commented Jun 26, 2016

Reconsidered this. I think using count is good in a way; it will probably break the code whenever the type changes (due refactoring whatever). Only if this were the case ;-) count($whatever) is still unpredictable for simple types.

Besides that this could lead to counts in loops, and what if the tested array is actually not empty: https://3v4l.org/1sCIf Im not a performance expert, but the gain for the 5.5/5.6 seems significant.

So im 👍 for either array() === $var or !$var. Where the first is perhaps more strict.

@phansys
Copy link
Contributor Author
phansys commented Jun 26, 2016

@wouterj, please note that my proposal isn't about a rule for php-cs-fixer, as the matching cases require attention to decide. What I'm proposing here is to aim developers to avoid a call to a function which isn't really required in order to achieve the expected goal, regardless the semantic perspective as @javiereguiluz pointed.
This means we should be able to write a new item at coding standards docs stating something like "Checks against empty arrays MUST be done using logical not ( ! ) operator".

@wouterj
Copy link
Member
wouterj commented Jun 26, 2016

What I'm proposing here is to aim developers to avoid a call to a function which isn't really required in order to achieve the expected goal, regardless the semantic perspective as @javiereguiluz pointed.

At this is exactly what I don't agree on. I mentioned the PHP CS Fixer to illustrate that this isn't a quick and trival change.

@linaori
Copy link
Contributor
linaori commented Jun 26, 2016

Considering this, I would say stick with count() for the sake of consistency. Notice that generators give a different value in count due to a generator object being returned (which might be a php bug?)

@fabpot
Copy link
Member
fabpot commented Jun 27, 2016

As the new code is less readable and as the current code does not have a huge performance impact, I'm 👎 as well for this change.

On a more general note, when thinking about making similar changes, we should also always think about this bigger impact on Symfony: what value does it bring (value is not just about features, it's also of course about maintainability, consistency, ...)? Is it worth discussing? Is it worth taking the time to do the change, discuss it, merge it, document it, enforce it, ...

@fabpot fabpot closed this as completed Jun 27, 2016
@ro0NL ro0NL mentioned this issue Sep 21, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@fabpot @javiereguiluz @nicolas-grekas @Tobion @wouterj @jvasseur @ro0NL @phansys @linaori @Simperfit
0