Hi Keith! keith chapman wrote: > Due to a design issue in the Axis2 API we have implemented the > getServices() method as follows, > > public HashMap<String, AxisService> getServices() { > HashMap<String, AxisService> hashMap = new HashMap<String, > AxisService>(this.allServices.size()); > String key; > for (Iterator<String> iter = > this.allServices.keySet().iterator(); iter.hasNext();){ > key = iter.next(); > hashMap.put(key, this.allServices.get(key)); > } > return hashMap; > } > > In my view this is highly inefficient, especially if you have plenty of > services in the system. Wouldn't it be better to fix the API and return > a Map instead of a HashMap? If we did that we could simple return > allServices instead of returning a copy of it.
You don't want to return an actual reference to a mutable object that backs a significant data model - otherwise people could just get that Map and (mistakenly or maliciously) randomly add and delete services. However, your point about the HashMap in the API is entirely right - we should make it a Map (which shouldn't break anyone already using it in it's more specific form). Also, I'm not sure why we're not just returning allServices.clone()... NIH? :) > If we are making this change I propose that we fix this for modules, > transports as well. Sure. --Glen