Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.4] Create a mapToGroups method #18949

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Conversation

JosephSilber
Copy link
Member

@JosephSilber JosephSilber commented Apr 26, 2017

$users->mapToGroups(function ($user) {
    return [$user->country => $user->id];
});

/*
    [
        'canada' => [23, 1],
        'france' => [34, 3],
        'germany' => [9],
        'us' => [4, 7, 2],
    ]
*/

@JosephSilber JosephSilber force-pushed the map-to-groups branch 7 times, most recently from 4312073 to 503d585 Compare April 26, 2017 17:56
@thecrypticace
Copy link
Contributor

Seems neat. I have, on occasion, needed to use ->map after a groupBy so this could be a neat addition. I'm not 100% on the name mapToGroups though. Doesn't feel right but I can't think of a better one.

Also, the above example with Higher Order Messages can be written as:

$users->groupBy("country")->map->pluck("id");

Want to see something even crazier? Add a groupBy proxy and you can write it like this:
Warning: Please don't actually do this. Code readability and intent are important

$users->groupBy->country->map->map->map->id;

Aside: keyBy and groupBy both would benefit from being in the default proxy list. Not sure why they weren't included. I know you can add them with Collection::proxy but it still seems like a sensible default.

@JosephSilber
Copy link
Member Author

the above example with Higher Order Messages can be written as

I intentionally created a simple example. But if you do more complex stuff, you may end up with two closures.

Additionally, if the value you're mapping to requires the original key, you're out of luck without this mapToGroups. You'd have to resort back to an ugly foreach.

@thecrypticace
Copy link
Contributor

I intentionally created a simple example. But if you do more complex stuff, you may end up with two closures.

Yeah, I figured as much. Was just pointing it out.

you're out of luck without this mapToGroups

Yeah. What I usually try to do is map first and include an extra entry for grouping if needed. But I always felt that it was a bit of a hack. Just depends on use case.

@ConnorVG
Copy link
Contributor

I feel like this should follow the naming of flatMap and be named groupMap.

@JosephSilber
Copy link
Member Author

JosephSilber commented Apr 27, 2017

@ConnorVG to me, groupMap would mean something else. I'd expect groupMap to work like this:

$collection = collect([
    'a' => [2, 5],
    'b' => [8, 9],
]);

$collection->groupMap(function ($number) {
    return $number * 2;
});

/*
    [
        'a' => [4, 10],
        'b' => [16, 18],
    ]
*/

...which would also maybe be a useful method, but this mapToGroups does something else.

@ConnorVG
Copy link
Contributor

Eh? That's just a map map.

@fernandobandeira
Copy link
Contributor

I think we could add another test for numeric keys:

$users->mapToGroups(function ($user) {
    return [$user->age => $user->id];
});

/*
    [
        17 => [23, 1],
        23 => [34, 3],
        31 => [9],
        44 => [4, 7, 2],
    ]
*/

So we assert that it won't reindex the array in the future...

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