Java deep copy - wrong reference assignment

Question

Please consider the following code

public class Pair implements Cloneable, Comparable<Para> {

    ...

    public Pair(String key, double value) throws Exception {
        if (value <= 0) throw new IllegalArgumentException();

        this.key = key;
        this.value = value;
    }

    ...

    @Override
    public Pair clone() throws CloneNotSupportedException {
        return (Pair) super.clone();
    }
}

and

public class BSTtree implements Dictionary, Cloneable {

    ...

    private class Node implements Cloneable, Comparable<Node> {
        Para elem;
        Node left = null;
        Node right = null;
        Node parent = null;

        Node () {
            this.elem = null;
        }

        Node (Pair elem) {
            this.elem = elem;
        }

        ...

        @Override
        public Node clone() throws CloneNotSupportedException {
            Node cloned = new Node();

            cloned.elem = elem.clone();
            if (left != null) cloned.left = left.clone();
            if (right != null) cloned.right = right.clone();

            return cloned;
        }
    }

    ...

    @Override
    public BSTtree clone() throws CloneNotSupportedException {
        BSTtree cloned = new BSTtree();
        cloned.root = root.clone();

        return cloned;
    }
}

I'm trying to implement my own BST tree having Cloneable interface. Where I have a problem is

BSTtree alfa = new BSTtree();
...
BSTtree beta = alfa.clone();

which I assume don't correctly set references to trees alfa and beta. Why do I think so? Take look at following

alfa.printTree(); //(VOID  1,00), (a  1,00), (b  2,00), (c  3,00), (e  5,00)
beta.printTree(); //(VOID  1,00), (a  1,00), (b  2,00), (c  3,00), (e  5,00)

beta.remove("a");

alfa.printTree(); //(VOID  1,00), (b  2,00), (c  3,00), (e  5,00)
beta.printTree(); //(VOID  1,00), (a  1,00), (b  2,00), (c  3,00), (e  5,00)

alfa.remove("e");

alfa.printTree(); //(VOID  1,00), (b  2,00), (c  3,00), (e  5,00)
beta.printTree(); //(VOID  1,00), (a  1,00), (b  2,00), (c  3,00), (e  5,00)

So you can see after cloning any removing from beta in fact removes from alfa, and removing from alfa does not remove at all. Of course before calling clone() every operation on alfa worked correctly.

This is for learning and the main task is to implement a working clone() method, so I don't want to use any other ways to perform a deep copy.

Please advise what am I doing wrong, as self-debugging haven't helped with it yet.

Implementing `Cloneable` is meaningless if you are not calling up to `Object.clone()`. — newacct, Oct 25 '14 at 23:49

Stefan Haustein · Accepted Answer · 2014-10-25T18:59:35.673

2

The parent update seems to be missing -- not sure if this is the only issue...

    @Override
    public Node clone() throws CloneNotSupportedException {
        Node cloned = new Node();

        cloned.elem = elem.clone();
        if (left != null) {
           cloned.left = left.clone();
           cloned.left.parent = cloned;
        }
        if (right != null) {
           cloned.right = right.clone();
           cloned.right.parent = cloned;
        }

        return cloned;
    }

P.S. I think the other issue is that the Node is a non-static inner class. So it always has an implicit pointer to the tree it was created in. Where you call new Node() in Node.clone, this is in the scope of the original tree, so it will still point to the original tree, but it needs to be an inner class of the new tree. Adding static to the inner class declaration will highlight the problem.

You may want to turn it into a static inner class and add an explicit pointer to the tree (if you need it -- some of the Node methods accessing root look like they should really be BSTtree methods).

edited Oct 25 '14 at 18:59

answered Oct 25 '14 at 15:36

Stefan Haustein

18,427
3
36
51

That's right, I forgot to update the parents. Nevertheless the main issue still exists, so there are more problems with my code. – Mateusz Kowalski Oct 25 '14 at 15:43
Does BSTTree have more members than root? I think the error may not be in the fragment you have posted... – Stefan Haustein Oct 25 '14 at 16:49
Yes, it does. Here is a full code of BSTtree class - http://pastebin.com/zDxJKUBV, and Pair class - http://pastebin.com/hNCQYpge – Mateusz Kowalski Oct 25 '14 at 17:10
I didn't find any other fields in BSTtree, but some of the Node methods seem to access the field root of the outer class. And the outer class can't be adjusted in clone() -- nodes will retain the root reference of their owner. I'd turn Node into a static inner class and fix the fallout. – Stefan Haustein Oct 25 '14 at 17:46
Sorry, but I don't get the _nodes will retain the root reference of their owner_ fragment - in `public BSTtree clone()` I clone root of the tree into `new BSTtree()`, so at this moment new tree has its own root, but the sons are shared with the old's one. To adjust them, I call `public Node clone()` via `root.clone()` which adjusts left son, right son, and so on. In the end I have the root cloned and all it's descendants adjusted correctly. Where is a bug in this reasoning? – Mateusz Kowalski Oct 25 '14 at 18:39
1

All the nodes hold an implicit reference to the BSTrree because they are non-static inner classes. Where you call new Node() in Node.clone, this is in the scope of the original tree, so they will still point to the original tree, but they need to point to the new tree. Adding static to the inner class declaration will highlight the problem. – Stefan Haustein Oct 25 '14 at 18:48
Okay, got it. But making `Node` class static will deny to use any references to `root` inside this class. First solution coming to my mind in this situation will be to move most of the methods from `Node` to `BSTtree` class. Is it a good way, or is there a more elegant solution? – Mateusz Kowalski Oct 25 '14 at 18:55
Yes, I think that's where they really belong. – Stefan Haustein Oct 25 '14 at 18:56

Java deep copy - wrong reference assignment

1 Answers1