improvement: count() now costs O(1) instad of O(n) #385
Conversation
There was a problem hiding this comment.
Does this change actually solve a problem for you? Did you run any benchmarks?
The obvious blocker for me is the fact that the constructor call gets unnecessarily expensive with your change.
Also since the logic behind the count() method gets more complex with your change, we will probably need more tests around the calculation.
That being said, I'm not sure the added complexity is worth it, tbh.
src/ArrayCollection.php
Outdated
| @@ -65,6 +71,7 @@ class ArrayCollection implements Collection, Selectable, Stringable | |||
| public function __construct(array $elements = []) | |||
| { | |||
| $this->elements = $elements; | |||
| $this->size = count($elements); | |||
There was a problem hiding this comment.
This increases the cost of the constructor call from O(1) to O(n).
There was a problem hiding this comment.
only if you create a collection from another collection.
All new collection costs O(1)
There was a problem hiding this comment.
only if you create a collection from another collection.
Which is a totally valid thing to do! This is a blocker!
|
The Static Analysis problem, from PHPstan, wasn't a problem of this PR |
Are you willing to bet? 😛 There's an easy way you could make sure of that, you know 😉 |
| @@ -98,7 +104,7 @@ public function first() | |||
| * @psalm-template K of array-key | |||
| * @psalm-template V | |||
| */ | |||
| protected function createFrom(array $elements) | |||
| protected function createFrom(array $elements): static | |||
There was a problem hiding this comment.
Also, the "changes" view makes it obvious which part of your changes is causing the issue.
| @@ -65,6 +70,7 @@ class ArrayCollection implements Collection, Selectable, Stringable | |||
| public function __construct(array $elements = []) | |||
| { | |||
| $this->elements = $elements; | |||
| $this->size = empty($elements) ? 0 : count($elements); | |||
There was a problem hiding this comment.
You don't need to avoid count() on an empty array. That's a cheap operation. counting non-empty arrays preemptively is what troubles me.
| @@ -98,7 +104,7 @@ public function first() | |||
| * @psalm-template K of array-key | |||
| * @psalm-template V | |||
| */ | |||
| protected function createFrom(array $elements) | |||
| protected function createFrom(array $elements): static | |||
There was a problem hiding this comment.
Adding a native return type is a breaking change. We must not do that in a minor or bugfix release. Apart from that, this change seems unrelated to the goal of this PR.
| @@ -3,4 +3,4 @@ | |||
| /build | |||
| /phpcs.xml | |||
| /.phpcs-cache | |||
| /.phpunit.result.cache | |||
| /.phpunit.result.cache | |||
|
FWIW |
Now the method
count()costs O(1) instad of O(n)