This is a non-trivial question. Basically, you have to think about any internal state of a class that you give to any other class via getter or by calling another class' setter. For example, if you do this:
Date now = new Date();
someObject.setDate(now);
// another use of "now" that expects its value to not have changed
then you potentially have two problems:
someObject could potentially change the value of "now", meaning the method above when it later uses that variable could have a different value than it expected, and- if after passing "
now" to someObject you change its value, and if someObject did not make a defensive copy, then you've changed the internal state of someObject.
You should either protected against both cases, or you should document your expectation of what is allowed or disallowed, depending on who the client of the code is. Another case is when a class has a Map and you provide a getter for the Map itself. If the Map is part of the internal state of the object and the object expects to fully manage the contents of the Map, then you should never let the Map out. If you must provide a getter for a map, then return Collections.unmodifiableMap(myMap) instead of myMap. Here you probably do not want to make a clone or defensive copy due to the potential cost. By returning your Map wrapped so that it cannot be modified, you are protecting your internal state from being modified by another class.
For many reasons, clone() is often not the right solution. Some better solutions are:
- For getters:
- Instead of returning a
Map, return only Iterators to either the keySet or to the Map.Entry or to whatever allows client code to do what it needs to do. In other words, return something that is essentially a read-only view of your internal state, or
- Return your mutable state object wrapped in an immutable wrapper similar to
Collections.unmodifiableMap()
- Rather than returning a
Map, provide a get method that takes a key and returns the corresponding value from the map. If all clients will do with the Map is get values out of it, then don't give clients the Map itself; instead, provide a getter that wraps the Map's get() method.
- For constructors:
- Use copy constructors in your object constructors to make a copy of anything passed in that is mutable.
- Design to take immutable quantities as constructor arguments when you can, rather than mutable quantities. Sometimes it makes sense to take the long returned by
new Date().getTime(), for example, rather than a Date object.
- Make as much of your state
final as possible, but remember that a final object can still be mutable and a final array can still be modified.
In all cases, if there is a question about who "owns" mutable state, document it on the getters or setters or constructors. Document it somewhere.
Here's a trivial example of bad code:
import java.util.Date;
public class Test {
public static void main(String[] args) {
Date now = new Date();
Thread t1 = new Thread(new MyRunnable(now, 500));
t1.start();
try { Thread.sleep(250); } catch (InterruptedException e) { }
now.setTime(new Date().getTime()); // BAD! Mutating our Date!
Thread t2 = new Thread(new MyRunnable(now, 500));
t2.start();
}
static public class MyRunnable implements Runnable {
private final Date date;
private final int count;
public MyRunnable(final Date date, final int count) {
this.date = date;
this.count = count;
}
public void run() {
try { Thread.sleep(count); } catch (InterruptedException e) { }
long time = new Date().getTime() - date.getTime();
System.out.println("Runtime = " + time);
}
}
}
You should see that each runnable sleeps for 500 msec, but instead you get the wrong time information. If you change the constructor to make a defensive copy:
public MyRunnable(final Date date, final int count) {
this.date = new Date(date.getTime());
this.count = count;
}
then you get the correct time information. This is a trivial example. You don't want to have to debug a complicated example.
NOTE: A common result of failure to properly manage state is a ConcurrentModificationException when iterating over a collection.
Should you code defensively? If you can guarantee that the same small team of expert programmers will always be the ones writing and maintaining your project, that they will continuously work on it so they retain memory of the details of the project, that the same people will work on it for the lifetime of the project, and that the project will never become "large," then perhaps you can get away with not doing so. But the cost to defensive programming is not large except in the rarest of cases -- and the benefit is large. Plus: defensive coding is a good habit. You don't want to encourage the development of bad habits of passing mutable data around to places that shouldn't have it. This will bite you some day. Of course, all of this depends on the required uptime of your project.
