[google-guice] [EMAIL PROTECTED] commented on revision r714.
Details are at http://code.google.com/p/google-guice/source/detail?r=714
Score: Positive
General Comment:
Until a solution is found to the permgen memory issue, we should:
1) Put a strong warning in the javadoc about excessive use of these
factories, and
2) Add a test that'll fail so long as the issue is unresolved.
Line-by-line comments:
File:
/trunk/extensions/assistedinject/src/com/google/inject/assistedinject/Factories.java
(r714)
===============================================================================
Line 172: * @param factoryInterface a Java interface that defines one or
more create methods.
-------------------------------------------------------------------------------
With the interface you have now - the two-arg static method - I'm not sure
you can support the "or more" case.
Line 176: public static <F> F create(Class<F> factoryInterface, Class<?>
constructedType) {
-------------------------------------------------------------------------------
I don't understand why you're using cglib here. Since you dropped the
builder, it seems superfluous.
You could just as easily use java.lang.reflect proxies and require the user
to do:
bind(PaymentFactory.class).toProvider(
Factories.providerOf(PaymentFactory.class, RealPayment.class));
Or, better yet:
bind(PaymentFactory.class).toProvider(
Factories.providerOf(PaymentFactory.class)
.thatSpecializes(Payment.class, RealPayment.class));
since that'd let you support the "or more" case easily.
Line 336: return "Factory";
-------------------------------------------------------------------------------
We can do better than this. At the very least, I'd want something like
"auto-generated " + factoryType.getName()
Line 339: @Override public boolean equals(Object o) {
-------------------------------------------------------------------------------
I know it doesn't ever get invoked this way as a practical matter, but
could you do me a favor and change this to:
return o == this || (o instanceof Base && ((Base)
o).invocationHandler == this);
It just bothers me to ever have .equals() where !a.equals(a), no matter how
inaccessible a is.
File:
/trunk/extensions/assistedinject/test/com/google/inject/assistedinject/FactoriesTest.java
(r714)
===============================================================================
Line 162: public void
testConstructorDoesntNeedAllFactoryMethodArguments() {
-------------------------------------------------------------------------------
Mandating that the argument lists match would be a bad idea for testing -
for example, suppose I have a LocalCarFactory and a RemoteCarFactory, and
the remote one takes some extra parameters about authentication. I
shouldn't have to clutter up my LocalCarFactory with parameters it doesn't
need. The interface can have extra arguments that aren't directly used,
and that's fine.
Besides, the interface might have arguments that aren't used until some
Provider injected deep into the object structure has get() called on it.
I'm not sure how you could test ahead of time that one of the apparently
unused parameters is eventually used deep in the object structure.
Respond to these comments at
http://code.google.com/p/google-guice/source/detail?r=714
--
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
-~----------~----~----~----~------~----~------~--~---
