Should I copy list both in constructor and in getter?

Question

I have a simple immutable class:

public class ColumnsWrapper implements Columns {
    private final List<String> columnNames;

    public ColumnsWrapper(List<String> columnNames) {
        this.columnNames = new ArrayList<String>(columnNames);
    }

    @Override
    public List<String> getNames() {
        return new ArrayList<String>(columnNames);
    }

}

Should I always save a copy of passed list in constructor and return copy of saved list in getter?

This class from library project and can be used in several projects in the future.

The purpose of this class is only to store immutable list of column names.

Answer 1

You claim you have a simple immutable class, but you don't.

All public methods on Immutable classes should also be final. For example, you claim your class is immutable, but, I can change it with:

class MyColumnsWrapper extends ColumnWrapper {

    private final List<String> mutableColumnNames;

    public ColumnsWrapper(List<String> columnNames) {
        super(columnNames);
        mutableColumnNames = columnNames;
    }

    @Override
    public List<String> getNames() {
        return mutablecolumnNames;
    }
}

...
ColumnsWrapper wrapper = new MyColumnsWrapper(mycols);
...

In other words, to be immutable, you also need to have non-overridable methods. The best, and easiest way to accomplish that is to make the class final.

Apart from that, yes, your class is a decent Immutable instance. Note that the immutability depends on the fact that the list consists of String values, which are also immutable.

Answer 2

It depends on what the goal is. Can the list never change after construction? Can it change but only the ColumnWrapper can do so? Or can it change and everyone is allowed to do that?

If the list can not change after construction, consider using an ImmutableList (from google's guava). You should then declare the field columnNames and the return value of getNames() to be of that type. You then either create an immutable list during construction or use that type for the constructor argument as well.

If the list can change after construction (but only ColumnsWrapper can do so), the getter should return an unmodifiableList. Note that this will cause exceptions if the client of your class tries to manipulate the list. You should then also copy during construction (as you currently do).

In any way should you document the behavior with comments on the respective public members (i.e. the constructor and the getter).

Edit

Ok, so the class has to be immutable. As @rolfl explains, it can be subclassed so this is not yet the case. You can either make the class final or make the constructor private and provide a static factory method.

Furthermore you have to make sure, that the list can not be modified. The easiest and most intention revealing way I know of is the ImmutableList I mentioned above. Another solution would look like this:

public class ColumnsWrapper implements Columns {

    private final List<String> columnNamesUnmodifiable;

    public ColumnsWrapper(List<String> columnNames) {
        List<String> columnNamesCopy = new ArrayList<>(columnNames);
        columnNamesUnmodifiable = Collections.unmodifiableList(columnNamesCopy);
    }

    // OPTION A

    @Override
    public List<String> getNamesUnmodifiable() {
        return columnNamesUnmodifiable;
    }

    // OPTION B

    @Override
    public Iterable<String> getNamesUnmodifiable() {
        return columnNamesUnmodifiable;
    }

}

Note that I changed the name to inform callers that they will get an unmodifiable instance.

I also provided another additional option (you have to choose one) with a different return type. If you are sure that callers will only iterate over the returned instance (as is often the case) the iterable will suffice. But it can not be used to add methods and since removal is an optional method (i.e. many iterators support no removal) it better conveys immutability.

I any way the interface documentation should also make that clear.

Answer 3

I would use unmodifiableList in that case. That way:

public class ColumnsWrapper implements Columns {

    private final List<String> columnNames;

    public ColumnsWrapper(List<String> columnNames) {
        this.columnNames = Collections.unmodifiableList(columnNames);
    }

    @Override
    public List<String> getNames() {
        return columnNames;
    }

}

EDIT: (in order to stop academic discussion)

Create a instance of the class that way:

Columns columns = new ColumnsWrapper(Arrays.asList("Column1", "Column2", "Column3"));

or

Columns columns = new ColumnsWrapper(new ArrayList(initialColumnList));

Answer 4

It depends on the semantics of your code:

• If you want to allow modification of the array obtained from the getter (like changing one column name, adding more columns from the outside, etc.) then you should not return a copy of it
• On the other hand, if you want to have the array appear as immutable to the outside then you should definitely return a copy of it in the getter too.

In both cases, you should document the behavior.