0

I'm trying to create a new deep copy of a list, including the objects, but when I change the objects in the second list, the objects in the original list change too. I implemented the clone method, but this doesn't work at all.

I have an abstract class Person.

internal abstract class Person : ICloneable
{
    protected string Name;
    public int Age { get; set; }

    public Person(string name, int age)
    {
        Name = name;
        Age = age;
    }

    public void sayMyName()
    {
        Console.WriteLine($"My name is {Name}");
    }

    public abstract object Clone();
}

A child class also:

internal class Student : Person, ICloneable
{
    public int Grade { get; set; }

    public Student(string name, int age, int grade) : base(name, age)
    {
        Grade = grade;
    }

    public override object Clone()
    {
        return new Student(Name, Age, Grade);
    }
}

And this is the main method:

internal class Program
{
    static void Main(string[] args)
    {
        Person s1 = new Student("Andrew", 20, 7);
        Person s2 = new Student("John", 21, 8);
        Person s3 = new Student("George", 19, 9);
        List<Person> personList = new List<Person>();
        personList.Add(s1);
        personList.Add(s2);
        personList.Add(s3);

        for(int i = 0; i < 3; i++)
        {
            List<Person> secondList = new List<Person>(personList);
            foreach(Person person in secondList)
            {
                person.sayMyName();
                Console.WriteLine(person.Age);
                person.Age--;
            }
            Console.WriteLine();
        }
    }
}

When I change the age of the persons in the second list, they also change in the original list. I implemented the clone method, why is this happening?

  • 1
    One approach for deep copy could be serialize -> deserialize – Renat Oct 11 '22 at 19:19
  • 7
    You're never using your clone method. Do you just expect the compiler to automatically know you want to clone it? The most common way to do this is [copy constructors](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/how-to-write-a-copy-constructor). – Jesse Oct 11 '22 at 19:19
  • So the only way to do that is manually? – Foreastbtch Oct 11 '22 at 19:32
  • 1
    Not the only way, but it is the most common because you have the most control over it. Another approach would be to serialize it to something then deserialize it like Renat said (like JSON or binary). This is slower and more intensive than just chaining copy constructors though. – Jesse Oct 11 '22 at 19:38

2 Answers2

2

When you copy a list (new List<Person>(personList)), you're only creating a new list with all the same objects in it. No new objects are getting created. I think you're intending to do this:

var secondList = personList.Select(p => (Person)p.Clone()).ToList();
// reduce ages in second list
foreach (Person person in secondList)
{
    person.Age--;
    person.sayMyName();
    Console.WriteLine(person.Age);
}
// ages from first list are not affected
foreach (Person person in personList)
{
    person.sayMyName();
    Console.WriteLine(person.Age);
}

For what it's worth, there are a lot of problems with the Clone concept, and most people find it to be the wrong approach, especially when inheritance is involved. You might be better off using records, which are by default immutable and put up some guard rails to avoid some of the common issues with cloning. Plus they add some features to make transformations easier.

internal record Person(string Name, int Age){

    public void sayMyName()
    {
        Console.WriteLine($"My name is {Name}");
    }

}

internal record Student(string Name, int Age, int Grade) : Person(Name, Age)
{
}
var youngerPeople = personList.Select(p => p with { Age = p.Age - 1}).ToList();
// The new list is also full of `Student`s, with reduced ages.
foreach (Person person in youngerPeople)
{
    person.sayMyName();
    Console.WriteLine(person.Age);
    Console.WriteLine(person.GetType().Name);
}
// The original list is untouched.
foreach (Person person in personList)
{
    person.sayMyName();
    Console.WriteLine(person.Age);
    Console.WriteLine(person.GetType().Name);
}
StriplingWarrior
  • 151,543
  • 27
  • 246
  • 315
-1

The most common way is to use a copy constructor.

A copy constructor is exactly what it sounds like, it's a constructor that copies the contents of an object to a new instance. Using a record will automatically create a copy constructor so you don't have to do it manually, but in this answer I will implement them manually as to not deviate too much from what you have, and to better show what exactly they do. Applying this to your code would look something like this:

internal class Person
{
    protected string Name;
    public int Age { get; set; }

    // Instance constructor
    public Person(string name, int age)
    {
        Name = name;
        Age = age;
    }

    // Copy constructor
    public Person(Person other)
    {
        Name = other.Name;
        Age = other.Age;
    }
}

internal class Student : Person
{
    public int Grade { get; set; }

    // Instance constructor
    public Student(string name, int age, int grade) : base(name, age)
    {
        Grade = grade;
    }
    
    // Copy constructor
    public Student(Student other) : base(other)
    {
        Grade = other.Grade;
    }
}

Another less manual (but more resource heavy) way is to serialize the object to something (JSON in this example) then deserialize it to a new object. For this example I'm using the Newtonsoft.JSON (aka Json.NET) library.

public static T Copy<T>(T obj)
{
    var json = JsonConvert.SerializeObject(obj);
    return JsonConvert.DeserializeObject<T>(json);
}

This method can be used with your classes like so:

var student = new Student("Bob", 15, 10);
var newStudent = Copy(student);

In order to copy an entire list, you can mix in a bit of linq magic.

With copy constructors:

var newList = personList
    .Select(person => 
        person is Student student ? 
            new Student(student) : 
            new Person(person)
    ).ToList();

And with the copy method:

var newList = personList
    .Select(person => 
        person is Student student ? 
            Copy(student) : 
            Copy(person)
    ).ToList();

Note that with both of these methods, since the list is a list of Person, you need to figure out if each person is an instance of Student and cast it first in order for the Grade property to be copied over and not just be set to 0.

Jesse
  • 1,386
  • 3
  • 9
  • 23