limpbizkit commented on revision r1041 in project google-guice.
Details are at http://code.google.com/p/google-guice/source/detail?r=1041
Score: Positive
General Comment:
Fantastic!
I've got an idea for not creating the binding for Map<K, Set<V>> if it
isn't necessary... We could make the PermitDuplicatesModule do the binding.
To make it work generally, I think the PermitDuplicatesModule would have to
take a Module as a constructor argument and install that. In the Map case,
it would take a module that binds a multimap. In the regular case, it would
take just a Modules.EMPTY_MODULE. What do you think?
The type access sure requires a lot of work. I wonder if there would be
much utility to create types by using placeholders for the variable parts?
Something like this:
public static TypeLiteral<K, V> mapOfSetOfProviderOf(
TypeLiteral<K> keyType, TypeLiteral<V> valueType) {
return new TypeLiteral<Map<A, Set<Provider<B>>>>() {}
.replaceAll(A.class, key)
.replaceAll(B.class, value);
}
Probably not generally reusable to be worth doing, but I think it's kinda
cute.
Line-by-line comments:
File:
/trunk/extensions/multibindings/src/com/google/inject/multibindings/MapBinder.java
(r1041)
===============================================================================
Line 198: Types.setOf(newParameterizedType(Provider.class,
valueType.getType()))));
-------------------------------------------------------------------------------
newParameterizedType(Provider.class --> Types.providerOf(
Line 221: * <ul>
-------------------------------------------------------------------------------
can you replace <ul><li> with plain old <p> tags? It's not really a list
Line 391:
entry.setValue(Collections.unmodifiableSet(entry.getValue()));
-------------------------------------------------------------------------------
ImmutableSet Builder?
Line 393: providerMultimap =
Collections.unmodifiableMap(providerMultimapMutable);
-------------------------------------------------------------------------------
Might as well use ImmutableMap here? It's more memory efficient and faster.
It's not a problem for providers, since they're never null.
Line 399: throw new ProvisionException("no binding for " +
providerMultimapKey
-------------------------------------------------------------------------------
hmmmmm..... would it be simpler to just always support this all the time?
Line 424: multimap.put(key,
Collections.unmodifiableSet(values));
-------------------------------------------------------------------------------
this one still needs to be an LHS due to null problems (yuck!)
File:
/trunk/extensions/multibindings/src/com/google/inject/multibindings/Multibinder.java
(r1041)
===============================================================================
Line 82: * <p><strong>Elements must be distinct.</strong> If multiple
bound elements
-------------------------------------------------------------------------------
Can you fix this comment also?
File:
/trunk/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java
(r1041)
===============================================================================
Line 53: public class MapBinderTest extends TestCase {
-------------------------------------------------------------------------------
add a test for Map<String, Set<Provider<String>>> ? (and it's
unmodifiability)
Respond to these comments at
http://code.google.com/p/google-guice/source/detail?r=1041
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"google-guice-dev" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/google-guice-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
