Shall we go ahead with this change then? Thanks, Keith.
On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi < amilasuriarach...@gmail.com> wrote: > > > 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/ > -- Keith Chapman Senior Software Engineer WSO2 Inc. Oxygenating the Web Service Platform. http://wso2.org/ blog: http://www.keith-chapman.org