[google-guice] limpbizkit commented on revision r714. Details are at http://code.google.com/p/google-guice/source/detail?r=714
Score: Neutral General Comment: Changes applied here: http://code.google.com/p/google-guice/source/detail?r=723 Line-by-line comments: File: /trunk/extensions/assistedinject/src/com/google/inject/assistedinject/Assisted.java (r714) =============================================================================== Line 35: String value() default ""; ------------------------------------------------------------------------------- Done. /** * The unique name for this parameter. This is matched to the [EMAIL PROTECTED] @Assisted} constructor * parameter with the same value. */ String value() default ""; File: /trunk/extensions/assistedinject/src/com/google/inject/assistedinject/Factories.java (r714) =============================================================================== Line 57: private static final Class[] ONLY_RIH = { RealInvocationHandler.class }; ------------------------------------------------------------------------------- Obsolete. Line 59: static final Assisted DEFAULT_ASSISTED = new Assisted() { ------------------------------------------------------------------------------- > perhaps call this DEFAULT_ANNOTATION Done. Line 104: * is not bound in the injector. ------------------------------------------------------------------------------- > in the "main" injector? Done. Changed to, "This serves to document that the parameter is not bound by your application's modules." Line 116: * }</pre> ------------------------------------------------------------------------------- > Should explain here that providers, provider methods, and deep injection are supported. Not done. I'm intentionally not getting into that, since I think it makes things more complicated than necessary. PERHAPS it would be good to mention deep injection... Line 126: * factory cannot be used until it has been initialized. ------------------------------------------------------------------------------- > until it has been initialized? do you mean until after the injector has been initialized? Fixed. Line 172: * @param factoryInterface a Java interface that defines one or more create methods. ------------------------------------------------------------------------------- > i don't remember why we did this before - why do we support the "or more" case? Does it actually come up much? > With the interface you have now - the two-arg static method - I'm not sure you can support the "or more" case We support multiple create methods. This is theoretically handy in case you want to remove a parameter without changing the callers. 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. cglib no longer used. Line 336: return "Factory"; ------------------------------------------------------------------------------- > We can do better than this. At the very least, I'd want something like "auto-generated " + factoryType.getName() Changed to "com.google.foo.Foo.Factory for com.google.foo.RealFoo" 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: Fixed. File: /trunk/extensions/assistedinject/test/com/google/inject/assistedinject/FactoriesTest.java (r714) =============================================================================== Line 103: return "vroom!"; ------------------------------------------------------------------------------- > noise? Fixed. Killjoy! Line 127: public void testFactoryWithMultipleMethods() { ------------------------------------------------------------------------------- > testFactoryUsesInjectedConstructor? Fixed. Line 389: public static class MultipleConstructorDefectiveCar implements Car { ------------------------------------------------------------------------------- > unused? Fixed. Line 475: assertEqualsBothWays(carFactory, carFactory); ------------------------------------------------------------------------------- > do you need to test hashCode also? assertEqualsBothWays checks equals() and hashCode() Line 483: public void testInjectingProviderOfParameter() { ------------------------------------------------------------------------------- > It's a bit weird to me to inject Assisted params into fields... not sure how I feel about this. Probably. It works, it's impossibly hard to take it out. Line 584: public void testDistinctKeys() { ------------------------------------------------------------------------------- > add a negative test? Fixed. 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 -~----------~----~----~----~------~----~------~--~---
