[google-guice] [EMAIL PROTECTED] commented on revision r723.
Details are at http://code.google.com/p/google-guice/source/detail?r=723
Score: Positive
General Comment:
Much better, thanks. I like this.
My line-by-line comments are mostly nits generated by throwing all the
automated tools I had on my laptop at the code; I'm not sure if any of them
are worth addressing.
Line-by-line comments:
File:
/trunk/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java
(r723)
===============================================================================
Line 77: return Assisted.class.getName() + "(value=)";
-------------------------------------------------------------------------------
"@" in front?
Line 142: errors.withSource(method).addMessage(
-------------------------------------------------------------------------------
There's no test for this else branch
Line 157: "Factories.create() factories may only be used in one
Injector!")));
-------------------------------------------------------------------------------
There's no test for this.
File:
/trunk/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java
(r723)
===============================================================================
Line 69: FactoryProvider.newFactory(ColoredCarFactory.class,
Camaro.class));
-------------------------------------------------------------------------------
I'll note that nowhere in this test do you assert that
FactoryProvider.newFactory is returning to you an instance of
FactoryProvider2. I can't decide if that's an oversight, or deliberate
good testing practices. (Since really, client code shouldn't ever know
about the FactoryProvider2 class at all)
Line 305: @SuppressWarnings("unused")
-------------------------------------------------------------------------------
Does your IDE really claim that this is unused?
Line 322: Car car =
injector.getInstance(ColoredCarFactory.class).create(Color.ORANGE);
-------------------------------------------------------------------------------
Nothing asserted about car...
Line 414: @SuppressWarnings("unused")
-------------------------------------------------------------------------------
Again, does your IDE think that these are unused? Mine doesn't...
Respond to these comments at
http://code.google.com/p/google-guice/source/detail?r=723
--
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
-~----------~----~----~----~------~----~------~--~---
