On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <g...@thoughtcraft.com> wrote:
> Amila Suriarachchi wrote: > > 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. > > > > As I remember this change was done to avoid concurrent modifications to > > service map[1]. > > Right - before this change we were doing something actively bad/wrong, i.e. > returning the internal map. After the change we were returning a cloned > copy > of the map (though not using clone() for some reason), which is good in > that > it prevented people from misusing it. > > > At that time services map was a HashMap and could not fix the issue by > > changing it to a ConcurrentHashMap since API method returns a HashMap. > > > > Currently anyone can get a copy of internal map (I think we can not use > > clone() method since internal implementation is ConcurrentHashMap and we > > need to return a HashMap). And if they want to add or remove service > > they have to use addService and removeService respectively. > > > > Keith, if you really need the internal map IMHO best way is to add a new > > API method. > > Ah, no. The "best way" is NOT TO OFFER ACCESS TO THE INTERNAL MAP. > > Keith's suggestion is to change the API so that it returns a Map - this > would > be much more correct since it increases the level of abstraction for the > method, and would therefore let us, the implementors, freely decide how > this > should work internally. Right now we have problems because someone made > this > method overly specific, and this is what we should fix. (I was incorrect > earlier when I said this wouldn't break people, btw - I was thinking about > stuff like getServices().get("MyService"), but of course "HashMap foo = > getServices()" would fail. I still think we should fix it.) > > My comment is that we should not only return a Map, we should change the > implementation to make sure the Map is immutable, and make sure the JavaDoc > explains what is going on. +1. Here the real problem is this API contains a hashMap instead of Map. What I got from the Keiths' first mail is that he need the API change to return the internal map without copying. I suggested to add a new method if he really need it so that only he use that method. I agree with you that this is not a good change. thanks, Amila. > > > Make sense? > > --Glen > -- Amila Suriarachchi WSO2 Inc. blog: http://amilachinthaka.blogspot.com/