[
https://issues.apache.org/jira/browse/COLLECTIONS-508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13919768#comment-13919768
]
Dipanjan Laha commented on COLLECTIONS-508:
-------------------------------------------
Hi Thomas,
Thanks for looking into the patch. Apologies for the duplicate files, must have
messed up somewhere while created the patch :)
Please find my comments below.
> * Set<Entry<K, Collection<V>>> entrySet():
> I am not so sure if this really makes sense, I think it would be better to
> have a method like Collection<Entry<K, V> entries
Yes, this definitely makes better sense. I will change the implementation
appropriately .
> * boolean containsValue(Object key, Object value): to be consistent with
> removeMapping, I would propose to name it similar, either both
> xxxMapping or xxxEntry, tbd
IMHO I think containsMapping goes well with removeMapping. What do you think?
> * it would also be interesting to have a Bag<K> keys() method that
> returns a view of all keys and how many mappings there are for each,
> but see also below
Find my comments below
> * size && totalSize: I would prefer that size returns the total number of
> mappings (what totalSize does now). The number of keys can be
> retrieved with keySet().size() imho.
I couldn't agree more. I kept it so that it would be consistent with the
MultiValueMap implementation, but it is quite confusing. I will make the
proposed changes.
> * a putAll(K key, Iterable<? extends V> values) would be nice too
I will add this method too.
> Something we have not considered yet is the semantic of the actual
> Collection type used in the MultiValuedMap. It might have a List or Set
> behavior, i.e. allowing duplicates or not. We could make specific
> interfaces for the different types of MultiValuedMaps derived from
> MultiValuedMap.
We can surely have specific interfaces for different types of MultiValuedMaps.
I was thinking on the lines that we can have a flag like allowDuplicates in the
implementing class which can be taken as an argument in a constructor. But your
proposal is better as we can then have strongly typed methods which can return
a List or a Set depending upon the behavior. My only concern is that we would
end up with two implementations for each type of MultiValuedMap, one each for
Set & List. If this is fine then we can go ahead with this. One more thing,
then should we have MultiValuedHashMap anymore? It would be
MultiValuedSetHashMap and MultiValuedListHashMap which can implement
MultiValuedMap(common to both) and MultiValuedSetMap and MultiValuedListMap
individually(In this case we also need to think about the names too). What do
you think?
>Regarding the Bag issue: actually I wanted to clean up the Bag interface
> to be fully Collection compliant for 4.0 but we decided to keep it as is. As
> a workaround there is now a CollectionBag decorator to make a Bag
> compliant to the Collection contract. We could also add now a new
> MultiValuedSet interface that is basically the same as a CollectionBag.
>
>What do you think?
I think the Bag<K> keys() method can be really useful IMHO. We can have a new
MultiValuedSet interface, but that would be kind of moving away from the Bag
interface. Wouldn't it be better to clean up the Bag interface or would it
hamper the backward compatibility? In that case we can have a MultiValuedSet
interface and have a MultiValuedSetWrapper(or some other name) class for
wrapping the existing Bag implementations or have a completely different set of
implementations for MultiValuedSet. I think IMHO that we should hold on with
the Bag<K> keys() method till we decide on the above issue. Let me know your
thoughts.
> MultiMap's methods are not strongly typed even though the interface supports
> generics
> -------------------------------------------------------------------------------------
>
> Key: COLLECTIONS-508
> URL: https://issues.apache.org/jira/browse/COLLECTIONS-508
> Project: Commons Collections
> Issue Type: Improvement
> Components: Map
> Affects Versions: 4.0
> Reporter: Dipanjan Laha
> Attachments: MultiValuedMap.patch
>
>
> Recently I had the need of using a MultiMap in one of my projects. While
> using the same, I found that the MultiMap interface has methods that are not
> strongly typed even though the interface supports generics. For example if I
> have a MultiMap like so
> MultiMap<String, User> multiMap = new MultiValueMap<String, User>();
> where User is a custom Class, then the get(key) method would return me an
> Object which I would need to cast to a Collection like so
> Collection<User> userCol = (Collection<User>) multiMap.get(key);
> I understand that this limitation comes from that fact that the MultiMap
> extends IterableMap which in turn extends Map and other interfaces. Hence the
> MultiMap cannot have a get method which returns a Collection instead of
> Object as that would mean implementing IterableMap with the Generics set to
> be <K,Collection<V>>. In that case the put method's signature would become
> public Collection<V> put(K key, Collection<V> value);
> which we do not want.The same problem would arise with other methods as well,
> ex: containsValue method.
> My proposal is why carry on the signatures of a Map and put it on MultiMap.
> Where as I do agree that it is a Map after all and has very similar
> implementation and functionality, it is very different at other levels. And
> even though the MultiMap interface supports generics, the methods are not
> strongly typed, which defeats the purpose of having generics. So why can't we
> have a separate set of interfaces for MultiMap which do not extend Map. That
> way we can have strongly typed methods on the MultiMap.
> I have included a a patch for these changes. It is not fully complete and has
> some gaps in some TestCases and the documentation but gives a fairly good
> idea of what I am talking about. Please let me know your thoughts on taking
> this approach. Then i will improve the implementation and submit another
> patch.
> The other way could be that we let MultiMap extend the interfaces it does
> today, but with proper types rather than Object. I mean something like this
> public interface MultiMap<K,V> extends IterableMap<K,Collection<V>> instead
> of
> public interface MultiMap<K,V> extends IterableMap<K,Object>
> And then have a separate set of methods on the MultiMap interface which
> supports the specific MultiMap functionality. For example, the put method
> with the above implementation would become
> Collection<V> put(K key, Collection<V> value)
> and we can have another method as
> V putValue(K key, V value)
> This way the functionality of Map is preserved along with strongly typed
> MultiMap methods. If you feel that this approach is better than the earlier
> one, i will implement the same and submit a patch
--
This message was sent by Atlassian JIRA
(v6.2#6252)
