Scott,
I was looking at some htmlcleaner code related to a couple of other discussions/tickets and noticed that some getter methods return, e.g., a List field rather than creating a copy; and some setting methods directly assign, e.g., a List object, into a field value rather than creating a copy.
I know that in the past you had already changed some existing methods to copy mutable field values rather than directly returning them. (I believe one issue I reported was an unexpected side effect of one such change.) But there are definitely still some classes that have getter methods that return a potentially mutable field for the return value, and setter methods that assign potentially mutable objects into fields (e.g., TagNode).
As I'm sure you're aware, this can be hazardous:
I suggest that when you have time for a careful audit, you consider replacing instances of this pattern:
private List<Foo> something; public List<? extends Foo> getSomething() { return something; } public void setSomething(List<? extends Foo> newSomething) { this.something = newSomething; }
with this pattern, or a similar pattern of your choosing:
private final List<Foo> something = new ArrayList<Foo>(); public List<? extends Foo> getSomething() { return new ArrayList<Foo>(something); } public void setSomething(List<? extends Foo> newSomething) { this.something.clear(); if (newSomething != null) { this.something.addAll(newSomething); } }
Obviously, depending upon what the internal invariants of the class in question are, and what the contract of the getter method is, such a change may not be appropriate at all. But I imagine that in most cases, it would be appropriate.
And also obviously, I would not advise making this kind of change without having test cases for the code/methods that it affects. I wouldn't want this suggestion to cause other code, which may currently rely upon getting direct access to various other classes' internals, to unexpectedly stop working as expected.
Clearly this is not a high priority item, but I thought that it was worth mentioning.
Sure, let me see what I can do.