views:

204

answers:

7

One of my classes has a field which contains a Set. This field is only ever filled in the constructor, and then read by other classes. Originally I had something like this:

public class Foo {
    public final Set<String> myItems;
    public Foo(Collection<String> theirItems) {
        this.myItems = new LinkedHashSet<String>(theirItems);
    }
}

But this goes against OO best practices, by which myItems should be private, and only accessed via setters and getters. So then I changed it to:

public class Foo {
    private final Set<String> myItems;
    public Foo(Collection<String> theirItems) {
        this.myItems = new LinkedHashSet<String>(theirItems);
    }
    public Set<String> getItems() {
        return myItems;
    }
}

Now myItems is private, but whoever calls getItems() can still add/remove items at will, which is essentially the same situation I had before. (I'm not actually concerned about somebody changing the item contents, this is more a theoretical question)

So then I changed getItems() to return an array:

public String[] getItems() {
    return myItems.toArray(new String[myItems.size()]);
}

Now my items are really private. Unfortunately, I know that the object which will read the items will actually want to work with a Set, so it would have to convert the array right back. I could also return a copy of myItems:

public Set<String> getItems() {
    return new LinkedHashSet<String>(myItems);
}

This gives the caller what they want, but creates a new Set on each access.

What do you do in a situation like this - preserve privacy at all costs, and accept the conversion/copying of the original structure, or sacrifice the control over the contents of the collection and rely on responsible callers?

A: 

I would clone the set to return it so that if the caller modified the set, it wouldn't affect your own set.

James Deville
And of course the caller should also clone the received collection, just in case the called forgot to do so... brave new world =)
Zed
A: 

I would go with your last option:

public Set<String> getItems() {
    return new LinkedHashSet<String>(myItems);
}
tster
+10  A: 

Return an unmodifiable view onto your set:

public Set<String> getItems() {
    return Collections.unmodifiableSet(myItems);
}

Note that that means the caller will still see any changes you make to the set, if they hang on to the returned set. If you don't want that, you'll have to make a copy... there's no (simple) way round that. (In theory you could make an unmodifiable copy and return a reference to that same copy until the next time you make a change, but that gets messy.)

One important point is to document whatever you choose so that the caller doesn't get any nasty surprises. In many ways I think that's actually the most important thing in most applications, where the caller isn't actually malicious. So long as it's clear what the effects will be, being defensive isn't quite as important in most cases. Of course, if your caller could be some untrustworthy code and your set is vital to security etc, you're in a different situation.

Jon Skeet
@tster: I was just editing in the "document it!" bit as you commented :)
Jon Skeet
haha, I was just deleting my comment as you commented.
tster
+8  A: 

It depends on the context. There are a few options:

  1. Return the collection. The caller can modify the collection as they wish, but they cannot assign a new collection. This is cheapest, but offers the least protection.
  2. Return a view (using unmodifiableXXX factories of the Collections class). The caller cannot modify the collection, but updates to the collection will be visible to the caller. This is usually relatively cheap because element storage isn't allocated; only a wrapper is created.
  3. Return a snapshot of the collection, using a copy constructor of the appropriate collection. Here, the caller gets a copy of the collection. They can modify the copy, but the original is not updated, and updates to the original are not visible in the copy. This is the most expensive.
erickson
+1  A: 

I either make a copy of the set, or I use Collections.unmodifiableSet().

If performance is an issue, I break the encapsulation rule and return the original set.

Maurice Perry
+3  A: 

How "safe" do you need to be? The array you return has references to the same objects that your set holds, if those objects have setters then you've allowed the caller to modify the contents of your set ... is that OK?

It would be possible to clone the set, or provide an immutable interface for accessing the set. It's a judgement call. If I were developing framework code I would tend to err on the side of safety and clone. I tend to be less conservative when the client is more tightly coupled, a related class in the same package, say.

djna
A: 

As an alternative to Collections.unmodifiableSet(java.util.Set) a project called Google Collections Library has com.google.common.collect.ImmutableSet.

Justus