-
Notifications
You must be signed in to change notification settings - Fork 604
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
Implement Iterate.toMultimap() #37
Conversation
fc2421c
to
bc7c316
Compare
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.
This doesn't need to be in this commit and can be taken into another issue. |
To be consistent with |
+1 on @motlin 's suggestion. |
@motlin , @itohro What will be the names of two methods? Adding all the methods which need the name to be finalized:
Also, we need to add two more methods, to cover all the patterns:
|
Thinking further on naming consistency with Note: I fixed generics signature for Iterable from
|
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). |
Do you think all four variants are useful? Or is this for completeness? |
@itohro I would want to have @motlin it is more for completeness. I think the most useful one to have will be:
|
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.
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.
I'm not keen on adding the 4th one, but we could name it below if we want to.
|
Some more thoughts on naming. Two of these patterns have workarounds already. They are the ones where you can call 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) |
I agree with @motlin the most common use cases I have come across were for these, also these methods are closer to the
I will add these two on |
The plan sounds good to me! |
On second thoughts, if I add this:
the other variant:
gets a workaround, because now the However, I would like to add
|
cdd7ca4
to
d45d0e9
Compare
Why don't we add 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 |
I'm good with this => "3 separate commits under 1 pull request". But let's wait for @motlin 's feedback. |
Apart from adding |
There's one reason you might want to split the pull request. I think adding 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. |
But do we require a variant without a keyFunction, will it not be using |
No it doesn't seem necessary. Seems nice to have.
|
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a key and several values (rather than one value)
031191e
to
2747457
Compare
7f77c47
to
05e5c38
Compare
@@ -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)}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one Javadoc comment using "Also see" and Javadoc links seems inconsistent with the rest.
45f8329
to
6df029a
Compare
Signed-off-by: Nikhil Nanivadekar <nikhil.nanivadekar@gs.com>
6df029a
to
c886e26
Compare
👍 |
Adding
toMultimap()
onRichIterable
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 oftoMultimap()
, this is because I want users to control the output instead of deciding which type ofMultimap
is returned for a particularIterable
.