Access safety of getters in Java

Question

So, we created a simple class with some private class member and automatically generated getter for it. But getter actually returned a reference to that member, resulting in gaining full access to a private member. Is that okay? Here's the code of a class:

public class User {

    private ArrayList<String> strings = new ArrayList(){ {
            add("String1");
            add("String2");
        } };

    public User() {
    }

    public ArrayList<String> getStrings() {
        return strings;
    }

    public void setStrings(ArrayList<String> strings) {
        this.strings = strings;
    }
}

Code of main method:

    public class Main {
    public static void main(String[] args){
        User user = new User();

        System.out.println(user.getStrings());
        user.getStrings().add("String3");
        System.out.println(user.getStrings());
    }
}

And output:

[String1, String2]

[String1, String2, String3]

I've changed the getter to this one:

public ArrayList<String> getStrings() {
    return (ArrayList<String>)strings.clone();
}

But the question remains, what getters are for if not for safety? And what is the right way to write them?

Answer 1

No, it isn't okay because it breaks encapsulation and thus the class can't maintain its own invariants. Same with constructors.

But the problem isn't with getters/setters, it's with the code that autogenerates them.

To cut a long story short: don't use autogenerated accessors blindly, if they're dealing with mutable structures, make defensive copies (or immutable equivalents).

---

As an aside, I would not have a getter with an ArrayList return type, even if it's just a copy. It's usually none of the client's business what kind of list you're returning, so my getter would look like this:

public List<String> getStrings() {
    return new ArrayList<>(strings);
}

Or using an immutable view:

public List<String> getStrings() {
    return Collections.unmodifiableList(strings);
}

Or using Guava's ImmutableList class:

public List<String> getStrings() {
    return ImmutableList.copyOf(strings);
}

There are subtle differences between the three solutions so which one's best may vary. As a general rule I prefer returning immutable structures because that makes it clear that changes made to the structure won't be reflected, i.e. user.getStrings().add( "X" ); will fail with an exception.

---

Another subtle problem with the code you showed us is the double braces initialisation. Imagine a class like this:

public class Foo {
   private List<String> strings = new ArrayList() {{ add("bar");}};
   private Object veryLargeField; //the object stored here consumes a lot of memory

   public List<String> getStrings() {
     return strings;
   }
}

Now imagine we're doing this:

private class Bar {
   private List<String> fooStrings;

   public Bar() {
     this.fooStrings = new Foo().getStrings();
   }
}

How much memory would Bar consume (or to use the precise term: retain)? Well, it turns out that quite a lot, because what you do with the double brace initialisation is create an anonymous inner class, which will contain a reference to its outer class (Foo), and thus while the list returned is accessible, all the other fields of Foo will be ineligible for garbage collection.