What's the preferred way to assign a collection from a parameter?

I have this class:

public MyClass {
    public void initialize(Collection<String> data) {
        this.data = data; // <-- Bad!
    }
    private Collection<String> data;
}

This is obviously bad style, because I'm introducing a shared mutable state. What's the preferred way to handle this?

• Ignore it?
• Clone the collection?
• ...?

EDIT: To clarify why this is bad, imagine this:

MyClass myObject = new MyClass();
List<String> data = new ArrayList<String>();
myObject.initialize(data); // myObject.data.size() == 0
data.add("Test"); // myObject.data.size开发者_开发技巧() == 1 

Just storing the reference poses a way to inject data into the private field myObject.data, although it should be completely private.

Depending on the nature of MyClass this could have serious impacts.

The best way is to deep clone the parameter. For performance reasons, this is usually not possible. On top of that, not all objects can be cloned, so deep copying might throw exceptions and cause all kinds of headache.

The next best way would be a "copy-on-write" clone. There is no support for this in the Java runtime.

If you think that it's possible someone mutates the collection, do a shallow copy using the copy constructor:

this.data = new HashSet<String> (data);

This will solve your problem (since String is immutable) but it will fail when the type in the set is mutable.

Another solution is to always make the sets immutable as soon as you store them somewhere:

Set<String> set = ...
...build the set...

// Freeze the set
set = Collections.unmodifiableSet(set);

// Now you can safely pass it elsewhere
obj.setData (set);

The idea here is turn collections into "value objects" as soon as possible. Anyone who wants to change the collection must copy it, change it and then save it back.

Within a class, you can keep the set mutable and wrap it in the getter (which you should do anyway).

Problems with this approach: Performance (but it's probably not as bad as you'd expect) and discipline (breaks if you forget it somewhere).

• Null check (if you want to restrict null)
• Either defensive copy (if you don't want shared state)
• or as you did (if a live view on data is useful)

Depends heavily on your requirements.

Edited: Ignoring should be no option. Silent fail is, well... a debugging nightmare.

public class Foo {
    private final Collection collection = new ArrayList();
    public void initialise(final Collection collection) {
        this.collection.addAll(collection);
    }
}

Sorry for not addressing your concern directly, but I would never directly pass a Collection to a setXxx() bean setter method. Instead, I would do:

private final List<MyClass> theList;

public void addXxx(MyClass item) { ... }
public void removeXxx(MyClass item) { ... }  // or index.

public void Iterator<MyClass> iterateXxx() {
  return Collections.unmodifiableList(theList).iterator();
}

I would go for defensive copying / deep cloning only if I am sure there would be no side effects from using it, and as for the speed, I wouldn't concern myself with it, since in business applications reliability has 10 times more priority than speed. ;-)

An idea will be to pass the data as a String array and create the Set inside MyClass. Of course MyClass should test that the input data is valid. I believe that this is a good practice anyway.

If both the caller of MyClass and MyClass itself actually work with a Set<String>, then you could consider cloning the collection. The Set however needs to be constructed somehow. I would prefer to move this responsibility to MyClass.