keith chapman wrote: > Um - we aren't currently returning the actual Map, we're returning a > clone of > it (just done manually instead of using clone()). You had suggested > returning the actual Map itself, which is what I was reacting to > above. I'm > not saying the API should go away. > > The reason we have implemented a clone is the limitation in the API. If > the return type was Map we could have returned the allServices map. > Wouldn't this clone be expensive when there are lots of services deployed?
Hm - it seems like you didn't actually get my point. We CAN'T return the actual allServices map because that would be breaking the abstraction boundary provided by the class. There is (and should be) a contract about how you add and remove services - if you don't follow the contract, then stuff like engaging global modules doesn't happen. In our case this stuff lives in the code in addServiceGroup() (whether or not that's where it should live is debatable, but the point is that it exists). If someone were to mess around with the actual contents of allServices, that would produce undefined results. This is basic OOP - don't expose the inner workings of your class by offering mutable references to private data. Instead of cloning, we could just use Collections.unmodifiableMap() for speed, but we'd need to be clear (in the code and the JavaDoc) whether we're returning a "moment in time" (i.e. a clone()) or a "live view" (which is I think what unmodifiableMap() gives you) - i.e. if some other thread deploys a service while I have the reference, can I see the new service or not? --Glen
