Add function where() #135
Add function where() #135
Conversation
|
I am contemplating the priority of properties over methods right now. From a puristic perspective I think it’s the right thing, from a "this is how PHP code looks nowadays mostly" I feel we should test for methods first, than for properties. WDYT? |
| * @param array $propertyMap | ||
| * @return bool | ||
| */ | ||
| $matches = function($object, array $propertyMap) use ($readProperty, &$matches) { |
There was a problem hiding this comment.
Any reason this should be a instead of just code inside of the foreach loop?
There was a problem hiding this comment.
If I inline it, the body of where() would look like this:
InvalidArgumentException::assertCollection($objects, __FUNCTION__, 1);
$result = [];
foreach ($objects as $obj)
{
foreach ($propertyMap as $property => $value) {
if (is_array($value)) {
if (!$matches($readProperty($object, $property), $value)) {
continue 2;
} else {
continue;
}
}
try {
// weak equality check to compare different instances of objects
if ($readProperty($object, $property) != $value) {
continue 2;
}
} catch (\OutOfBoundsException $outOfBoundsException) {
continue 2;
}
}
$result[] = $obj;
}
return $result;
I think it is unclear what is happening in this code. If one of my colleages commited this it would raise an eyebrow.
There was a problem hiding this comment.
I agree, not pretty but for a library like this where we try add as little overhead as possible, every function call counts
There was a problem hiding this comment.
I'll run a benchmark to see what the performance difference is. The function call may not have that much of an inpact due to the pointer chaising and reflection that we have regardless.
There was a problem hiding this comment.
Unfortunately it indeed makes a pretty big difference, around 50% slowdown in the worst-case.
There was a problem hiding this comment.
Doing the function call is worse or inlining the code?
There was a problem hiding this comment.
Doing the function call is 50% slower in the worst case, meaning when the shortest path is taken through the code so function call overhead is maximized
There was a problem hiding this comment.
Yep, that was my intuition but good that we verified it
|
W.r.t. priority of properties and methods I think you have a point that methods should take precedence. It would prevent some access violations. There are 2 real-world situations that I can think of where this occurs.
I'll switch it around to methods take precedence. |
|
Any news? |
| throw new \OutOfBoundsException; | ||
| } | ||
| } else { | ||
| throw new \InvalidArgumentException('Expected object or array'); |
There was a problem hiding this comment.
"Expected …, got …" would help a lot while debugging
| if (array_key_exists($property, $object)) { | ||
| return $object[$property]; | ||
| } else { | ||
| throw new \OutOfBoundsException; |
| if (is_object($object)) { | ||
| if (method_exists($object, $property)) { | ||
| return $object->{$property}(); | ||
| } elseif (property_exists($object, $property)) { |
There was a problem hiding this comment.
property_exists() will return true for private properties. in_array() + get_class_vars() should work though.
There was a problem hiding this comment.
Isn't that a usefull error though "can't access private property"?
There was a problem hiding this comment.
Principle of the least surprise for me would mean for it to behave like a a non-existent property. Imagine you are using where() on a list and you catch the InvalidArgumentException because you foresee some property being missing. Now you go ahead and change the class that is used somewhere in you application and introduce a private property and suddenly you have a different error case. I would handle it consistently with the "non-existent property case".
|
I don't want to do more inlining. That's why I've let go of the PR. |
| if (is_object($object)) { | ||
| if (method_exists($object, $property)) { | ||
| return $object->{$property}(); | ||
| } elseif (property_exists($object, $property)) { |
There was a problem hiding this comment.
Principle of the least surprise for me would mean for it to behave like a a non-existent property. Imagine you are using where() on a list and you catch the InvalidArgumentException because you foresee some property being missing. Now you go ahead and change the class that is used somewhere in you application and introduce a private property and suddenly you have a different error case. I would handle it consistently with the "non-existent property case".
| Multiple properties can be matched for. They must all match. | ||
|
|
||
| ```php | ||
| $fredsFromGithub = where($users, [ |
There was a problem hiding this comment.
Being German this must totally $fredFromJupiter
| ```php | ||
| $fredsFromGithub = where($users, [ | ||
| 'name' => 'Fred', | ||
| 'organization' => 'GitHub' |
|
Closing for inactivity |
Resubmit of #122 with some feedback processed