What's the best practice for returning an internal collection in Java?

Question

I'm curious what is considered a better practice when returning a collection of objects from a mutable class:

public class Someclass {

  public List<String> strings;  

  public add(String in){ strings.add(in); }
  public remove(String in) { strings.remove(in); }   

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

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

I always assumed wrapping the internal collection in an Unmodifiable was the best approach since I didn't see a reason to incur the overhead of creating a copy. However, it occurred to me that any changes made to the list internally would be exposed to the client which strikes me as bad.

Stephen C · Accepted Answer · 2011-05-13T16:14:44.410

I don't think there is a simple "best practice" answer to this. It really depends on how much of the machine's resources you are willing to spend on preventing violation of the classes data abstraction boundaries. And it depends on what risks you are actually trying to mitigate; e.g. is it simple (non-concurrent) bugs, concurrency bugs, or information leaks.

The options are:

Do nothing; i.e. return the collection as-is.
Return the collection wrapped in an immutable wrapper class.
Return a shallow copy of the collection.
Return a deep copy of the collection.

In addition to the direct costs of copying, other performance-related factors are memory usage and garbage generation and the impact on concurrency. For instance, if multiple threads are updating the collection and/or "getting" it, then making a copy of the collection typically involves locking it ... which could potentially make the operation a concurrency bottleneck.

In summary, you need to balance the cost / performance implications against the potential or actual risks and costs of not taking the precautions.

score 2 · Answer 2 · answered May 13 '11 at 01:04

2

I like to do the latter, but using the .clone() method.

answered May 13 '11 at 01:04

V a a m Y o b

492
3
7

The latter approach, using new ArrayList, or using .clone(), is the right approach to reduce concurrency issues. Collections.unmodifiableList() increases possible concurrency issues in a multi-threaded environment. – Julius Musseau May 13 '11 at 01:15

score 0 · Answer 3 · answered May 13 '11 at 01:04

0

If you are concerned about exposing changes; and your object implements Cloneable, then you can return a Cloned deep copy of your object list back to the invoker.

answered May 13 '11 at 01:04

Basanth Roy

6,272
5
25
25

score 0 · Answer 4 · answered May 13 '11 at 06:22

'new ArrayList(strings)' is safer than 'Collections.unmodifiableList(strings)'.

Because Collections.unmodifiableList just forwards the source list.

Sample:

List<String> list1 = someclass.getStrings(); //Use Collections.unmodifiableList(strings) implementation
List<String> list2 = someclass.getStrings(); //Use new ArrayList(strings) implementation

someclass.add("foo");

Now, you'll see list1 is added! list2 not.

What's the best practice for returning an internal collection in Java?

4 Answers4