CODEWITHSUNDEEP
javaclone
Alexandru Barbarosie
Alexandru Barbarosie

Reputation: 3002

Why clone() doesn't work as expected?

Having the following code in java

public Polynomial multiply(Polynomial aPolynomial){
    Polynomial ret = new Polynomial();
    for (Tuple i: this.pol){
        Polynomial temp = aPolynomial.clone();
        System.out.print(temp);
        for (Tuple j: temp.pol){
            j.setCoef(j.getCoef() * i.getCoef());
            j.setPower(j.getPower() + i.getPower());
        }
        ret = ret.add(temp).clone();
    }       
    return ret;
}

I get as output for System.out.print(temp)always different values. This means that aPolynomialget's altered somewhere during runtime.

Changing Polynomial temp = aPolynomial.clone(); to:

LinkedList<Tuple> list1 = (LinkedList<Tuple>) aPolynomial.pol.clone();
Polynomial temp = new Polynomial(list1);

doesn't help either the output for System.out.print(temp) being also different on every run of the loop.

Where is my mistake?

Edit:

public Polynomial clone() {
    try {
        return (Polynomial)super.clone();
    } catch (CloneNotSupportedException e) {
        e.printStackTrace();
    }
    return null;
}

printing the hashCode() of temp and aPolynomial does result in two different values.

aPolynomial does have the same hashCode on every run of the for loop.

answers to some question in the comments:

since Polynomialdoesn't inherit from anywhere, as far as I ma concerned super.clone() will refer to Object

I do have my own toString method.

Upvotes: 5

Views: 2127

Answers (2)

Mateusz Dymczyk
Mateusz Dymczyk

Reputation: 15141

I am pretty sure that Java's Object#clone() does a shallow copy by default. Meaning it will create a new instance of your object (so "x.clone() != x") but the member fields will be directly copied. This means that if you have references as members the addresses will be copied and not the value. Therefore you have a clone which internally points to the same objects. When you change the state of objects in the clone it is equal to changing the state of the objects in the original.

You should do a deep copy() which will also create a new instance of your members (Tuples I guess in your case) so they don't point to the same objects. This you have to implement yourself.

That being said using clone() is regarded as bad practice and you shouldn't do it (why, you can Google it, especially look for Bloch's clone() vs copy constructor). Besides copy constructors (so a constructor that takes an argument of the same type as the object it constructs and uses it as a template) there are also special deep copying libraries. Or you can go with serializing your objects and copying them that way but that's only if you like the hard way.

@Edit: here's some interview where he speaks a bit about it http://www.artima.com/intv/bloch13.html. For his full text you would need to refer to Effective Java 2nd Edition

Upvotes: 4

nemesisx00
nemesisx00

Reputation: 2066

I believe the problem you are experiencing is this: You verified that temp, the clone of aPolynomial, is actually a separate object. But the values you are using from that object are in the reference temp.pol. During the clone method, that reference is being copied over to the new Polynomial instance, so that both aPolynomial and temp refer to the same object with their pol member.

The easiest solution would be to implement a custom clone() method where you also clone the pol reference to the new instance of Polynomial.

Something along the lines of:

public Polynomial clone() {
    try {
        Polynomial p = (Polynomial)super.clone();
        p.pol = this.pol.clone();
        return p;
    } catch (CloneNotSupportedException e) {
        e.printStackTrace();
    }
    return null;
}

Upvotes: 7

Related Questions