Implement Iterate.toMultimap() #37

nikhilnanivadekar · 2016-02-04T03:16:20Z

Adding toMultimap() on RichIterable is a major version change. It can be added in the static utility for a minor version.
There is a new MultimapKeyValuePutProcedure created, if this will be a major version change, I can inline the call for minor version change, and replace with the Procedure in major version.
I have only added the targetMultimap variant of toMultimap(), this is because I want users to control the output instead of deciding which type of Multimap is returned for a particular Iterable.

itohro · 2016-02-04T09:11:18Z

It looks good besides the unused import above. I believe adding a new procedure class is ok as minor changes (it doesn't force existing clients to use the new API).

Just one thought after looking at the new API. Would it be also helpful if we have an additional API like below (we may want to have a better name)? The only difference is that value function is expected to return collection of values, and we can eventually call putAll() when putting values to a multimap.

public static <T, K, V> MutableMultimap<K, V> toFlatValueMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, Iterable<? extends V>> valueFunction,
        MutableMultimap<K, V> targetMultimap)

This doesn't need to be in this commit and can be taken into another issue.

motlin · 2016-02-04T13:55:53Z

To be consistent with groupBy() and groupByEach(), I think that this functionality is the one that should be renamed and that the alternate type signature suggested by @itohro should get the name toMultimap(). What do you think?

itohro · 2016-02-04T13:59:19Z

+1 on @motlin 's suggestion.

nikhilnanivadekar · 2016-02-04T23:59:15Z

@motlin , @itohro What will be the names of two methods? Adding all the methods which need the name to be finalized:

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends V> valueFunction,
        MutableMultimap<K, V> targetMultimap)

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, Iterable<? extends V>> valueFunction,
        MutableMultimap<K, V> targetMultimap)

Also, we need to add two more methods, to cover all the patterns:

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, Iterable<? extends K>> keyFunction,
        Function<? super T, ? extends V> valueFunction,
        MutableMultimap<K, V> targetMultimap)

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, Iterable<? extends K>> keyFunction,
        Function<? super T, Iterable<? extends V>> valueFunction,
        MutableMultimap<K, V> targetMultimap)

itohro · 2016-02-05T04:15:58Z

Thinking further on naming consistency with groupBy(), groupByEach(), collect() and flatCollect(). How do the names below sound?

Note: I fixed generics signature for Iterable from Iterable<? extends V> to ? extends Iterable<V>.

public static <T, K, V> MutableMultimap<K, V> collectGroupBy(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends V> valueFunction,
        MutableMultimap<K, V> targetMultimap)

public static <T, K, V> MutableMultimap<K, V> flatCollectGroupBy(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends Iterable<V>> valueFunction,
        MutableMultimap<K, V> targetMultimap)

public static <T, K, V> MutableMultimap<K, V> collectGroupByEach(
        Iterable<T> iterable,
        Function<? super T, ? extends Iterable<V>> keyFunction,
        Function<? super T, ? extends V> valueFunction,
        MutableMultimap<K, V> targetMultimap)

public static <T, K, V> MutableMultimap<K, V> flatCollectGroupByEach(
        Iterable<T> iterable,
        Function<? super T, ? extends Iterable<V>> keyFunction,
        Function<? super T, ? extends Iterable<V>> valueFunction,
        MutableMultimap<K, V> targetMultimap)

itohro · 2016-02-05T06:51:41Z

And if we go with the method names above, we might want to change the name and order of functions as collectFunction (from valueFunction) and groupByFunction (from keyFunction).

motlin · 2016-02-05T13:33:28Z

Do you think all four variants are useful? Or is this for completeness?

nikhilnanivadekar · 2016-02-05T17:08:45Z

@itohro I would want to have toMultimap() in the method names, this is because for consistency with other iteration patterns of toList(),toSet(), toMap(), toBag().

@motlin it is more for completeness. I think the most useful one to have will be:

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends V> valueFunction,
        MutableMultimap<K, V> targetMultimap)

itohro · 2016-02-08T01:07:48Z

Putting aside the naming for a moment, I could think of the 3 APIs below are equally useful. Taking company kata domain as an example.

Get customer names per city:
collectGroupBy => from customers, get a multimap with key=city and value=names
Get ordered items per city:
flatCollectGroupBy => from customers, get a multimap with key=city and value=items
Get cities per ordered item:
collectGroupByEach => from customers, get a multimap with key=item and value=cities

There might be some cases for flatCollectGroupByEach as well, but it might not be as common as other 3 variants.

Coming back to the naming, I agree we'd want to have an API named toMultimap() because that's where people may look up first when thinking about the use cases above. Following @motlin 's original suggestion, how about naming them as below? We could add references to other variants each other in Javadoc for better discoverability.

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends Iterable<V>> valueFunction,
        MutableMultimap<K, V> targetMultimap)

public static <T, K, V> MutableMultimap<K, V> collectGroupBy(
        Iterable<T> iterable,
        Function<? super T, ? extends K> groupByFunction,
        Function<? super T, ? extends V> collectFunction,
        MutableMultimap<K, V> targetMultimap)

public static <T, K, V> MutableMultimap<K, V> collectGroupByEach(
        Iterable<T> iterable,
        Function<? super T, ? extends Iterable<V>> groupByEachFunction,
        Function<? super T, ? extends V> collectFunction,
        MutableMultimap<K, V> targetMultimap)

I'm not keen on adding the 4th one, but we could name it below if we want to.

public static <T, K, V> MutableMultimap<K, V> toMultimapByEach(
        Iterable<T> iterable,
        Function<? super T, ? extends Iterable<V>> keyFunction,
        Function<? super T, ? extends Iterable<V>> valueFunction,
        MutableMultimap<K, V> targetMultimap)

motlin · 2016-02-08T17:49:23Z

Some more thoughts on naming. Two of these patterns have workarounds already. They are the ones where you can call groupBy() or groupByEach() followed by collectValues(). Since they have a workaround, I'm not convinced we should add them at all. I thought about making these methods overloads since they have a different number of parameters than groupBy() and groupByEach(). But it would interfere with a form of groupBy() that groups multiple levels, in other words, two Functions from T to K1 and K2. So I think the names suggested by @itohro are good, but I'd flip them so that they sort alphabetically with groupBy() instead of collect().

public static <T, K, V> MutableMultimap<K, V> groupByCollect(
        Iterable<T> iterable,
        Function<? super T, ? extends K> groupByFunction,
        Function<? super T, ? extends V> collectFunction)

public static <T, K, V> MutableMultimap<K, V> groupByEachCollect(
        Iterable<T> iterable,
        Function<? super T, ? extends Iterable<K>> groupByEachFunction,
        Function<? super T, ? extends V> collectFunction)

But like I said, maybe it would be better not to add them. The methods I've found myself needing without great workarounds are these two.

public static <T, V> MutableMultimap<T, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends Iterable<V>> valueFunction)

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends Iterable<V>> valueFunction)

nikhilnanivadekar · 2016-02-08T20:01:58Z

I agree with @motlin the most common use cases I have come across were for these, also these methods are closer to the toMap() patterns which are present.

public static <T, V> MutableMultimap<T, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends Iterable<V>> valueFunction)

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends Iterable<V>> valueFunction)

I will add these two on Iterate for now, and add them on RichIterable for the next major release.

itohro · 2016-02-09T00:38:20Z

The plan sounds good to me!

nikhilnanivadekar · 2016-02-09T04:47:03Z

On second thoughts, if I add this:

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends Iterable<V>> valueFunction)

the other variant:

public static <T, V> MutableMultimap<T, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends Iterable<V>> valueFunction)

gets a workaround, because now the keyFunction can just be each -> each or Functions.identity()

However, I would like to add groupByCollect(), since I need this frequently and the workaround is to call groupBy() first and then collectValues() basically two iterations.
This is a viable option once we have lazy implementations Multimaps.

public static <T, K, V> MutableMultimap<K, V> groupByCollect(
        Iterable<T> iterable,
        Function<? super T, ? extends K> groupByFunction,
        Function<? super T, ? extends V> collectFunction)

itohro · 2016-02-09T17:50:59Z

Why don't we add groupByEachCollect() as well then? As I mentioned above, the 3 APIs are equally useful and I'd add it, too, if we were to add groupByCollect().

One minor suggestion in Javadoc. Let's add @see tag to refer to other method variants each other. They get different names but they're very similar by nature. For example, users might lookup toMultimap() first when he/she need is actually groupByCollect() (as you did @nikhilnanivadekar , and I would have done it, too :) ). Having @see tag is useful for better discoverability.

nikhilnanivadekar · 2016-02-09T18:04:02Z

In that case, I will split all the three as 3 separate commits under 1 pull request, or do you want 3 different pull requests.
@itohro, @motlin what do you think?

itohro · 2016-02-09T18:06:33Z

I'm good with this => "3 separate commits under 1 pull request".

But let's wait for @motlin 's feedback.

nikhilnanivadekar · 2016-02-09T18:23:55Z

Apart from adding groupByEachCollect() can you please review the code for toMultimap() and groupByCollect(), if there is feedback, I can fix it in one commit itself.

motlin · 2016-02-09T18:56:42Z

There's one reason you might want to split the pull request. I think adding toMultimap() won't require much discussion and can be pulled right away.

public static <T, K, V> MutableMultimap<K, V> toMultimap(
        Iterable<T> iterable,
        Function<? super T, ? extends K> keyFunction,
        Function<? super T, ? extends Iterable<V>> valueFunction)

Maybe even the variant without a keyFunction can be pulled right away. For the other variants, we probably should discuss naming and get feedback from more people, including @donraab.

nikhilnanivadekar · 2016-02-09T19:01:32Z

But do we require a variant without a keyFunction, will it not be using Functions.identity() or each -> each.
Or do you want to add it for completeness?

motlin · 2016-02-09T19:08:25Z

No it doesn't seem necessary. Seems nice to have.
On Tue, Feb 9, 2016 at 2:01 PM Nikhil Nanivadekar notifications@github.com
wrote:

But do we require a variant without a keyFunction, will it not be using
Functions.identity() or each -> each.
Or do you want to add it for completeness?

—
Reply to this email directly or view it on GitHub
#37 (comment)
.

motlin · 2016-02-11T16:24:48Z

eclipse-collections/src/main/java/org/eclipse/collections/impl/utility/Iterate.java

@@ -3258,6 +3301,45 @@ public static boolean contains(Iterable<?> iterable, Object value)
    }

    /**
+     * Iterate over the specified collection applying the specified Functions to each element to calculate
+     * a key and value, add the results to {@code targetMultimap} and return the {@code targetMultimap}.
+     * <p>


a key and several values (rather than one value)

itohro · 2016-02-18T10:03:23Z

If you could move other commits (69c0a6c and onwards) to a separate branch, we can merge 7f77c47 right away in this PR. The changes in 7f77c4 looks good to me.

As @motlin suggested, let's discuss with @donraab in regards to the necessity and naming of the other variants.

motlin · 2016-02-19T00:48:29Z

eclipse-collections/src/main/java/org/eclipse/collections/impl/utility/Iterate.java

@@ -3118,7 +3163,11 @@ public static boolean contains(Iterable<?> iterable, Object value)
    }

    /**
+     * Also see {@link Iterate#groupByEach(Iterable, Function, MutableMultimap)}, {@link Iterate#toMultimap(Iterable, Function, Function, MutableMultimap)}.


This one Javadoc comment using "Also see" and Javadoc links seems inconsistent with the rest.

Signed-off-by: Nikhil Nanivadekar <nikhil.nanivadekar@gs.com>

motlin · 2016-02-20T20:36:28Z

👍

Implement Iterate.toMultimap()

nikhilnanivadekar force-pushed the toMultimap branch 2 times, most recently from fc2421c to bc7c316 Compare February 4, 2016 03:31

motlin added the minor-version label Feb 8, 2016

nikhilnanivadekar force-pushed the toMultimap branch 4 times, most recently from cdd7ca4 to d45d0e9 Compare February 9, 2016 07:07

This was referenced Feb 9, 2016

Implement RichIterable.toMultimap(). #46

Open

Implement RichIterable.groupByAndCollect(). #47

Closed

nikhilnanivadekar mentioned this pull request Feb 9, 2016

Implement RichIterable.groupByEachAndCollect(). #48

Open

motlin reviewed Feb 11, 2016
View reviewed changes

nikhilnanivadekar force-pushed the toMultimap branch 3 times, most recently from 031191e to 2747457 Compare February 18, 2016 03:06

nikhilnanivadekar force-pushed the toMultimap branch 2 times, most recently from 7f77c47 to 05e5c38 Compare February 18, 2016 17:41

motlin reviewed Feb 19, 2016
View reviewed changes

nikhilnanivadekar force-pushed the toMultimap branch 3 times, most recently from 45f8329 to 6df029a Compare February 19, 2016 20:49

Implement Iterate.toMultimap().

c886e26

Signed-off-by: Nikhil Nanivadekar <nikhil.nanivadekar@gs.com>

nikhilnanivadekar force-pushed the toMultimap branch from 6df029a to c886e26 Compare February 20, 2016 19:34

motlin added a commit that referenced this pull request Feb 20, 2016

Merge pull request #37 from nikhilnanivadekar/toMultimap

f59428e

Implement Iterate.toMultimap()

motlin merged commit f59428e into eclipse:master Feb 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Iterate.toMultimap() #37

Implement Iterate.toMultimap() #37

nikhilnanivadekar commented Feb 4, 2016

itohro commented Feb 4, 2016

motlin commented Feb 4, 2016

itohro commented Feb 4, 2016

nikhilnanivadekar commented Feb 4, 2016

itohro commented Feb 5, 2016

itohro commented Feb 5, 2016

motlin commented Feb 5, 2016

nikhilnanivadekar commented Feb 5, 2016

itohro commented Feb 8, 2016

motlin commented Feb 8, 2016

nikhilnanivadekar commented Feb 8, 2016

itohro commented Feb 9, 2016

nikhilnanivadekar commented Feb 9, 2016

itohro commented Feb 9, 2016

nikhilnanivadekar commented Feb 9, 2016

itohro commented Feb 9, 2016

nikhilnanivadekar commented Feb 9, 2016

motlin commented Feb 9, 2016

nikhilnanivadekar commented Feb 9, 2016

motlin commented Feb 9, 2016

motlin Feb 11, 2016

itohro commented Feb 18, 2016

motlin Feb 19, 2016

motlin commented Feb 20, 2016

Implement Iterate.toMultimap() #37

Implement Iterate.toMultimap() #37

Conversation

nikhilnanivadekar commented Feb 4, 2016

itohro commented Feb 4, 2016

motlin commented Feb 4, 2016

itohro commented Feb 4, 2016

nikhilnanivadekar commented Feb 4, 2016

itohro commented Feb 5, 2016

itohro commented Feb 5, 2016

motlin commented Feb 5, 2016

nikhilnanivadekar commented Feb 5, 2016

itohro commented Feb 8, 2016

motlin commented Feb 8, 2016

nikhilnanivadekar commented Feb 8, 2016

itohro commented Feb 9, 2016

nikhilnanivadekar commented Feb 9, 2016

itohro commented Feb 9, 2016

nikhilnanivadekar commented Feb 9, 2016

itohro commented Feb 9, 2016

nikhilnanivadekar commented Feb 9, 2016

motlin commented Feb 9, 2016

nikhilnanivadekar commented Feb 9, 2016

motlin commented Feb 9, 2016

motlin Feb 11, 2016

Choose a reason for hiding this comment

itohro commented Feb 18, 2016

motlin Feb 19, 2016

Choose a reason for hiding this comment

motlin commented Feb 20, 2016