0

Suppose I have the following Set

Set<String> fruits = new HashSet<String>();
fruits.add("Apple")
fruits.add("Grapes")
fruits.add("Orange")

If I wanted to create a defensive copy, so that if the original list is modified, the copy doesn't reflect it, I can do this:

Set<String> unmodifiableFruits = Collections.unmodifiableSet(new HashSet(fruits))

so if I do this:

fruits.add("Pineapple")     
println(unmodifiableFruits)

unmodifiableFruits won't have pineapple.

or I can so this:

Set<String> unmodifiableFruits = Collections.unmodifiableCollection(fruits)

and the result is the same, unmodifiableFruits won't have pineapple.

Questions:

  1. Suppose if I were passing fruits as an argument to a class, is the preferred method the Collections.unmodifiableCollection()?

The reason is, I've read that declaring new in the constructor is a bad practice, if I were to use Collections.unmodifiableSet(), I would need to declare a new HashSet<String>(fruits).

  1. Why can't I do this ?

    Collections.unmodifiableSet(fruits)

and have it return an unmodifiable collection.

instead I have to do this:

Collections.unmodifiableSet(new HashSet<String>(fruits))

Is it because Set is an interface and it doesn't know which implementation to return?

  • You _could_, but that would mean that changes to `fruits` _would_ get reflected in `unmodifiableFruits`, even though `unmodifiableFruits` can't be changed directly. – Louis Wasserman Aug 29 '18 at 01:58
  • 1
    `Set unmodifiableFruits = Collections.unmodifiableCollection(fruits)` won't compile. – shmosel Aug 29 '18 at 01:59
  • @shmosel - Using Groovy it does. :D –  Aug 29 '18 at 02:02
  • @LouisWasserman - Is using `Collections.unmodifiableCollection()` the proper way when passing a `set` into the constructor? –  Aug 29 '18 at 02:03
  • 2
    That's odd. I'm curious what it's doing under the hood. Either way, as mentioned, it will actually reflect updates to the underlying collection, as implied by the [documentation](https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#unmodifiableSet-java.util.Set-). – shmosel Aug 29 '18 at 02:04
  • @shmosel - My question is though why do I have to say `Collections.unmodifiableSet(new HashSet(fruits))`? Why couldn't the developers do it so that I don't have to create a `new Hashset`? WIth `Collections.unmodifiableCollection()` I don't have to create a new `HashSet`. I just pass in the collection. –  Aug 29 '18 at 02:09
  • @shmosel - Is it because `Set` is an interface and the `Collections.unmodifiableSet()` doesn't know what to return? –  Aug 29 '18 at 02:11
  • If you're suggesting there's a difference between `unmodifiableSet()` and `unmodifiableCollection()` with respect to mutability, there isn't. Unless Groovy is secretly creating a new set under the hood in your case. – shmosel Aug 29 '18 at 02:11
  • 1
    Using `new` in a constructor isn't wrong as a rule, for example [this answer](https://softwareengineering.stackexchange.com/a/365123/110799) actually names the scenario in your question as a specific example of `new` being "quite normal". Also, in Java 10, you can use [`Set.copyOf`](https://docs.oracle.com/javase/10/docs/api/java/util/Set.html#copyOf%28java.util.Collection%29) to create an immutable set which is independent of the source collection. – Radiodef Aug 29 '18 at 02:15
  • @shmosel Groovy *is* secretly creating a new set under the hood. See [my answer](https://stackoverflow.com/a/52068694/5221149). – Andreas Aug 29 '18 at 02:36
  • It’s rather strange to ask why these `unmodifiable…` methods do not create a new set/collection in some situations, when the very purpose of these methods is to *never* create a new collection, but to return a *view* to the original collection. It would be a horrible design, if these methods were sometimes creating a new collection and sometimes not and it is even worse to hear that Groovy does exactly that, even on the language level. As @Radiodef mentioned, the Java 10 solution to get an immutable set is `copyOf` which is smart enough not copy already immutable sets. – Holger Aug 30 '18 at 11:58
  • @Holger - It's all in the interest of writing code and learning how these methods work :D. –  Aug 30 '18 at 12:03

1 Answers1

2

Groovy has enhanced collection methods, meaning that it has added methods to the standard collection classes.

Once of those methods is toSet():

Convert a Collection to a Set. Always returns a new Set even if the Collection is already a Set.

Example usage:

def result = [1, 2, 2, 2, 3].toSet()
assert result instanceof Set
assert result == [1, 2, 3] as Set

When you write this:

Set<String> unmodifiableFruits = Collections.unmodifiableCollection(fruits)

it implies a .toSet() call to coerce the Collection returned by unmodifiableCollection into a Set, implicitly copying the data.

When you write this:

Set<String> unmodifiableFruits = Collections.unmodifiableSet(fruits)

the returned value is already a Set, so toSet() is not called, meaning that unmodifiableFruits and fruits share data.

That is why you have to explicitly copy the data when using unmodifiableSet, by adding new HashSet(...).

Is using Collections.unmodifiableCollection() the proper way when passing a set into the constructor?

Absolutely not. Using unmodifiableCollection() and assigning to a Set, implicitly invoking toSet which copies the data, is hiding the fact that a copy is executed.

To ensure code readability, i.e. that anyone reading the code (including yourself in 3 years) will understand what it does, write the code to explicitly copy the data, using the copy-constructor.

Well, of course, unless this is an exercise in code obfuscation, in which case it's a nice misleading trick.

Community
  • 1
  • 1
Andreas
  • 154,647
  • 11
  • 152
  • 247
  • *To ensure code readability, i.e. that anyone reading the code (including yourself in 3 years) will understand what it does, write the code to explicitly copy the data, using the copy-constructor.* You mean use `Collections.unmodifiableSet(new HashSet(fruits))` for readability? –  Aug 29 '18 at 02:38
  • *Absolutely not. Using unmodifiableCollection() and assigning to a Set, implicitly invoking toSet which copies the data, is hiding the fact that a copy is executed.* Why would hiding the fact the copy was executed be wrong? I think using `unmodifiableCollection()` is appropriate in some cases, take this one for example: https://stackoverflow.com/questions/46672320/getters-for-display/46675088 –  Aug 29 '18 at 02:43
  • @Sveta Yes to first comment. --- You should always use the `unmodifiableXxx` that matches the type you're interested in. That's why `Collections` has [8 versions of the method](https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#i58). In the link you provided, `unmodifiableCollection` is only used when the return type is `Collection`. The fact that Groovy *silently* converts a `Collection` to a `Set`, with the hidden side-effect of copying the data, is unfortunate *(in my opinion)*. – Andreas Aug 29 '18 at 02:50
  • @Sveta *"Why would hiding the fact the copy was executed be wrong?"* Because people reading your code, e.g. if they have to modify it, may misinterpret what it does. Unless you're deliberately obfuscating code, code should be a clear as possible to readers of the code, to prevent mistakes and coding errors. – Andreas Aug 29 '18 at 02:53
  • Wow, I learned something about Groovy today. +1. I agree, the silent copying is very confusing. – Stuart Marks Aug 29 '18 at 04:10
  • 2
    @Sveta No one's saying `unmodifiableCollection()` isn't useful. We're saying that the implicit conversion of Collection to Set is highly deceptive, as it results in a mutable copy, instead of the immutable wrapper you'd expect at a glance. – shmosel Aug 29 '18 at 05:34
  • @shmosel - Suppose if I'm stubborn and I'm determined to use `unmodifiableCollection()` despite the readability issues, even if I documented what `unmodifiableCollection()` is doing in my code, would that be better? You could make the argument that `Collections.unmodifiableSet(new HashSet(fruits))` doesn't need documentation because a reader will know what's happening. –  Aug 29 '18 at 15:20
  • @Sveta You can do whatever you want... I'm not sure what your question is. – shmosel Aug 29 '18 at 16:33
  • @shmosel - If I used `unmodifiableCollection()` and documented what it was doing in my code, would it be better? –  Aug 29 '18 at 16:35
  • @Sveta It's not a question of better. They do different things, like I said. – shmosel Aug 29 '18 at 16:40
  • @Sveta No, documenting the misbehavior is not a good way to get around misuse of a helper method. As shmosel already said, you're ending up with 1) a `Set`, not a `Collection`, 2) the `Set` is *mutable*, not immutable, and 3) a copy, not a wrapper. This means you're getting the entire **opposite** of what the method is supposed to give you. Very, very bad. – Andreas Aug 29 '18 at 16:41
  • @Andreas - Good points. Given that `Collections.unmodifiableSet()` is more appropriate, when passing a `set` to the constructor, to make copy I would need to do `Collections.unmodifiableSet(new HashSet(fruits))`. This ensures the client can't change the collection after object creation, it's independent. As linked in my question, using `new` in the constructor is not a good idea, but in this case, is it okay? –  Aug 29 '18 at 17:05
  • @Sveta *"using `new` in the constructor is not a good idea"* You seem to have a terminology issue here. You cannot invoke a *constructor* without the use of the `new` keyword. --- Also, why do you believe copying a collection using `new` is not a good idea? It is the recommended way of doing it. – Andreas Aug 29 '18 at 17:10
  • What I mean is suppose if I a class called `Book` and I had a `set` called `authors` in the `Book` class constructor I would say `Collections.unmodifiableSet(new HashSet(authors))`. Is calling `HashSet(authors))` from the Book constructor okay? See link for details of what I'm talking about. https://softwareengineering.stackexchange.com/questions/365119/is-using-new-in-the-constructor-always-bad –  Aug 29 '18 at 17:16
  • @Sveta That is about creating *new* related objects, not about copying mutable objects to isolate the data given in parameters. Unfortunately, they use the meme *"using `new` in a constructor"* to refer to this, but since creating objects and copying objects (using copy-constructor) both use the `new` operator, the meme is misleading. --- Basically, in the example you just gave, the meme says to not use `new Author("Hemmingway")` to create a *new* author object inside the `Book` constructor. – Andreas Aug 29 '18 at 17:26
  • @Andreas - All questions have been answered, including the ones in the comments. Thanks! –  Aug 29 '18 at 17:28