I have one service method that receive parameters as a Map. I will need to add new attributes to this map inside the service. If I pass the original Map object to the parameter instead of a copy, the original parameters object will be changed.
My question here is: which class is responsible for maintain the integrity of the parameter object?
Should the Client class pass a copy in case the Service changes the parameters, or should the ‘Servicedo a copy of the parameters to avoid hurting theClient`?
public class Client{
public void search(){
Map<String, Object> parameters = new HashMap<>();
// defensive parameters copy here?
new Service().search(parameters);
// parameters can be changed if I don't use a defensive copy
}
}
public class Service{
public ... search(Map<String, Object> parameters){
// defensive parameters copy here?
parameters.put("newParam", 1);
}
}
2
In short: If Client cares about the parameters after the fact, then it should create a defensive deep copy of the map (not only the map, but also of the values). If Service needs to modify the collection, then it should create a defensive copy. But we can do better…
To safely pass the Map (though not to safely pass its contents), Client does not need to copy the map, it can use Collections.unmodifiableMap() to create an unmodifiable view.
If Client cares about the map after the fact, then it probably also cares about the contents of the map. Does Client know how to make a defensive copy of all subclasses of Object that will be included in the map? If the actual Objects being used are immutable types, then Client doesn’t need to know how to make a defensive copy of them, and passing the unmodifiable collection is sufficient.
If the actual Objects being used are mutable types, then Client needs to make a deep copy of them, or needs to send copies (or immutables) of whatever Service is going to do with the map. For example, if Service actually intends to call .toString() on all Objects in the map, then maybe the contract can be changed such that Service accepts a Map<String, String>. Maybe Service is a bit more clever, and can also handle Objects that might implement Collection, and uses the .toString() method for non-collections, but iterates over collections and calls the .toString() on each of those. If that is the case, perhaps Client can contain the logic for getting to that step, and the contract changed to Map<String, List<String>>; remember to pass unmodifiable views of the collections, and not the collections themselves.
Wherever possible, I prefer to pass unmodifiable views or immutable collections (for example, from the Guava library), containing immutable objects. If Service actually needs to modify the collection it should be making a copy (because it should know that side effects are generally undesirable), deep if necessary. You will find out in testing if Service actually does produce side effects, because it will throw an exception when trying to modify the unmodifiable/immutable collection, in which case Client needs to create a mutable copy of the collection, pass that, and then never use it again.
As a general rule, I don’t write methods that have side effects when writing an interface to be called by others. Methods with side effects are bad in general, though it could happen that you perform some minor state change other than the one expected.
Even if the client didn’t need to use this data, I don’t think assumptions like that should be made when it comes to the interface. I would go so far as to pass an ImmutableDictionary to the server to make this fact clear. The server should use these parameters and apply them. In this case, you’d probably just copy them, though the server can do with that information whatever it likes. It just can’t change that data, unless of course that was the intended action, in my humble opinion.