'Collections.unmodifiableCollection' in constructor

Question

From time to time during code reviews I see constructors like that one:

Foo(Collection<String> words) {
    this.words = Collections.unmodifiableCollection(words);
}

Is this proper way of protecting internal state of the class? If not, what's the idiomatic approach to create proper defensive copy in constructors?

Answer 1

It should be, but it isn't correct because the caller can still modify the underlying list.

Instead of wrapping the list, you should make a defensive copy, for example by using Guava's ImmutableList instead.

Foo(Collection<String> words) {
    if (words == null) {
       throw new NullPointerException( "words cannot be null" );
    }
    this.words = ImmutableList.copyOf(words);
    if (this.words.isEmpty()) { //example extra precondition
       throw new IllegalArgumentException( "words can't be empty" );
    }
}

So the correct way of establishing the initial state for the class is:

1. Check the input parameter for null.
2. If the input type isn't guaranteed to be immutable (as is the case with Collection), make a defensive copy. In this case, because the element type is immutable (String), a shallow copy will do, but if it wasn't, you'd have to make a deeper copy.
3. Perform any further precondition checking on the copy. (Performing it on the original could leave you open to TOCTTOU attacks.)

Answer 2

Collections.unmodifiableCollection(words);

only creates wrapper via which you can't modify words, but it doesn't mean that words can't be modified elsewhere. For example:

List<String> words = new ArrayList<>();
words.add("foo");
Collection<String> fixed = Collections.unmodifiableCollection(words);

System.out.println(fixed);
words.add("bar");
System.out.println(fixed);

result:

[foo]
[foo, bar]

If you want to preserve current state of words in unmodifiable collection you will need to create your own copy of elements from passed collection and then wrap it with Collections.unmodifiableCollection(wordsCopy);

like if you only want to preserve order of words:

this.words = Collections.unmodifiableCollection(new ArrayList<>(words));
// separate list holding current words ---------^^^^^^^^^^^^^^^^^^^^^^

Answer 3

No, that doesn't protect it fully.

The idiom I like to use to make sure that the contents is immutable:

public Breaker(Collection<String> words) {
    this.words = Collections.unmodifiableCollection(
        Arrays.asList(
            words.toArray(
                new String[words.size()]
            )
        )
    );
}

The downside here though, is that if you pass in a HashSet or TreeSet, it'll lose the speed lookup. You could do something other than converting it to a fixed size List if you cared about Hash or Tree characteristics.