C# stack overflow

Question

I am trying to find out why I am getting a stack overflow exception. I am creating a simple card game for a school assignment and when I clone the cards to return them I get the stack overflow exception.

So I got this card class:

public class Card : ICloneable
{
    ....

    #region ICloneable Members

    public object Clone()
    {
        return this.Clone(); // <--- here is the error thrown when the first card is to be cloned
    }

    #endregion
}

and I have a class called Hand which then clones the cards:

internal class Hand
{
        internal List<Card> GetCards()
        {
            return m_Cards.CloneList<Card>(); // m_Cards is a List with card objects
        }
}

Last, I got an extension method for the List:

    public static List<T> CloneList<T>(this List<T> listToClone) where T : ICloneable
    {
        return listToClone.Select(item => (T)item.Clone()).ToList();
    }

The error gets thrown in the card class (IClonable method),

An unhandled exception of type 'System.StackOverflowException' occurred in CardLibrary.dll

Do the cards need to be mutable i.e. do they have state that can change? If not then you could just have a set of immutable cards that you could reuse in different collections. No cloneing needed. — pjp, Jul 29 '09 at 09:42
Please see also this discussion regarding ICloneable: http://stackoverflow.com/questions/699210/why-should-i-implement-icloneable-in-c — peterchen, Jul 29 '09 at 09:57
After reading the title of this question I was thinking this belongs on meta... ;-) — EricSchaefer, Jul 29 '09 at 10:04

score 22 · Accepted Answer · answered Jul 29 '09 at 09:23

22

You're calling yourself:

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

This results in infinite recursion.

Your Clone() method should copy all properties/fields to a new object:

public object Clone()
{
    Card newCard = new Card();

    newCard.X = this.X;
    // ...

    return newCard;
}

or you could use MemberwiseClone()

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

But that gives you less control over the cloning process.

answered Jul 29 '09 at 09:23

Philippe Leybaert

168,566
31
210
223

1

+1, great answer. Whenever using `MemberwiseClone()` just don't forget that it only creates a shallow copy, i.e. if a class field is a reference type, the reference is copied, but not the referenced object. – Dirk Vollmar Jul 29 '09 at 09:53

score 0 · Answer 2 · answered Jul 29 '09 at 10:00

I've tended to use MemberwiseClone() for the simple data, and then implemented ICloneable throghout the hierarchy of elements that I've needed to clone, so:

public class CRMLazyLoadPrefs : ICloneable
{
    public bool Core { get; set; }
    public bool Events { get; set; }    
    public bool SubCategories { get; set; }
    public OrganisationLazyLoadPrefs { get; set; }

    public object Clone()
    {
        CRMLazyLoadPrefs _prefs = new CRMLazyLoadPrefs();
        // firstly, shallow copy the booleans
        _prefs  = (CRMLazyLoadPrefs)this.MemberwiseClone();
        // then deep copy the other bits
        _prefs.Organisation = (OrganisationLazyLoadPrefs)this.Organisation.Clone();
    }
}

Where OrganisationLazyLoadPrefs also implements ICloneable and so on and so forth throughout the hierarchy.

Hope this helps, Cheers, Terry

just seen that comment from @peterchen though, will have to take a look at this in more detail - keen to follow best practice wherever possible. — Terry_Brown, Jul 29 '09 at 10:03

C# stack overflow

2 Answers2