Java copy constructor and clone doesnt work

Question

public class Stav implements Cloneable{
private int[] pole;

public Stav(int[] pole){
    this.pole = pole;
}
public Stav(Stav a){
    this.pole = a.pole;
}
public void move(boolean left){
    int empty = findEmpty();
    if(left){
        this.pole[empty] = this.pole[empty - 1];
        this.pole[empty - 1] = 0;
    }
    else{
        this.pole[empty] = this.pole[empty + 1];
        this.pole[empty + 1] = 0;
    }
}

and

Jednotka pom = fronta.remove();
Stav nStav = new Stav(pom.getStav().getPole());
Stav pomStav = new Stav(nStav);

and when i call

pomStav.move(false);

the value of nStav will change same as pomStav... somebody can help me with this? I have some more metods in code but its so long so I didnt copy here like findEmpty etc.

You are not calling the clone() method anyware, so you are not cloning anything. — MTilsted, Nov 03 '14 at 23:23
Read this: http://stackoverflow.com/questions/40480/is-java-pass-by-reference-or-pass-by-value — Floegipoky, Nov 03 '14 at 23:23
`public Stav(Stav a) { int[] b = new int[a.pole.length]; System.arraycopy(a.pole, 0, b, 0, a.pole.length); this(b); }` — Mr. Polywhirl, Nov 03 '14 at 23:24

score 1 · Answer 1 · answered Nov 03 '14 at 23:26

1

public Stav(Stav a){
    this.pole = a.pole;
}

When you call this, the underlying int[] pole is shared between the two instances as you are copying the reference to the object. Instead of copying the reference, create a new array and copy the values:

public Stav( Stav a )
{
    this.pole = new int[ a.pole.length ];
    for( int i = 0; i < this.pole.length; ++i )
        this.pole[ i ] = a.pole[ i ];
}

answered Nov 03 '14 at 23:26

clcto

9,530
20
42

2

Since arrays implement `clone()` in a useful manner, I prefer to use that to copy them. If you do it manually, though, then at least you should copy the elements via `System.arraycopy()` rather than via an explicit loop. – John Bollinger Nov 03 '14 at 23:29
1

Rather than manally copying the values, it'd be better to use System.arraycopy or Arrays.copyOf. – Sbodd Nov 03 '14 at 23:29

score 0 · Answer 2 · answered Nov 03 '14 at 23:27

If you want your Stav instances to be independent of each other, then you have a problem here

public Stav(Stav a){
    this.pole = a.pole;
}

... and here

Stav nStav = new Stav(pom.getStav().getPole());

... or alternatively, here

public Stav(int[] pole){
    this.pole = pole;
}

. The effect is that you are giving your Stav instances references to the same array for their pole members. If you want them to be independent then you must give them copies of the array. One way to do that would be to write your constructors like so:

public Stav(int[] pole){
    this.pole = ((pole == null) ? null : pole.clone();
}
public Stav(Stav a){
    this(a.pole);
}

score 0 · Answer 3 · answered Nov 03 '14 at 23:29

Your copy constructor (and your default "clone" method) are doing a shallow copy:

this.pole = a.pole;

After the above statement, both this.pole and a.pole are references to the same array. If you want to do a slightly-deeper copy, you could do

this.pole = Arrays.copyOf (a.pole, a.pole.length);

After this operation, changes to the arrays would not be propagated between instances - though changes to the contents of the arrays would be if they were non-primitive arrays.

If you want your object to also do a deep copy when cloned, you need to override the clone() method:

public Object clone() {
  return new Stav(this);
}

(Do note that cloning has a number of problems - I highly recommend reading the chapters on cloning in Effective Java if you're going to be doing significant work involving cloning.

Java copy constructor and clone doesnt work

3 Answers3