loophp / collection

A (memory) friendly, easy, lazy and modular collection class.
https://loophp-collection.rtfd.io/
MIT License
721 stars 35 forks source link

Reduction operations should return a single value #256

Closed AlexandruGG closed 2 years ago

AlexandruGG commented 2 years ago

Context

As described here, user experience would be improved if reduction operations would return a value directly, rather than a collection.

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):

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).

Steps required to reproduce the problem

$result = Collection::fromIterable([1, 4, 3, 0, 2])->min();

Expected Result

// 0

Actual Result

// Collection([4 => 4])

Operations

AlexandruGG commented 2 years ago

@drupol let me know if you can think of other operations that we might want to include in this.

Even though they are not reduction operations, it feels like a good opportunity to revisit: first, last, head, get. From the usage you've seen, are they actually used in instances other than calling current immediately after? E.g. are they used to apply further transformations to a single element?

drupol commented 2 years ago

Hi,

I think I know why I chose to do that. It's because a "reducing" operation such as those listed in here are also able to, instead of "reducing" the input data into one single value, "expand" the input data into a bigger set.

AlexandruGG commented 2 years ago

Hi,

I think I know why I chose to do that. It's because a "reducing" operation such as those listed in here are also able to, instead of "reducing" the input data into one single value, "expand" the input data into a bigger set.

Hey, could you pls give some examples of this?

drupol commented 2 years ago
<?php

namespace Test;

use loophp\collection\Collection;

include __DIR__ . '/vendor/autoload.php';

$input = [
    'a' => [
        'b', 'c'
    ],
    'd' => [
        'e', 'f', 'g'
    ],
];

$c = Collection::fromIterable($input)
    ->foldLeft(
        static function (array $carry, array $value, string $key) {
            $carry[] = $key;
            $carry = array_merge($carry, $value);

            return $carry;
        },
        []
    );
AlexandruGG commented 2 years ago

I see, I feel like this is only a thing because in PHP we can use arrays as both lists and maps 😄. You'd have some type restrictions in other languages that might prevent doing this.

Anyway, I see a few options to move forward here:

  1. Ignore this usage and make all reductions return a single value - the simplest case, might be a bit inconvenient for the usage you described above; would be interesting to know if this usage is common

  2. Pick and choose which operations to modify - for example, we could modify only compare, min, max and leave the other one as they are - I'm not the biggest fan of this because it makes things a bit inconsistent

  3. Have some logic to determine whether to keep returning a Collection or not - for example, we could check the $carry parameter: if it's an array, we keep things in a collection. If not, return a single value

None of these are perfect, they all have some compromises. Can you think of any others? Or which one do you find more appealing?

drupol commented 2 years ago

I have to think about that.

I would definitely go for the first option. Indeed, if the intended result is to expand the input set, flatMap should be used:

$c = Collection::fromIterable($input)
    ->flatMap(
        static function (array $value, string $key) {
            yield $key;
            yield from $value;
        }
    );
AlexandruGG commented 2 years ago

Found a few more that could be included in this:

The purpose of these is to always result in a single value

ipranjal commented 2 years ago

+1 on this, I did have few workaround in my projects but having this in core would be awesome

drupol commented 2 years ago

I have to work on it... Expect some changes during the weekend.