[google-guice] limpbizkit commented on revision r723. Details are at http://code.google.com/p/google-guice/source/detail?r=723
Score: Neutral General Comment: Thanks for the thorough review. Doing static analysis on the tests was a nice touch. Fixed here: http://code.google.com/p/google-guice/source/detail?r=730 Line-by-line comments: File: /trunk/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java (r723) =============================================================================== Line 77: return Assisted.class.getName() + "(value=)"; ------------------------------------------------------------------------------- Apparently, this format is consistent with the annotation instances returned by reflection. Added a test case for toString(). Line 142: errors.withSource(method).addMessage( ------------------------------------------------------------------------------- Fixed. Line 157: "Factories.create() factories may only be used in one Injector!"))); ------------------------------------------------------------------------------- Fixed. 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) I don't really care either way. If I refactor the code such that it returns a regular FactoryProvider, I don't think I care as long as the behavioural tests pass. Line 305: @SuppressWarnings("unused") ------------------------------------------------------------------------------- My IDE warns unless variables are both read and written to. http://go/eclipseupgrade Line 322: Car car = injector.getInstance(ColoredCarFactory.class).create(Color.ORANGE); ------------------------------------------------------------------------------- Fixed. 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 -~----------~----~----~----~------~----~------~--~---
