The copy constructor creates dependent copy

Question

I implemented the copy constructor as it is described here. But still the problem is that when I update route_copy, then the same update is applied to route. So, I don't understand what is wrong in my code?

public class Route implements Comparable<Route> {
    private List<Site> sites;

    public Route()
    {
        sites = new ArrayList<Site>();
    }

    public Route(List<Site> sites)
    {
        this.sites = sites;
    }

    /**
     * Copy constructor
     */
    public Route(Route r) {
        this(r.sites);
    }

    public void deleteSite(Site s) {
        this.sites.remove(s);
    }
}

public processData(Route route)
{
  Route route_copy = new Route(route);
  Site s = selectSite(route_copy);
  route_copy.deleteSite(s); // !!! now 'route' does not contain an  element 's'
}

Your "copy constructor" is not making a copy of the input list. Try something like — Alexandre Dupriez, Jul 17 '13 at 14:52
Java doesn't really support copy constructors to the same degree as C++ i.e. automatic invocation. Wouldn't overriding `clone()` be more appropriate? — DuncanACoulter, Jul 17 '13 at 14:56
@DuncanACoulter: I had the same problem after overriding the clone method and implementing the class Route as Cloneable. — Klausos Klausos, Jul 17 '13 at 14:57
@KlausosKlausos Just as well probably, some searching has revealed that the current consensus is to [avoid clone](http://www.artima.com/intv/bloch13.html) — DuncanACoulter, Jul 17 '13 at 15:00

score 5 · Accepted Answer · edited May 23 '17 at 10:25

5

In your copy constructor, you are just doing a shallow copy, while you need to do a deep copy:

public Route(Route r) {
    this(r.sites);
}

Here, you are still copying the reference of the list, which still points to the same ArrayList. You should modify it to create copy of list too. Possibly, you also need to create copy of the elements inside the arraylist like so:

public Route(Route r) {
    List<Site> newSites = new ArrayList<Site>();

    for (Site obj: r.sites) {
        // Add copy of obj to the newSites
        // So you need yet another copy constructor in 'Site' class.
    }

    this.sites = newSites;
}

Check this post - Shallow Copy vs Deep Copy.

edited May 23 '17 at 10:25

Community

1
1

answered Jul 17 '13 at 14:52

Rohit Jain

209,639
45
409
525

1

This won't compile. `this(...)` must be the first statement in a constructor. – DannyMo Jul 17 '13 at 14:54
1

@damo. OOPs. Fixed. Thanks :) – Rohit Jain Jul 17 '13 at 14:54
ITSPoI will still referencing to the same ITSPoI in the 2 list, if you mutate object in the list will be reflected, still a shallow copy – nachokk Jul 17 '13 at 14:55
you can serialize and deserealize the list to make a deep copy too, if the list is serializable – nachokk Jul 17 '13 at 15:03
I think it depends on the intent. If `Site` isn't _owned_ by the `Route`, you wouldn't want to create multiple instances of the same site... – DannyMo Jul 17 '13 at 15:05
@damo. Yeah that depends. I would let OP to decide onto that. – Rohit Jain Jul 17 '13 at 15:08

score 2 · Answer 2 · answered Jul 17 '13 at 14:52

2

Your "copy constructor" is not making a copy of the input list. Try something like

public Route(List<Site> sites)
{
    this.sites = new ArrayList<Site>(sites);
}

for your second constructor.

answered Jul 17 '13 at 14:52

Alexandre Dupriez

3,026
20
25

Will this create a completely independent copy? – Klausos Klausos Jul 17 '13 at 14:59
It will create a copy of the ArrayList, but not of the elements hold by the list itself (the Site objects). – Alexandre Dupriez Jul 17 '13 at 15:17
1

@KlausosKlausos ... which means that while `copyList.remove()` would not affect the original list; `copyList.get(i).setSomething("")` would. So, it depends on how you're going to use the copy. – Ravi K Thapliyal Jul 17 '13 at 15:22

score 2 · Answer 3 · answered Jul 17 '13 at 14:53

2

Ofcourse it'll create dependent copy also called Shallow copy.

You need a Deep copy.

answered Jul 17 '13 at 14:53

VishalDevgire

4,232
10
33
59

score 2 · Answer 4 · answered Jul 17 '13 at 14:53

The problem is that both lists are still pointing to the same memory location, thus, any operation on one list eventually modifies the other.

You can try and use the copy constructor of the ArrayList:

public ArrayList(Collection c)

Constructs a list containing the elements of the specified collection, in the order they are returned by the collection's iterator.

Like so:

     public Route(Route r) {
     this(new ArrayList<Site>(r.sites));
     }

Note however that doing any modifications to the Site objects within the list might have repurcussions on the other objects stored in the other list, depending on how complex your Site object is.

score 1 · Answer 5 · answered Jul 17 '13 at 14:52

what you do with the copy constructor is just make the new Route use the list of the old Route and therefor any change to one of them immediately effect the other

what you need to do is make the copy constructor make a new list:

sites = new ArrayList<Site>(oldList);

The copy constructor creates dependent copy

5 Answers5

Linked

Related