Copy constructor versus Clone()

Question

You should not derive from ICloneable.

The reason is that when Microsoft designed the .net framework they never specified whether the Clone() method on ICloneable should be a deep or shallow clone, thus the interface is semantically broken as your callers won't know whether the call will deep or shallow clone the object.

Instead, you should define your own IDeepCloneable (and IShallowCloneable) interfaces with DeepClone() (and ShallowClone()) methods.

You can define two interfaces, one with a generic parameter to support strongly typed cloning and one without to keep the weakly typed cloning ability for when you are working with collections of different types of cloneable objects:

public interface IDeepCloneable
{
    object DeepClone();
}
public interface IDeepCloneable<T> : IDeepCloneable
{
    T DeepClone();
}

Which you would then implement like this:

public class SampleClass : IDeepCloneable<SampleClass>
{
    public SampleClass DeepClone()
    {
        // Deep clone your object
        return ...;
    }
    object IDeepCloneable.DeepClone()   
    {
        return this.DeepClone();
    }
}

Generally I prefer to use the interfaces described as opposed to a copy constructor it keeps the intent very clear. A copy constructor would probably be assumed to be a deep clone, but it's certainly not as much of a clear intent as using an IDeepClonable interface.

This is discussed in the .net Framework Design Guidelines and on Brad Abrams' blog

_{(I suppose if you are writing an application (as opposed to a framework/library) so you can be sure no one outside of your team will be calling your code, it doesn't matter so much and you can assign a semantic meaning of "deepclone" to the .net ICloneable interface, but you should make sure this is well documented and well understood within your team. Personally I'd stick to the framework guidelines.)}

In C#, what is the preferred way to add (deep) copy functionality to a class? Should one implement the copy constructor, or rather derive from ICloneable and implement the Clone() method?

The problem with ICloneable is, as others have mentioned, that it does not specify whether it is a deep or shallow copy, which makes it practically unuseable and, in practice, rarely used. It also returns object, which is a pain, since it requires a lot of casting. (And though you specifically mentioned classes in the question, implementing ICloneable on a struct requires boxing.)

A copy constuctor also suffers from one of the problems with ICloneable. It isn't obvious whether a copy constructor is doing a deep or shallow copy.

Account clonedAccount = new Account(currentAccount); // Deep or shallow?

It would be best to create a DeepClone() method. This way the intent is perfectly clear.

This raises the question of whether it should be a static or instance method.

Account clonedAccount = currentAccount.DeepClone();  // instance method

or

Account clonedAccount = Account.DeepClone(currentAccount); // static method

I slightly prefer the static version sometimes, just because cloning seems like something that is being done to an object rather than something the object is doing. In either case, there are going to be issues to deal with when cloning objects that are part of an inheritence hierarchy, and how those issues are delt with may ultimately drive the design.

class CheckingAccount : Account
{
    CheckAuthorizationScheme checkAuthorizationScheme;

    public override Account DeepClone()
    {
        CheckingAccount clone = new CheckingAccount();
        DeepCloneFields(clone);
        return clone;
    }

    protected override void DeepCloneFields(Account clone)
    {
        base.DeepCloneFields(clone);

        ((CheckingAccount)clone).checkAuthorizationScheme = this.checkAuthorizationScheme.DeepClone();
    }
}

I recommend using a copy constructor over a clone method primarily because a clone method will prevent you from making fields readonly that could have been if you had used a constructor instead.

If you require polymorphic cloning, you can then add an abstract or virtual Clone() method to your base class that you implement with a call to the copy constructor.

If you require more than one kind of copy (ex: deep/shallow) you can specify it with a parameter in the copy constructor, although in my experience I find that usually a mixture of deep and shallow copying is what I need.

Ex:

public class BaseType {
   readonly int mBaseField;

   public BaseType(BaseType pSource) =>
      mBaseField = pSource.mBaseField;

   public virtual BaseType Clone() =>
      new BaseType(this);
}

public class SubType : BaseType {
   readonly int mSubField;

   public SubType(SubType pSource)
   : base(pSource) =>
      mSubField = pSource.mSubField;

   public override BaseType Clone() =>
      new SubType(this);
}

There is a great argument that you should implement clone() using a protected copy constructor

It is better to provide a protected (non-public) copy constructor and invoke that from the clone method. This gives us the ability to delegate the task of creating an object to an instance of a class itself, thus providing extensibility and also, safely creating the objects using the protected copy constructor.

So this is not a "versus" question. You may need both copy constructor(s) and a clone interface to do it right.

(Although the recommended public interface is the Clone() interface rather than Constructor-based.)

Don't get caught-up in the explicit deep or shallow argument in the other answers. In the real world it is almost always something in-between - and either way, should not be the caller's concern.

The Clone() contract is simply "won't change when I change the first one". How much of the graph you have to copy, or how you avoid infinite recursion to make that happen shouldn't concern the caller.

Implementing ICloneable's not recommended due to the fact that it's not specified whether it's a deep or shallow copy, so I'd go for the constructor, or just implement something yourself. Maybe call it DeepCopy() to make it really obvious!

You'll run into problems with copy constructors and abstract classes. Imagine you want to do the following:

abstract class A
{
    public A()
    {
    }

    public A(A ToCopy)
    {
        X = ToCopy.X;
    }
    public int X;
}

class B : A
{
    public B()
    {
    }

    public B(B ToCopy) : base(ToCopy)
    {
        Y = ToCopy.Y;
    }
    public int Y;
}

class C : A
{
    public C()
    {
    }

    public C(C ToCopy)
        : base(ToCopy)
    {
        Z = ToCopy.Z;
    }
    public int Z;
}

class Program
{
    static void Main(string[] args)
    {
        List<A> list = new List<A>();

        B b = new B();
        b.X = 1;
        b.Y = 2;
        list.Add(b);

        C c = new C();
        c.X = 3;
        c.Z = 4;
        list.Add(c);

        List<A> cloneList = new List<A>();

        //Won't work
        //foreach (A a in list)
        //    cloneList.Add(new A(a)); //Not this time batman!

        //Works, but is nasty for anything less contrived than this example.
        foreach (A a in list)
        {
            if(a is B)
                cloneList.Add(new B((B)a));
            if (a is C)
                cloneList.Add(new C((C)a));
        }
    }
}

Right after doing the above, you start wishing you'd either used an interface, or settled for a DeepCopy()/ICloneable.Clone() implementation.