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.

  • return Collections.unmodifiableList(columnNames); instead of return new ArrayList<String>(columnNames); would be more efficient. – assylias Oct 29 '14 at 8:28
  • @assylias: See my comment on this answer why that is no good idea. (It is also premature optimization as there will most likely not be thousands of column names. And if there are, some other place will surely be the bottleneck.) – Nicolai Oct 29 '14 at 11:11
  • @NicolaiParlog I suggested to make the change in the getNames method, not in the constructor. It is an optimisation but I would not call it premature in the sense that it does not make the code more complex and will clearly perform better (if only because it will generate less garbage) so why not do it? – assylias Oct 29 '14 at 11:14
  • @assylias I am sorry, you are absolutely right. I just missed the return. :) I still advice to already make the instance owned by the class unmodifiable. This makes the class truly immutable and allows the getter to do no extra work. – Nicolai Oct 29 '14 at 20:50
  • @NicolaiParlog I had not seen your answer - it makes sense indeed. – assylias Oct 29 '14 at 21:46
up vote 5 down vote accepted

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.

  • getNames() still returns a mutable Collection. Example: getNames().remove(2) – dit Oct 28 '14 at 17:14
  • @dit - yes, even in the original class, the collectionr eturned by the get is mutable, but the base class is not. – rolfl Oct 28 '14 at 17:16
  • From the Question: "The purpose of this class is only to store immutable list of column names." – dit Oct 28 '14 at 17:17
  • @dit - I think there's a miscommunication here, I am not sure I understand your concerns. Let's chat about it in the 2nd monitor – rolfl Oct 28 '14 at 17:19

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.

  • I'm pretty sure you can't do that in Java. You declare two methods of the same class with no parameters, and that will fail to compile. It's also not necessary, as List<String> implements Iterable<String> – raptortech97 Oct 29 '14 at 15:54
  • Of course you are right and it was not my intent to propose both methods. Rather I'd recommend to use the second. I improved the answer to clarify why. – Nicolai Oct 29 '14 at 20:57

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));
  • 4
    This class is not immutable. Collections.unmodifiableList does not make the specified instance unmodifiable, only the returned one. But the latter is still backed by the former. This means that whoever invoked the constructor still has a reference to the modifiable list and can change the class's state. – Nicolai Oct 28 '14 at 18:31
  • @NicolaiParlog who needs to make the specified instance unmodifiable? "The purpose of this class is only to store immutable list of column names" – dit Oct 28 '14 at 21:25
  • 2
    You can make it immutable by doing this.columnNames = Collections.unmodifiableList(new ArrayList<String>(columnNames)); - make a new copy so you own all access to it, and then make the only view into that list be immutable. – corsiKa Oct 28 '14 at 22:39
  • 1
    @dit: But the way you wrote it, the list in the field columnNames is not immutable. It is only an unmodifiable view on the constructor argument. Whoever holds the original reference on the argument can still change the column names. This makes the class mutable. – Nicolai Oct 29 '14 at 11:08

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.

  • The first interpretation doesn't sound much like an "immutable class" to me. – 200_success Nov 1 '14 at 9:11

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.