0

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?

Michal Kordas
  • 10,475
  • 7
  • 58
  • 103
  • Protecting self from self... 1) Constructors should only assign parameters to fields. 2) Making an argument unmodifiable in local scope does not mean it's unmodifiable at the call-site this constructor was invoked from. 3) If it must be unmodifiable, let the call-site wrap it. 4) If it's supposed to be exposed elsewhere, you might think of an unmodifiable _copy_ (usually created in a static factory method). – Lyubomyr Shaydariv Jan 28 '16 at 21:16

3 Answers3

2

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.)
biziclop
  • 48,926
  • 12
  • 77
  • 104
  • I have just `Collection`, not necessarily this is a `List` – Michal Kordas Jan 28 '16 at 20:58
  • 1
    @MichalKordas You can still treat it as a `Collection`, it should make no difference. If it does, it is wrong to accept any collection in the first place. – biziclop Jan 28 '16 at 21:00
  • 2
    @MichalKordas: That doesn't change biziclop's point. `Collections.unmodifiableCollection` just wraps an existing collection... if the caller can modify that collection, you're still not really protecting the internal state of the class. – Jon Skeet Jan 28 '16 at 21:00
1
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 ---------^^^^^^^^^^^^^^^^^^^^^^
Pshemo
  • 122,468
  • 25
  • 185
  • 269
  • can you suggest how idiomatic constructor for `Collection` should look like including copy creation? – Michal Kordas Jan 28 '16 at 21:05
  • Collection is abstract type so we can't simply create new one. You could try using reflection to locate copying constructor (if there is any) but this could overcomplicate things. I would probably simply use `new ArrayList<>(words)` but if it is good solution may depend on what you want to do with it later. – Pshemo Jan 28 '16 at 21:13
0

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.

Daniel
  • 4,481
  • 14
  • 34
  • 1
    Another downside is that we are getting rid of proper generic type since `toArray` returns `Object[]`, not `T[]`. – Pshemo Jan 28 '16 at 21:16
  • `toArray(String.class)` will not compile. You may wanted to use `toArray(new String[words.size()])` (also no casting is needed here) – Pshemo Jan 28 '16 at 21:19
  • Sorry, doing this without pulling up my IDE ;-) – Daniel Jan 28 '16 at 21:21
  • Also possible in Java 8 is `Collections.unmodifiableCollection(Arrays.asList(words.stream().toArray(String[]::new)));` – Daniel Jan 28 '16 at 21:27
  • True, now that you corrected that mistake, why `Arrays.asList(code.code(code))` instead of `new ArrayList<>(words)` :) – Pshemo Jan 28 '16 at 21:31
  • `Arrays.asList` creates a wrapper around the fixed-sized input array. `new ArrayList<>(words)` will create a variable sized array, which may end up bigger than required. – Daniel Jan 28 '16 at 21:36
  • But that array will be used only by `Collections.unmodifiableCollection` which doesn't allow us to modify it anyway, so there is no real gain here (or I am missing something). – Pshemo Jan 28 '16 at 21:39
  • 1
    It's a minimal gain, but the gain is that the new ArrayList<> constructor may allocate a larger array than it needs to. Arrays.asList uses the exact size of the array. – Daniel Jan 28 '16 at 21:43
  • Oh, now I see what you meant in previous comment (well it means it is getting late or it is time for a break). – Pshemo Jan 28 '16 at 21:46