Does it follow java conventions to only provide a getter for collections? [closed]

Question

I recently encountered code like the following:

public List<Item> getItems() {
    if (items == null) {
        items = new ArrayList<Item>();
    }
    return this.items;
}

and crucially, no setter method.

So if you wanted to add to the arrayList, you would have to do

foo.getItems().add(...)  

rather than

foo.setItems(myArrayList)

I've not seen this idiom before, and I can't say I like it, but when I generated some mapping code using mapstruct.org (great tool by the way), mapstruct handles it fine and correctly generates code that uses the getter as a setter as well.

I'm just wondering - is this a common idiom that I've somehow missed? It seems pointless to me, but perhaps there is some wisdom behind it I'm not seeing?

Answer 1

It is common but it is not recommended.

The problem isn't with the lazy initialisation (that part is fine) but the exposure of what should be an implementation detail. Once you expose the actual, mutable(!) list object you store in your field, the caller of the code can do anything with the list, even things you don't expect.

They could for example remove objects when really all you want to allow is add(). They could modify it from different threads, making your code break in interesting and frustrating ways. They could even cast it to a raw List and fill it with entirely different types of objects, making your code throw ClassCastExceptions.

In other words, it makes it impossible to enforce class invariants.

Note that there are two things that work together to cause this problem:

1. The exposure of an object stored in a field.
2. And the fact the object is mutable.

If any of those two aren't true, there's no problem. So this is fine:

public String getFoo() {
   return this.foo;
}

Because String is immutable. And this is fine too:

public List<String> getFooList() {
   return new ArrayList<>( this.fooList );
}

Because now you're returning a defensive copy and not the actual object. (However, if the elements of the lists were mutable, you'd be in trouble again.)

---

There is a more subtle variation to this problem...

Imagine this scenario:

public class Foo { 
   private List<String> list;
   public Foo( List<String> list ) {
     this.list = list; // Don't do this
   }
   ...
}

This looks perfectly harmless, and you see it in many places. However there is a hidden catch here too: by not making a copy before storing the list, you're in exactly the same situation. You can't stop someone from doing this:

List<String> list = new ArrayList<>();
list.add( "nice item" );
Foo foo = new Foo( list );
list.add( "hahahaha" );
list.add( "i've just added more items to your list and you don't know about it." );
list.add( "i'm an evil genius" );

So you should be making defensive copies both before assigning a mutable object to a field and when returning it.

---

If it's so dangerous to expose mutable fields of a class, why don't people make defensive copies all the time then?

Apart from simply not knowing why it isn't a good idea, there are two broad categories of excuses given.

1. Performance. Making a copy of an object every time you call a getter is expensive. This is of course true but it's not always as expensive as you'd think. And if you find yourself paying too high a cost for defensive copies, there are usually ways out: for example by designing your classes to be immutable. If all your classes are immutable, you won't need defensive copies, ever.
2. Only I will call this code and I won't abuse it. Promise. This is again something that could be valid depending on the situation. If you're writing a small standalone application on your own, it's probably true that you won't abuse it. Or if you're writing a small library but you only expose details of classes that aren't part of the public API. In most other cases though you simply can't be sure that someone somewhere won't abuse it. And when that happens, your code probably won't break with a bang. It usually just starts doing slightly...odd things occasionally. And that's the worst kind of bug to try to find.

Answer 2

There are two things in your code:

1. Lazy initialization, which is a common approach.
2. Returning a reference to a mutable inner ArrayList, which is not fine, because it breaks encapsulation. If you would like to add items to this list, you should expose addItem() method only.