Hmm. I agree that (B) is a bit cleaner, but since JsLibrary only has one public static method right now, it may be overkill... so I vote for (A) for the moment. I could be convinced otherwise though.
On Mon, Oct 19, 2009 at 1:53 PM, <[email protected]> wrote: > > http://codereview.appspot.com/135048/diff/1/2 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java > (right): > > http://codereview.appspot.com/135048/diff/1/2#newcode79 > Line 79: JsLibrary numberConstants = > createJsLibrary(JsLibrary.Type.RESOURCE, > The pattern you are suggesting is a "JsLibrary factory" pattern, which I > do like. I could do this one of 3 ways, and would like to get some > comments on it: > > A) Simply inject JsFeatureLoader and it becomes the factory. > > B) Create a class (and maybe an interface and default impl) > JsLibraryFactory, and put the static methods of JsLibrary into this. > Personally I like this, it would be very clear you have a factory > pattern at work, and would work better for me, but it is a larger code > change. > > C) Simply stay with this minimalist change. > > > On 2009/10/19 19:06:04, johnfargo wrote: > >> To me this isn't injected so much as overridable. Actual injection >> > would make > >> sense to me: make createJsLibrary in JsFeatureLoader public, then >> > @Inject the > >> JsFeatureLoader here. >> > > The downside of this is that JsFeatureLoader wasn't really designed >> > from the > >> start to be @Injectable... but that will have to be cleaned up later >> > anyway. > > http://codereview.appspot.com/135048 >
