Should `setX(Object o)` methods perform deep or shallow copies of objects?

Question

My particular situation is related to Java, but I believe this is a more general OOP question than just Java programming.

The Question: Should "mutator" methods perform deep or shallow copies?

An example:
Let's assume my object has the following structure:

public class MyObj {
  ArrayList<Integer> myList;
  //constructors, etc.

  public void setMyList(ArrayList<Integer> list) {
    this.myList = list; //Option 1
    //OR
    this.myList = new ArrayList<Integer>(list); //Option 2
  }
}

My assumption is that Option 2 is best. This is because it prevents an external class from having direct access to MyObj's list. For example, if I did:

public class SomeOtherClass {
  public static void main(String[] args) {
    ArrayList<Integer> exampleList = new ArrayList<>(Arrays.asList(1, 2, 3));

    MyObj a = new MyObj();
    a.setMyList(exampleList);

    exampleList.add(4);
  }
}

At the end of main, Option 1 means that a.myList contains 4, while Option 2 means a.myList still consists of 1, 2, 3. To me, the second seems preferable.

Is there a standard/convention regarding this situation?

Answer 1

Either approach can be correct; it depends on the semantics you want. Basically, the difference is that the version with the copy means "set myList to contain these elements: list", whereas the version without the copy means "set myList to be this list object: list". I think the former semantics are more often desirable (especially since lists are fairly "transparent": we usually think of lists solely in terms of the elements they contain, and not in terms of some stable "identity" independent of the contents); but there are valid uses for the latter.

If you go with the copy approach, by the way, here are a few things to keep in mind:

• It's a good idea to make sure your get method returns an unmodifiable list, using either Collections.unmodifiableList or something like Guava's ImmutableList.copyOf. (Note that you probably do not want to just return new ArrayList<Integer>(myList), because although that protects myList from being modified by client code, it does not make it obvious that modifying the returned list does not affect the original object. It's better for myObj.getMyList().add(3) to fail noisily.)
• List may not be the appropriate abstraction if you don't actually want client code to manipulate the list as a list. And even if your getter returns List<Integer>, it might be appropriate for your setter to accept Iterable<Integer>.

Answer 2

Note that the same question arises for getters. Not copying a mutable field in a getter means exposing the content for modification.

---

You're speaking about shallow vs deep copy, but you snippet show no vs shallow copy. A deep copy would go like

void setTwoLevelList(List<List<Integer>> twoLevelList) {
    this.twoLevelList = Lists.newArrayList();
    for (List l : twoLevelList) this.twoLevelList.add(Lists.newArrayList(l));
}

or even more complicated for deeper structures or infeasible for something like

void setListOfT(List<T> t) {
    ??? how to copy T?
}

The problem in Java is that it has neither a shallow nor a deep copy. It has clone, which is broken by design and underspecified:

Creates and returns a copy of this object. The precise meaning of "copy" may depend on the class of the object.

Back to the question

So should we do no, shallow, or deep copy in setters?

Ideally, there should be no setters at all. Immutable objects are the easiest to use, but unfortunately, not always possible.

There's no general rule for setters, except that you should avoid them (even more than mutable state in general). Often, an immutable object with a builder or a wither is an alternative.

If you have to write a setter, then I'd recommend to make the field immutable, e.g., use an ImmutableList. If its members are immutable too, then there's no reason to copy anything.

Otherwise, I would copy as deep as I can. Sometimes, an exception must be made for performance reasons, e.g., when passing large objects frequently. In a such a case it's advisable to make the method package private

A good solution is often something like

void setMyList(List<Integer> list) {
    this.list = ImmutableList.copyOf(list);
}