As a rule of thumb, Guice should generally only create: * what you want it to populate with other dependencies * what you want to be testable with mock substitutes * Singletons
If it doesn't come under one of these, you likely dont need to Guice it (example: an "Address" data model object). Dhanji. On Sun, Feb 1, 2009 at 10:22 AM, Matt G <[email protected]> wrote: > > Thanks for the reply. > > I've been trying to let Guice do as much object creation as possible. > Perhaps I've taken this too far... > > So just to summarise what you have recommended: > > 1) Don't bother injecting empty collections, or other simple objects > that don't need to be unit tested. > 2) Don't bother injecting objects whose sole purpose is to clone other > simple objects, just use one of the static Collections methods, or new > it up youself. > 3) Unit test my helper classes to ensure that it does not return the > internal Map. > > I suppose my problem is working out when it is appropriate for Guice > to create the object, and when it is appropriate for me to create the > object. > > As you spell out in your blog, IOC is about making unit testing > easier, and as it is not necessary to unit test core java components > such as empty maps, it is not worth the effort to get Guice to create > them. However, I don't think I'd go as far to say injecting an empty > map was container abuse. For example, if I wanted to see what the > effects of using different Map implementations were, it would be nice > to have the empty map type defined in configuration to aid comparison > testing. Perhaps what to take from this is that you should only get > Guice to inject empty collections if you have a concrete for doing so. > > Thanks, > > Matt. > > > Eric Anderson wrote: > > Matt, > > > > Unless something outside of your MapHelper needs a reference to the same > > instance of the map, I would not pass the empty collection into the > > constructor. I would have the MapHelper create the map (via normal > > construction) in the constructor of the MapHelper. I might even just use > a > > field initializer. > > > > I never use DI to create empty collections. I have used it to create > > populated collections (say, with a bunch of validation strategies). > > > > If you have concerns about returning the actual internal instance, simply > > write some unit tests that force the behavior. I would probably use > static > > methods for actually cloning the collection. In this case, I would > probably > > write some JUnit tests as follows: > > > > public class MapHelper_when_populated_Test { > > > > private MapHelper helper; > > > > @Before > > public void setup() { > > helper = new MapHelper(); > > helper.doSomethingThatAddsToTheMap("key", "value"); > > } > > > > @Test > > public void mapHelper_should_keep_the_things_that_are_added() { > > assertEquals("value", helper.getMap().get("key")); > > } > > > > @Test > > public void mapHelper_should_not_expose_its_internal_map() { > > Map map1 = helper.getMap(); > > Map map2 = helper.getMap(); > > > > //If the instances are different, then the helper is probably not > > exposing the internal collection > > assertFalse(map1 == map2); > > > > //Just to make sure nothing funny is going on > > map1.put("key2", "value2"); > > assertNull(map2.get("key2")); > > } > > } > > > > The second bit on the second test is probably not needed. But, this > ensures > > that you aren't getting back different wrapper instances that are backed > by > > the same internal class (I've seen this behavior before). > > > > I started writing much more about when I actuall do provide collection > from > > Guice, and it turned into a blog post: > > > > > http://testinfected.blogspot.com/2009/01/when-collections-are-configuration.html > > > > Eric > > > > On Sat, Jan 31, 2009 at 1:03 PM, Matt G <[email protected]> > wrote: > > > > > > > > Hi, > > > > > > Let me start by stating that I am very new to dependency injection and > > > Guice (1 week ish), so it is entirely possible that some, if not all > > > of the following questions will be rather silly... anyway, with the > > > disclaimer out the way, here I go... > > > > > > Let's say I have some sort of helper class, that populates a map in a > > > particular way, and then returns the map so that it is appropriate for > > > use in another class via a Map<K,V> getMap() method. The helper class > > > will clearly have a dependency on some sort of empty map, so it makes > > > sense to have a Map passed directly into the constructor. So far so > > > good... > > > > > > > > > Q1) > > > > > > Given that the map is expected to be empty, it clearly makes sense to > > > check for this fact in the constructor, and throw an > > > IllegalArgumentException if it is not the case. I was considering on > > > creating an @Empty annotation, so that Empty collections can be > > > differentiated between non empty ones. If there are too many > > > implementing classes however, I'd need to create new ones > > > @EmptyHashMap, @ EmptyEnumMap etc... > > > How do people generally pass in empty collections into classes? Often > > > classes have dependencies on empty collections that are then > > > populated, and used internally without exposing the Map via the class > > > api. In both of the above situations, how do people generally pass in > > > these empty Collections? > > > > > > MapHelper > > > { > > > > > > { > > > > > > Ok, going back to the Helper class example. We have a class that > > > returns some sort of appropriately populated map via getMap(). > > > However, if we want the helper class to be immutatble, we cannot > > > return the internal map directly, so we will want to create a new Map > > > based on the internal one. Therefore, the class will now have another > > > dependency. We need some sort of method that creates a new Map from an > > > old one. In other words we need some sort of class that has some sort > > > of Map<K,V> create(Map<> map) method. > > > > > > Q2) > > > > > > a) Should this new class implement Provider<Map<>> ? > > > Or should I just create a MapFactory interface, and associated > > > HashMapFactory implementation or whatever? > > > > > > b) Alternatively I could create some sot of HashMapFactory class that > > > has a createEmptyMap method, a createPopulatedMap(Map...) method etc, > > > and just pass this all emcompasing Map factory class into the > > > constructor. in this case there would no longer be any need to pass in > > > the empty map directly. > > > > > > The b) approach seems to make sense to me, but wanted to check with > > > others first. We're getting near the end of this post now, I > > > promise... > > > > > > Ok, so now I have another class, that internally expects a map of the > > > form created by the helper class. > > > > > > Q3 > > > a) How would I best pass this to my class? by passing the helper > > > directly, or the Map directly??? > > > > > > > > > class NewClass > > > { > > > > > > private final Map<> map; > > > > > > NewClass(Map<> map) > > > { > > > // To make class immutable need to do this.map = new > > > HashMap<..>(map) or something, > > > // so I need a new dependency to carry this out, a > > > MapFactory or something. > > > } > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > When creating a new class I find I will often have a dependency on > > > some sort of empty collection, that is used internally, but not > > > exposed via the class api. So with the goal of completly removing the > > > need for the new operator in my code, I pass this directly into the > > > constructor. Great, job done. However > > > > > > > > > > > > > > > > > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "google-guice" 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?hl=en -~----------~----~----~----~------~----~------~--~---
