Putting custom ICloneable implementation in abstract base class

Question

I am trying to implement ICloneable.Clone() in an abstract base class, so that subclasses don't need to worry about it. A fundamental responsibility of that class is to have an internal Guid property that should be passed along to the clones - that is, it's more like a "property clone". This allows me to clone an item taken from a repository, change its properties without altering the original item, and afterwards submit the changed instance back to the repo in a way that it can recognize it by Id.

But my current implementation is facing a problem: I cannot create the instances to pass the Id along, because the class is abstract!

public abstract class RepoItem : ICloneable, IEquatable<RepoItem>
{
    protected RepoItem()
    {
        Id = Guid.NewGuid();
    }

    private RepoItem(Guid id)
    {
        Id = id;
    }

    public Guid Id { get; private set; } 

    public object Clone()
    {
        return new RepoItem(Id);  // cannot create instance of abstract class
    }

    public bool Equals(RepoItem other)
    {
        return other.Id == Id;
    }
}

Is there a way to overcome this problem? Is this a decent design to begin with?

You're creating the wrong type anyway. And the problem is, you have no idea what the right type should be in the base class. You'll need to create an instance of the actual runtime type - e.g. through an abstract method that the derived classes are going to implement. — Luaan, Oct 03 '16 at 18:15
You might as well just make `Clone` abstract because the child class are the ones that will know how to clone themselves. — juharr, Oct 03 '16 at 18:18
@juharr Effectively, probably the best solution to ensure that `Clone` is implemented by derived class. One still has to ensure that `Clone` is overriden for all class that could be instanciated. — Phil1970, Oct 03 '16 at 21:57
Sure, but then why use `ICloneable`? Just make a generic helper method that can clone any object. If you don't want to use inheritance/polymorphism, there's no point in making it part of the base class. — Luaan, Oct 04 '16 at 07:11

score 3 · Accepted Answer · edited May 23 '17 at 12:19

3

Take a step back. You should not be implementing this interface at all, so whether the implementation goes in a base class or whatever is completely beside the point. Simply don't go there in the first place.

This interface has been deprecated since 2003. See:

Why should I implement ICloneable in c#?

for details.

edited May 23 '17 at 12:19

Community

1
1

answered Oct 03 '16 at 22:25

Eric Lippert

647,829
179
1,238
2,067

Well, I think this was autoritative enough ;o) Thanks!! – heltonbiker Oct 04 '16 at 13:09

score 0 · Answer 2 · answered Oct 03 '16 at 22:06

As already explained in comments, you cannot do that...

The best thing to do is probably to make Clone an abstract method (to ensure that cloning is available for all derived classes although one has to explicitly override Clone if the are more that one level of derivation and more that one level has a class that can be instanciated.).

After that, having kind of copy-constructor would be the way to go:

class RepoItem : ICloneable
{
    public abstract void Clone();
    protected RepoItem(RepoItem other) { Id = other.Id; }
}

class Derived1 : RepoItem
{
    protected Derived1(Derived1 other) : base(other) 
    { 
        myField1 = other.myField1; 
    }
    public virtual object Clone() { return new Derived1(this); }

    private int myField1;
}

class Derived2 : Derived1
{
    protected Derived2(Derived2 other) : base(other) 
    { 
        myField2 = other.myField2; 
    }
    public override object Clone() { return new Derived2(this); }

    private int myField2;
}

I'm not sure if I got virtual and override right as I rarely write such code.

Putting custom ICloneable implementation in abstract base class

2 Answers2