Skip to content

Conversation

@phanan
Copy link
Contributor

@phanan phanan commented Mar 20, 2015

I know @taylorotwell said he tends to "shy away" from these "cleanup PRs," but can't help it.

@arrilot
Copy link
Contributor

arrilot commented Mar 20, 2015

Using count() inside for is an anti-pattern because it executes on every iteration and leads to performance degradation on large arrays

@phanan
Copy link
Contributor Author

phanan commented Mar 20, 2015

@arrilot No.

@laurencei
Copy link
Contributor

So out of curiosity - I was wondering how much effect using count() on the loop had based upon @arrilot comments

For a 100,000 array:

  • Using $tokenCount ran the 100,000 loop in 0.005 secs
  • Using count($array) ran the 100,000 loop in 0.05 secs

So it is an order of magnitude faster to precalculate the array size. However that is only for an extremely large array. Arrays less than 1,000 had no noticeable runtime difference.

Note - I dont support or deny the PR one way or the other - was just curious and thought I'd post my results here "for science"

@andrewhood125
Copy link
Contributor

👎

@GrahamCampbell
Copy link
Collaborator

@phanan, unfortunately, you are not correct.

@phanan
Copy link
Contributor Author

phanan commented Mar 20, 2015

@GrahamCampbell No worries, I've been wrong numerous times in my life 😄 But which statement of mine is incorrect this time?

@theshiftexchange and @arrilot According to the for loop documentation:

The first expression (expr1) is evaluated (executed) once unconditionally at the beginning of the loop.

Which means the call to count($array) is evaluated only once. Try for yourself:

<?php

function called() {
    echo 'Called';
    return 10;
}

for ($i = 0, $j = called(); $i < $j; ++$i) {
    echo $i;
}

// output: Called0123456789

@GrahamCampbell
Copy link
Collaborator

Ohhh, @phanan you are actually correct. I apologise. I misread the diff as having the function call after the semicolon, rather than before it, in the bit where we initialise the iteration.

@GrahamCampbell
Copy link
Collaborator

@theshiftexchange No, this isn't executed every time. If it were, then $i would get initialised on every iteration, which of course isn't what happens.

@laurencei
Copy link
Contributor

Yep - in my test I didnt do the var initizilation the way @phanan did it.

His way is correct and produces same run times for the array

@GrahamCampbell
Copy link
Collaborator

Even then, this change is totally pointless. We could put every single variable in that bit of the for statement if we wanted to, but why would we? The current way is actually more readable.

@GrahamCampbell
Copy link
Collaborator

And the performance is identical.

@GrahamCampbell
Copy link
Collaborator

You say "removed", but actually, you've basically just renamed it, and moved it a little bit lower in the code...

@phanan
Copy link
Contributor Author

phanan commented Mar 20, 2015

@GrahamCampbell Agreed. I tend to be too nitpicking time to time...

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

Successfully merging this pull request may close these issues.

5 participants