New Operations: Compare, Max, Min

[AlexandruGG]

This PR:

• Introduces two operations to compute the minimum and maximum value of a collection, as well as to compare items with a custom comparator
• Is covered by unit tests
• Has static analysis tests (psalm, phpstan)
• Has documentation

[AlexandruGG]

I didn't want to do it in this PR because I wanted it to be simple and just rely on foldLeft1, but I think we should make a dedicated PR before the next major release to change the behaviour of all reduction operations like reduce, foldLeft, foldRight etc.

While in some cases it might be useful to have these operations return a collection, it's much more common for users to expect these to return a single value; after all, that's the typical behaviour of reductions. This is the case in other languages (just a couple examples):

• Java Streams
• Rust std::iter::Iterator::fold

This change would make Collection easier to use and also make it easier to differentiate between some operations, like Reduce (will return a single value) and Reduction (will continue to return a collection).

WDYT @drupol?

[drupol]

I think we should do that before the next major release indeed.

[drupol]

I would like to work on this, we should gather our idea for next major versions, can't wait for it :=)

[AlexandruGG]

yep! I'll create an issue and we can list out the methods we think need this treatment

[drupol]

I have another idea for this which would require a bit of rewriting if you're ok with this.

How about changing the behavior of the optional callback that we can pass to min() or max() ?

The callback would return an numeric value only and then, we compare for min and max internally in the operation, so we don't let the user alter the behavior of these operation through a callback.

Example:

$callback = static fn (object $o): int => $o->age;

$result = Collection::fromIterable([(object) ['id' => 2, 'age' => 5], (object) ['id' => 1, 'age' => 10]])
    ->min($callback)
    ->current(); // (object) ['id' => 2, 'age' => 5]

WDYT ?

[AlexandruGG]

I have another idea for this which would require a bit of rewriting if you're ok with this.

How about changing the behavior of the optional callback that we can pass to min() or max() ?

The callback would return an numeric value only and then, we compare for min and max internally in the operation, so we don't let the user alter the behavior of these operation through a callback.

Example:

$callback = static fn (object $o): int => $o->age;

$result = Collection::fromIterable([(object) ['id' => 2, 'age' => 5], (object) ['id' => 1, 'age' => 10]])
    ->min($callback)
    ->current(); // (object) ['id' => 2, 'age' => 5]

WDYT ?

Hey @drupol, thanks for looking at this.

I like the intent of restricting a bit the callback that can be passed to these operations. However, returning a single value from the callback you propose would not be sufficient because of two reasons I think:

1. I might want to make the comparison on more than one value. This is quite common for objects and unfortunately in PHP we don't have something like the Comparable interface in Java. I can see there was an RFC at some point but wasn't implemented.

Because of this, I might want to do:

$maxCallback = static fn (stdClass $left, stdClass $right): stdClass => $left->age > $right->age && $left->height > $right->height
    ? $left
    : $right
;

2. I might not want to use the max or min functions at all

I think the restriction at the type level should be sufficient, and further than that it can be the responsibility of the user to ensure the correct element is returned from the callback based on their desired effect.

I'm open to other suggestions though!

[drupol]

Ok I understand and it's fine for me. However, I think we can already achieve this using a combination of the Sort and Head operations, no ?

[AlexandruGG]

Ok I understand and it's fine for me. However, I think we can already achieve this using a combination of the Sort and Head operations, no ?

Sure, I think that would work. But are you saying that these operations should be based on those ones instead of the fold, or that there isn't a need for min and max to exist?

[drupol]

Hi Alex,

min and max would definitely be interesting to have. Especially that the implementation you made is mostly lazy.

FYI, the implementation of the Sort operation is not lazy and requires the iterator to be dumped into an ArrayIterator before calling the sort method(it's very ugly yeah !). This is something I would like to refactor to a more general operation "compare" that would be based on basically what you did in min and max.

Maybe this is something within this PR, adding a compare operation, and then use some kind of new operations like min and max using this new compare operation ? This way we can also get rid of code duplication and have 3 new operations !

WDYT ?

[AlexandruGG]

Hi Alex,

min and max would definitely be interesting to have. Especially that the implementation you made is mostly lazy.

FYI, the implementation of the Sort operation is not lazy and requires the iterator to be dumped into an ArrayIterator before calling the sort method(it's very ugly yeah !). This is something I would like to refactor to a more general operation "compare" that would be based on basically what you did in min and max.

Maybe this is something within this PR, adding a compare operation, and then use some kind of new operations like min and max using this new compare operation ? This way we can also get rid of code duplication and have 3 new operations !

WDYT ?

Hey @drupol,

Thanks for the clarifications.

It's not entirely clear to me how Sort would be refactored, so I would leave that to you, but regarding the other stuff are you thinking something like this?

$col = Collection::fromIterable([1, 2, 3]);

$callback = static fn ($left, $right) => $left > $right ? $right : $left;
$result = $col->compare($callback)->current(); // 3

$max = $col->max(); // internally uses `compare` + "max callback" -> 3
$min = $col->min(); // internally uses `compare` + "min callback" -> 1

If this is what you were thinking, should compare have a "default callback"? I'm not sure if one would be appropriate, so we could make the callback required for that operation - but at least min and max will be more standard and non-customizable.

[drupol]

No worries for the sort operation, I'll deal with it in time.

For the rest, yes that's precisely that. WDYT about the idea? Would you be ok with that?

We could then even think to do that:

$obj1 = new StdClass;
$obj2 = new StdClass;
$obj1->weight = 1;
$obj2->weight = 2;
$col = Collection::fromIterable([$obj1, $obj2]);

$accessor = static fn (object $left): int => $left->weight; // Naming might be improved.
$callback = static fn ($left, $right) => $left > $right ? $right : $left;
$result = $col->compare($callback, $accessor)->current(); // 3

$max = $col->max($accessor); // internally uses `compare` + "max callback" -> 3
$min = $col->min($accessor); // internally uses `compare` + "min callback" -> 1

[AlexandruGG]

No worries for the sort operation, I'll deal with it in time.

For the rest, yes that's precisely that. WDYT about the idea? Would you be ok with that?

We could then even think to do that:

$obj1 = new StdClass;
$obj2 = new StdClass;
$obj1->weight = 1;
$obj2->weight = 2;
$col = Collection::fromIterable([$obj1, $obj2]);

$accessor = static fn (object $left): int => $left->weight; // Naming might be improved.
$callback = static fn ($left, $right) => $left > $right ? $right : $left;
$result = $col->compare($callback, $accessor)->current(); // 3

$max = $col->max($accessor); // internally uses `compare` + "max callback" -> 3
$min = $col->min($accessor); // internally uses `compare` + "min callback" -> 1

Yes, that looks good.

I'll work to implement those in this PR once I get the chance.

[AlexandruGG]

No worries for the sort operation, I'll deal with it in time.

For the rest, yes that's precisely that. WDYT about the idea? Would you be ok with that?

We could then even think to do that:

$obj1 = new StdClass;
$obj2 = new StdClass;
$obj1->weight = 1;
$obj2->weight = 2;
$col = Collection::fromIterable([$obj1, $obj2]);

$accessor = static fn (object $left): int => $left->weight; // Naming might be improved.
$callback = static fn ($left, $right) => $left > $right ? $right : $left;
$result = $col->compare($callback, $accessor)->current(); // 3

$max = $col->max($accessor); // internally uses `compare` + "max callback" -> 3
$min = $col->min($accessor); // internally uses `compare` + "min callback" -> 1

I think I didn't look at this properly the first time - it seems that in this scenario these operations are doing a hidden "map" and changing the collection from a list of stdClass to int via the accessor. I'm not in favour of this approach, it makes this more complex and harder to understand.

I'll implement a simpler version like the one proposed here.

[drupol]

I think I didn't look at this properly the first time - it seems that in this scenario these operations are doing a hidden "map" and changing the collection from a list of stdClass to int via the accessor. I'm not in favour of this approach, it makes this more complex and harder to understand.

That's precisely that! What is wrong with that?

I'll implement a simpler version like the one proposed here.

As you wish :)

[AlexandruGG]

I think I didn't look at this properly the first time - it seems that in this scenario these operations are doing a hidden "map" and changing the collection from a list of stdClass to int via the accessor. I'm not in favour of this approach, it makes this more complex and harder to understand.

That's precisely that! What is wrong with that?

I think it makes the API and the typing more complex...it extends the range of things these comparison operations do, whereas it would be nicer to have a separation of responsibilities. I wouldn't expect a comparison operation to also change the type.

In any case the additions would be backwards compatible so we can always extend them if there's demand.

[drupol]

I think it makes the API and the typing more complex...it extends the range of things these comparison operations do, whereas it would be nicer to have a separation of responsibilities. I wouldn't expect a comparison operation to also change the type.

It doesn't change it in the collection. It changes it locally so it can run the callback, but the original collection items stays unchanged.

Don't forget that most of the operations are lazy, so, applying a map or doing a local transformation is less or more the same in the end.

[AlexandruGG]

I think it makes the API and the typing more complex...it extends the range of things these comparison operations do, whereas it would be nicer to have a separation of responsibilities. I wouldn't expect a comparison operation to also change the type.

It doesn't change it in the collection. It changes it locally so it can run the callback, but the original collection items stays unchanged.

Don't forget that most of the operations are lazy, so, applying a map or doing a local transformation is less or more the same in the end.

I know it doesn't change the original collection and that the operations are lazy, that's not the issue. My concern was that the added complexity is not worth it in this case. Each operation can have it's own role - in this case compare effectively only selects from the original collection, doesn't need to do any further transformation.

[drupol]

I know it doesn't change the original collection and that the operations are lazy, that's not the issue. My concern was that the added complexity is not worth it in this case. Each operation can have it's own role - in this case compare effectively only selects from the original collection, doesn't need to do any further transformation.

I fully agree !

[AlexandruGG]

This should be ready for review again, let me know if you're happy with this version! I think it's a nice addition for now, we can tweak things further later if needed :)

[drupol]

This is indeed a very nice addition as usual :) I will carefully review this in the evening and let you know asap.

Thanks !!!

[drupol]

Nothing to add here ! all good :) Ready to merge !

[drupol]

💯

[AlexandruGG]

Thank you! 😁

[drupol]

Let's create issues for the remaining tasks we discussed in here !