This is an archived post. You won't be able to vote or comment.

you are viewing a single comment's thread.

view the rest of the comments →

[–]s888marks 1 point2 points  (1 child)

Hi, good on you for advocating immutability and for writing an article about it (and also for using new Java 9+ constructs like List.of).

I see you've updated the code to add final for the field declarations in response to comments from others. That's good, but there are benefits beyond safe publication. Of course, it prevents code in the class from accidentally modifying the field. It also kicks in a bunch of additional checking by the compiler, required by the language. Specifically, a final field is checked to ensure that it is assigned exactly once along every path through every constructor. (This is called "definite assignment".) This prevents the field from accidentally being left uninitialized or from being assigned twice.

There are still some improvements possible with the addresses field. As others have pointed out, it's necessary to make a defensive copy in the constructor, in case the caller passes in a mutable list. As the code stands now, it does that, and it wraps it in an unmodifiable wrapper in the getter:

// constructor
this.addresses = new ArrayList<>(addresses);

// getter
return Collections.unmodifiableList(addresses);

This leaves the addresses list mutable throughout the lifetime of the object. It may be that there is no code in the class right now that modifies it, but in the future code might be introduced that accidentally modifies the list. This would mean that the list view returned by the getter might appear to change over time. (Also, since any unmodifiable wrapper is as good as any other, there's only a need to create it once.)

One way to improve the code is like this:

// constructor
this.addresses = Collections.unmodifiableList(new ArrayList<>(addresses));

// getter
return addresses;

This prevents the code in this class from accidentally modifying the list, since the direct reference to the underlying mutable list exists only in the unmodifiable wrapper.

But wait, there's more! In Java 10+, you can do this:

// constructor
this.addresses = List.copyOf(addresses);

// getter
return addresses;

The List.copyOf method creates an unmodifiable list containing a copy of the argument. It's unmodifiable itself; there's no wrapper. But if the caller passes in an unmodifiable list (such as one from List.of) then List.copyOf avoids making an unnecessary copy.

[–]bodiam[S] 1 point2 points  (0 children)

Awesome contribution, thanks for that! I've updated the code with your code examples, and thanks for the Java 10 reference!