Is there anything wrong with this IClonable implementation

Question

I haven't programmed in C# but my son asked me if there is anything wrong with this implementation

public class Person : A, IClonable {
....

    public object Clone() {
       return this;
    }
}

My gut feeling is that it is wrong because this Clone() method implementation does not return any new object. I think that the Clone() method should create a new object or call a method that creates a new object and then return it. That is what I said to my son, but not having done any C# programming I became uncertain. Could someone shed a bit light on this.

You're right: the method should create a new object. – sloth Mar 18 '14 at 08:54 — sloth, Mar 18 '14 at 08:54

score 4 · Accepted Answer · edited May 23 '17 at 11:43

My gut feeling is that it is wrong because this Clone() method implementation does not return any new object

That feeling does not deceive you. You need to create a new object if you want to create a copy of it. Otherwise it's just the same reference and this implemenntation is pointless and misleading.

Consider that your class has a StringProperty:

Person p1 = new Person{ StringProperty = "Foo" };
Person p2 = (Person)p1.Clone();
p2.StringProperty = "Bah";
Console.Write(p1.StringProperty); // "Bah"

You see that even if i change the property on p2 i also modify StringProperty of the other instance since it's actually the same.

So you need something like this:

public object Clone() {
    Person p2 = new Person();
    p2.StringProperty = this.StringProperty;
    // ...
    return p2;
}

Although i prefer to create a different method Copy instead since it's often not clear what Clone does. Even Microsoft recommends against implementing ICloneable.

Why should I implement ICloneable in c#?

score 3 · Answer 2 · answered Mar 18 '14 at 08:55

Interfaces are contracts. If your class implements ICloneable, it promises to:

Supports cloning, which creates a new instance of a class with the same value as an existing instance.

Now if the writer of Clone() { return this; }, or anyone else using this code, relies on the return value to be a clone of the original, and makes some modifications that may not have been made on the original object, you have a bug to track down.

Mzf · Answer 3 · 2014-03-18T09:01:37.960

2

Read MSDN and check this examples

I think you are right - you dont create new object , so if one want to clone and change the object - he will change the original object , which is not what expected

edited Mar 18 '14 at 09:01

answered Mar 18 '14 at 08:56

Mzf

5,210
2
24
37

That is not a reference to IClonable – Emond Mar 18 '14 at 08:59

score 1 · Answer 4 · answered Mar 18 '14 at 08:59

1

Note that the IClonable documentation does NOT speficy a deep or shallow copy.

It just specifies that it should copy. And this implementation doesn't.

answered Mar 18 '14 at 08:59

Emond

50,210
11
84
115

score 1 · Answer 5 · answered Mar 18 '14 at 09:33

To clone object try this.

Method 1:

public class Person : ICloneable
{
    public string LastName { get; set; }
    public string FirstName { get; set; }
    public Address PersonAddress { get; set; }

    public object Clone()
    {
        Person newPerson = (Person)this.MemberwiseClone();
        newPerson.PersonAddress = (Address)this.PersonAddress.Clone();

        return newPerson;
    }
}

public class Address : ICloneable
{
    public int HouseNumber { get; set; }
    public string StreetName { get; set; }

    public object Clone()
    {
        return this.MemberwiseClone();
    }
}

Method 2:

public class Person : ICloneable
{
    public string LastName { get; set; }
    public string FirstName { get; set; }
    public Address PersonAddress { get; set; }

    public object Clone()
    {
        object objResult = null;
        using (MemoryStream ms = new MemoryStream())
        {
            BinaryFormatter bf = new BinaryFormatter();
            bf.Serialize(ms, this);

            ms.Position = 0;
            objResult = bf.Deserialize(ms);
        }
        return objResult;
    }
}

Method 3 :

public class Person : ICloneable
    {
        public string LastName { get; set; }
        public string FirstName { get; set; }
        public Address PersonAddress { get; set; }

        public object Clone()
        {
            var objResult = new Person();
            objResult.LastName = this.LastName;
            objResult.FirstName = this.FirstName;
            objResult.PersonAddress = new Address();
            objResult.PersonAddress.HouseNumber = this.PersonAddress.HouseNumber;
            objResult.PersonAddress.StreetName = this.PersonAddress.StreetName;

            return objResult;
        }
    }

Is there anything wrong with this IClonable implementation

5 Answers5