Class to represent playing card

Question

I admit I'm sort of stuck on this. I couldn't think of a better way of representing the card value that will be used in a playing card game. How well thought out is the casting an enum value to an int? I might be wrong but it does not seem very OOP. Is there a different way I should be doing it instead?

void Main()
{
    Card testCard = new Card(CardSuit.Diamonds, (int)CardValue.Ace);
}

public enum CardSuit
{
    Spades,
    Hearts,
    Diamonds,
    Clubs
}

public enum CardValue
{
    Ace = 1,
    Two = 2, Three = 3, Four = 4, Five = 5, Six = 6, Seven = 7, Eight = 8, Nine = 9, Ten = 10,
    Jack = 11,
    Queen = 12,
    King = 13
}

public class Card
{
    public CardSuit CardSuit { get; private set; }
    public int CardValue { get; private set; }

    public Card(CardSuit cardSuit, int cardValue)
    {
        CardSuit = cardSuit;
        CardValue = cardValue;
    }
}

Answer 1

Use the types you have

Why are you storing the card value as an int? You have already created a CardValue enum, so use it. Change the Card.CardValue property and the cardValue parameter in the Card constructor to be of type CardValue. That way, you don't need to cast to an int when creating a card, and you can always get the numeric card value by casting ((int)cardValue) when you need it.

---

Argument Validation

What happens if you use new Card((CardSuit)100, (CardValue)100). Your card will have an invalid suit and value. In order to validate the arguments, you can use Enum.IsDefined, which will cause a performance penalty since it uses reflection, or you can hardcode a valid range of values.

---

Type Naming

The name CardValue seems too general for such a specific type (thanks to @RickDavin). "Value" implies it may refer to any value from the card, such as its suit or rank. A better name might be CardRank.

Property Naming

As noted by @Heslacher, type names shouldn't be used as property names. For instance, in the Card class, the property CardSuit and has a redundant name. Consider a variable Card card = new Card(). Using card.Suit is much clearer and more concise than using card.CardSuit. In addition, you should change the property CardValue to Rank, since the type CardValue was renamed.

---

Readonly Properties

It seems Rank and Suit are supposed to be readonly. Prior to C#6, your approach was an acceptable one, so long as you remembered not to change those properties outside of the constructor. However, if you're using C#6, a better alternative is to declare them without setters. Like readonly fields, properties that have only getters can only be set inside the constructor, but not outside of it.

---

With that, the Card class should look like:

public class Card
{
    public CardSuit Suit { get; }
    public CardRank Rank { get; }

    public Card(CardSuit suit, CardRank rank)
    {
        if (suit < CardSuit.Spades || suit > CardSuit.Clubs) // or if (!Enum.IsDefined(typeof(CardSuit), suit))
        {
            // handle error (possibly with ArgumentException)
        }
        if (rank < CardRank.Ace || rank > CardRank.King) // or if (!Enum.IsDefined(typeof(CardValue), rank))
        {
            // handle error
        }
        Suit = suit;
        Rank = rank;
    }
}

Answer 2

In addition to the great answer provided by Fernando Matsumoto you should consider implementing the equality/hashcode methods/operators. Reference equality isn't how cards are treated in the real world. In the real world it doesn't matter which deck the Jack of Clubs came from it is always a Jack of Clubs.

public class Card : IEquatable<Card>
{
    public Card(CardSuit suit, CardRank rank)
    {
        // Suit and Rank validation logic here

        Suit = suit;
        Rank = rank;
    }

    // Replace this with the plain get call like Fernando's answer in C# 6
    // You can also leave it and be careful or create a private readonly backing property
    public CardSuit Suit { get; private set; }

    public CardRank Rank { get; private set; }

    public static bool operator ==(Card left, Card right)
    {
        // Review https://msdn.microsoft.com/en-US/library/ms173147%28v=vs.80%29.aspx
        // If both are null, or both are same instance, return true.
        if (ReferenceEquals(left, right))
        {
            return true;
        }

        // If one is null, but not both, return false.
        if (((object)left == null) || ((object)right == null))
        {
            return false;
        }

        // Return true if the fields match:
        return left.Equals(right);
    }

    public static bool operator !=(Card left, Card right)
    {
        return !(left == right);
    }

    public bool Equals(Card other)
    {
        if(other == null)
        {
            return false;
        }

        return Rank == other.Rank && Suit == other.Suit;
    }

    public override bool Equals(object obj)
    {
        return Equals(obj as Card);
    }

    public override int GetHashCode()
    {
        return Suit.GetHashCode() ^ Rank.GetHashCode();
    }

    // Not strictly needed but why not do it....
    public override string ToString()
    {
        // Take advantage of how the Enum.ToString() method functions
        return Rank.ToString() + " of " + Suit.ToString();
    }
}

Something else you might consider is implementing the IComparable<Card> interface. That would let you sort a user's hand trivially. I'll leave that exercise to you because I don't want to debate what is the proper suit order¹.

---

This is fairly nit-picky that has more to do with domain knowledge than OOP. The order you specify your suits is up to you but I'd recommend alternating the color of the suits. When you display the hand to the user alternating colors help the users distinguish Hearts from Diamonds, and Spades from Clubs. Respecting this in the CardSuit enum will make the sorting/comparison logic easier.

If you do this then you should keep in mind that you're encoding implicit knowledge into the enum. Since the enum should be extremely stable this implicit knowledge feels like a reasonable trade-off to writing a suit order extension method, suit wrapper with an order value, or an external IComparer<Suit> implementation which must be specified for each comparison/sort.

---

^{_{1: Even though it is clearly Spades, Diamonds, Clubs, Hearts... ;)}}