CODEWITHSUNDEEP
javaobjectconstructorcopy
iaacp
iaacp

Reputation: 4845

Copying an Object in Java without affecting the original via copy constructor

I'm trying to copy an Object, which will then be modified, without changing the original object.

I found this solution and it seemed the best approach would be a copy constructor - from my understanding, this would give me a deep copy (a completely separate object from the original).

So I tried that. However, I've noticed that when the following code executes, it's effecting all previous objects from which it was copied. When I call surveyCopy.take(), which will change values inside of the Survey, it's also changing the values inside of selectedSurvey. 

public class MainDriver {
...
//Code that is supposed to create the copy
case "11":  selectedSurvey = retrieveBlankSurvey(currentSurveys);
            Survey surveyCopy = new Survey(selectedSurvey);
            surveyCopy.take(consoleIO);
            currentSurveys.add(surveyCopy);
            break;
}

and here is code for my copy constructor:

public class Survey implements Serializable
{
    ArrayList<Question> questionList;
    int numQuestions;
    String taker;
    String surveyName;
    boolean isTaken;

    //Copy constructor
    public Survey(Survey incoming)
    {
        this.taker = incoming.getTaker();
        this.numQuestions = incoming.getNumQuestions();
        this.questionList = incoming.getQuestionList();
        this.surveyName = incoming.getSurveyName();
        this.isTaken = incoming.isTaken();
    }
}

So what exactly is the issue? Does a copy constructor not work that way? Is the way I coded it wrong?

Upvotes: 5

Views: 13658

Answers (4)

DhineshYes
DhineshYes

Reputation: 1088

If we need to copy a simple pojo (Not nested). Then shallow copy is enough.

Cloner class

import java.lang.reflect.Field;

public class Cloner {
    public static <T> T cloneShallow(T srcEntity, T destEntity){
        try {
            return copy(srcEntity, destEntity);
        }catch (Exception e){
            e.printStackTrace();
        }
        return null;
    }

    private static <T> T copy(T srcEntity, T destEntity) throws IllegalAccessException, InstantiationException {
        if(srcEntity == null){
            return null;
        }

        Class<?> clazz = srcEntity.getClass();

        T newEntity;

        if(destEntity != null){
            newEntity = destEntity;
        }else{
            //create new instance
            newEntity = (T) srcEntity.getClass().newInstance();
        }

        while (clazz != null) {
            copyFields(srcEntity, newEntity, clazz);
            clazz = clazz.getSuperclass();
        }

        return newEntity;
    }

    private static  <T> T copyFields(T entity, T newEntity, Class<?> clazz) throws IllegalAccessException {
        for (Field field : clazz.getDeclaredFields()) {
            field.setAccessible(true);
            field.set(newEntity, field.get(entity));
        }
        return newEntity;
    }
}

Lets call..

eg.
Apple apple = new Apple();
apple.setColor("Green");

Apple newApple = Cloner.cloneShallow(apple, new Apple());
( or )
Apple newApple = Cloner.cloneShallow(apple, null);

Upvotes: 0

Jack
Jack

Reputation: 133629

The problem when creating deep copies is the fact that everything that is not a primitive type is copied by reference unless you are using a specific deep copy constructor on it too.

In your specific case you have no problems with bool, int or String variables because you pass them around by value (actually String is passed by reference but it's immutable so there is no problem) but you are passing a ArrayList<Question> questionList. When you do

this.object = incoming.object

you just copy a reference. So both variables point to the same object in memory, so you are not deeply copying it. You must create another instance of the object that has the same inner values, then you will be sure, eg this.object = new YourObject(incoming.object).

Mind that usually means that more is your class complicated in the composition tree, more you will have to go deep in the variables until you copied them all.

Upvotes: 5

Brian Agnew
Brian Agnew

Reputation: 272417

This

this.questionList = incoming.getQuestionList();

most likely copies the reference to the original list (I say probably since it's possible that getQuestionList() gives you a defensive copy). You will probably have to make a new copy of that list. And perhaps the contained Question objects. And perhaps anything that they reference.

This is the problem with deep copies. In order to do this reliably you have to copy all mutable objects. Note that if an object is immutable (e.g. the Strings) then they can't be changed and so you can reference the originals confident that they won't be changed. The same applies to primitives. One good reason for encouraging immutability in your code base.

If you can't make an immutable class, write your class such that it makes defensive copies. i.e. when a client asks it for a collection, it should make a copy and return that. Otherwise your supposedly well-meaning clients could alter your internal state (inadvertently or otherwise).

Upvotes: 8

Jon Skeet
Jon Skeet

Reputation: 1503409

This is the problem, in your copy constructor:

this.questionList = incoming.getQuestionList();

That's just copying the reference to the list. Both objects will still refer to the same object.

You can use:

this.questionList = new ArrayList<Question>(incoming.getQuestionList());

to create a copy of the original list - but this is still not good enough if Question itself is mutable. In that case, you'd have to create a copy of each Question object to achieve total isolation.

Your other fields are okay, as they're either primitives or references to String (which is immutable, allowing you to share references safely).

Upvotes: 14

Related Questions