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.
Make sense?
--Glen
