Method runs for all instances of a class

Question

I ran across this problem, which has been driving me nuts. In a nutshell, I instantiate two objects of the same class. When I run a method in one object, the other object is affected too as if I called a method on a 2nd object explicitly. I was wondering if someone could please give me a hand on this.

Suppose, I have class Portfolio...

public class Portfolio implements Cloneable {

public ArrayList<Instrument> portfolio;
private String name;
private String description;

public Portfolio() {
    portfolio = new ArrayList<Instrument>();
}

public Portfolio(Portfolio copyFrom) {
    //attempt to copy the object by value
    this.portfolio = (ArrayList<Instrument>) copyFrom.portfolio.clone();
    this.name = copyFrom.name;
    this.description = copyFrom.description;
}

public void applyStress(Stress stress) {
    this.portfolio.set(0,this.portfolio.get(0)+1;
}

1st constructor is used to instantiate an object, etc. 2nd constructor is used to copy an object by value.

A method applyStress is used to run through sum calculations. in our case I simplified the method, so that it does nothing but adds +1 to whatever is in the object.

So I would instantiate an object as

Portfolio p = new Portfolio();

then I would assign to a portfolio field, some instruments;

p.portfolio = someInstrumentList;

then I would copy by value the portfolio p into pCopy:

Portfolio pCopy = new Portfolio(p);

So at this time I am have two objects that are the same. Further one is a copy-by-value object. Changing values of fields in pCopy does not affect same fields in p.

Now, when I run a method applyStress on p, then the values of the instrument list in pCopy will also change.

In other words, if p.portfolio.get(0) == 1, then after p.applyStress, I would expect to see that p.portfolio.get(0) is 2 and pCopy.portfolio.get(0) is 1

But what I see instead is p.portfolio.get(0) is 2 and pCopy.portfolio.get(0) is also 2

I do not understand why this happens. It is not the static modifier issue, as there is no static modifiers. Anyone has any ideas?

Answer 1

The clone method applied to you your ArrayList reference does a shallow copy, not a deep copy. This implies that whatever you had in the original collection is shared by the cloned one.

This means that you need to clone every instrument as well, or provide a copy constructor for every one of them.

this.portfolio = new ArrayList<Instrument>();
for(Instrument toBeCopiedInstrument : copyFrom.portfolio){
   this.portfolio.add(new Instrument(toBeCopiedInstrument ));
}

Answer 2

By default .clone() does what is called a shallow copy, this means it just copies a reference to the objects that are held in the List that is being cloned, it doesn't actually copy the objects themselves to new instances.

What you need to do is implement a custom deep copy for the List and each of the items held in the list. But deep clone is a broken concept and implementation in Java.

A copy constructor isn't a really good pattern in Java either, because you end up copying references as well in most cases and every object you inject to the constructor has to follow the same copy constructor semantics all the way down the chain. Unlike C++, this is manual, tedious, unmaintainable and error prone process!

.clone() and implements Cloneable are some of the most complex to get correct concepts in Java. They are rarely needed in well designed applications. That is, if you are using .clone() you are probably doing it wrong. If making bit wise copies of your objects are part of your design for something other than storage, you might want to revisit your design.

Josh Bloch on Design

Object's clone method is very tricky. It's based on field copies, and it's "extra-linguistic." It creates an object without calling a constructor. There are no guarantees that it preserves the invariants established by the constructors. There have been lots of bugs over the years, both in and outside Sun, stemming from the fact that if you just call super.clone repeatedly up the chain until you have cloned an object, you have a shallow copy of the object. The clone generally shares state with the object being cloned. If that state is mutable, you don't have two independent objects. If you modify one, the other changes as well. And all of a sudden, you get random behavior.

Immutable

A better pattern is to make everything immutable. That way you don't need separate instances, you can share instances until they need to change, then they change and you have a new instance with the new data, that can be shared without any side effects.