Deck of cards with shuffle and sort functionality

Question

Going through Head First C#'s Chapter 8: Enums and Collections, I learned about List<T>, as well as IComparable<T> and IComparer<T>. One exercise near the end asked to make a program to draw cards at random, then sort them.

I wanted to go the extra mile and make a fully usable, realistic card deck (their solution just picked 5 values at random from 2 enums, which could result in drawing the same card twice).

Any and all advice is welcome, albeit please understand that I have yet to learn LINQ in a coming chapter, so if you suggest to use it for something please at least explain why/advantages over vanilla C#.

I'm not including the using statement, they are the default ones that Visual Studio adds when creating a C# console project.

Kind

enum Kind
{
    Ace,
    Two,
    Three,
    Four,
    Five,
    Six,
    Seven,
    Eight,
    Nine,
    Ten,
    Jack,
    Queen,
    King,
}

Suit

enum Suit
{
    Clubs,
    Diamonds,
    Hearts,
    Spades,
}

Card

class Card : IComparable<Card>
{
    public Kind Kind;
    public Suit Suit;

    public Card(Kind kind, Suit suit)
    {
        Kind = kind;
        Suit = suit;
    }

    public int CompareTo(Card other)
    { 
        if (Suit > other.Suit)
        {
            return 1;
        }
        if (Suit < other.Suit)
        {
            return -1;
        }
        return Kind > other.Kind ? 1 : -1;
    }

    public override string ToString()
    {
        return $"{Kind} of {Suit}";
    }
}

CardDeck

class CardDeck
{
    public List<Card> Cards;

    public CardDeck()
    {
        Cards = new List<Card>();
        int numSuits = Enum.GetNames(typeof(Suit)).Length;
        int numKinds = Enum.GetNames(typeof(Kind)).Length;

        for (int suit = 0; suit < numSuits; suit++)
        {
            for (int kind = 0; kind < numKinds; kind++)
            {
                Cards.Add(new Card((Kind)kind, (Suit)suit));
            }
        }
    }

    public int CountCardsInDeck => Cards.Count;

    public Card DrawTopCard()
    {
        Card drawnCard = Cards[0];
        Cards.RemoveAt(0);
        return drawnCard;
    }

    public Card DrawBottomCard()
    {
        int lastCardIndex = CountCardsInDeck - 1;
        Card drawnCard = Cards[lastCardIndex];
        Cards.RemoveAt(lastCardIndex);
        return drawnCard;
    }

    public Card DrawRandomCard()
    {
        Random random = new Random();
        int randomCardIndex = random.Next(CountCardsInDeck);
        Card drawnCard = Cards[randomCardIndex];
        Cards.RemoveAt(randomCardIndex);
        return drawnCard;
    }

    public void AddCardOnTop(Card card)
    {
        if (!Cards.Contains(card))
        {
            Cards[0] = card;
            return;
        }
        throw new InvalidOperationException($"Deck already contains card {card}.");
    }

    public void AddCardOnBottom(Card card)
    {
        if (!Cards.Contains(card))
        {
            Cards.Add(card);
            return;
        }
        throw new InvalidOperationException($"Deck already contains card {card}.");
    }

    public void AddCardAtRandom(Card card)
    {
        if (!Cards.Contains(card))
        {
            Random random = new Random();
            Cards[random.Next(CountCardsInDeck)] = card;
            return;
        }
        throw new InvalidOperationException($"Deck already contains card {card}.");
    }

    public void Shuffle()
    {
        // Fisher-Yates shuffle method
        Random random = new Random();
        int n = CountCardsInDeck;
        while (n > 1)
        {
            n--;
            int k = random.Next(n + 1);
            Card randomCard = Cards[k];
            Cards[k] = Cards[n];
            Cards[n] = randomCard;
        }
    }

    public void Sort() => Cards.Sort();

    public void Sort(IComparer<Card> comparer) => Cards.Sort(comparer);

    public void WriteToConsole()
    {
        foreach (Card card in Cards)
        {
            Console.WriteLine(card);
        }
    }
}

CardOrderMethod

enum CardOrderMethod
{
    SuitThenKind,
    KindThenSuit,
}

CardSorter

class CardSorter : IComparer<Card>
{
    public CardOrderMethod SortBy = CardOrderMethod.SuitThenKind;

    public int Compare(Card x, Card y)
    {
        if (SortBy == CardOrderMethod.SuitThenKind)
        {
            if (x.Suit > y.Suit)
            {
                return 1;
            }
            if (x.Suit < y.Suit)
            {
                return -1;
            }
            return x.Kind > y.Kind ? 1 : -1;
        }
        if (SortBy == CardOrderMethod.KindThenSuit)
        {
            if (x.Kind > y.Kind)
            {
                return 1;
            }
            if (x.Kind < y.Kind)
            {
                return -1;
            }
            return x.Suit > y.Suit ? 1 : -1;
        }
        throw new NotImplementedException($"CardOrderMethod {SortBy} is not implemented.");
    }
}

Program

class Program
{
    static void Main(string[] args)
    {
        CardDeck cardDeck = new CardDeck();

        cardDeck.Shuffle();
        Console.WriteLine("---Shuffled deck---");
        cardDeck.WriteToConsole();

        CardSorter sorter = new CardSorter
        {
            SortBy = CardOrderMethod.SuitThenKind
        };
        cardDeck.Sort(sorter);
        Console.WriteLine("---Sorted deck: SuitThenKind---");
        cardDeck.WriteToConsole();

        cardDeck.Shuffle();

        sorter.SortBy = CardOrderMethod.KindThenSuit;
        cardDeck.Sort(sorter);
        Console.WriteLine("---Sorted deck: Kind Then Suit---");
        cardDeck.WriteToConsole();

        // Keep console open until a key is pressed
        Console.ReadKey();
    }
}

Answer 1

  enum Kind
  {
    ...
    Queen,
    King,
    Joker // ???
  }

Jokers is the joker. You're not considering jokers

---

    class Card : IComparable<Card> 
  {
     ...

I'm not convinced that there is a default comparison for cards? It could be misunderstood by consumers. On the other hand a deck of cards is always sorted by Suit and then Kind, but the comparison of cards is highly context dependent - dependent on the rules of the game. See further below.

---

public int CompareTo(Card other)
{ 
    if (Suit > other.Suit)
    {
        return 1;
    }
    if (Suit < other.Suit)
    {
        return -1;
    }
    return Kind > other.Kind ? 1 : -1;
}

can be simplified to:

public int CompareTo(Card other)
{
  if (other == null) return 1;
  if (Suit != other.Suit)
    return Suit.CompareTo(other.Suit);
  return Kind.CompareTo(other.Kind);
}

---

CardDeck is maybe a little verbose. IMO Deck is sufficient.

---

   Cards = new List<Card>();
    int numSuits = Enum.GetNames(typeof(Suit)).Length;
    int numKinds = Enum.GetNames(typeof(Kind)).Length;
    for (int suit = 0; suit < numSuits; suit++)
    {
        for (int kind = 0; kind < numKinds; kind++)
        {
            Cards.Add(new Card((Kind)kind, (Suit)suit));
        }
    }

There is a simpler way to do this:

  Cards = new List<Card>();

  foreach (Suit suit in Enum.GetValues(typeof(Suit)))
  {
    foreach (Kind kind in Enum.GetValues(typeof(Kind)))
    {
      Cards.Add(new Card(kind, suit));
    }
  }

---

CountCardsInDeck again: Count is sufficient. What should it else count if not cards in the deck?

---

public Card DrawTopCard()
{
    Card drawnCard = Cards[0];
    Cards.RemoveAt(0);
    return drawnCard;
}

public Card DrawBottomCard()
{
    int lastCardIndex = CountCardsInDeck - 1;
    Card drawnCard = Cards[lastCardIndex];
    Cards.RemoveAt(lastCardIndex);
    return drawnCard;
}

public Card DrawRandomCard()
{
    Random random = new Random();
    int randomCardIndex = random.Next(CountCardsInDeck);
    Card drawnCard = Cards[randomCardIndex];
    Cards.RemoveAt(randomCardIndex);
    return drawnCard;
}

This can be simplified:

public Card DrawCardAt(int index)
{
  if (index < 0 || index >= Count)
    throw new ArgumentOutOfRangeException(nameof(index));

  Card card = Cards[index];
  Cards.RemoveAt(index);
  return card;
}

public Card DrawTopCard()
{
  return DrawCardAt(0);
}

public Card DrawBottomCard()
{
  return DrawCardAt(Count - 1);
}

public Card DrawRandomCard()
{
  Random random = new Random();
  int index = random.Next(Count);
  return DrawCardAt(index);
}

---

   public void AddCardOnTop(Card card)
   {
        if (!Cards.Contains(card))
        {
            Cards[0] = card;
            return;
        }
        throw new InvalidOperationException($"Deck already contains card {card}.");
   }

A cleaner way to make precautions:

public void AddCardOnTop(Card card)
{      
  if (Cards.Contains(card))
    throw new InvalidOperationException($"Deck already contains card {card}.");

  //Cards[0] = card;
  Cards.Insert(0, card);
}

You replace the existing first card with a new one. Is that what you want? If so the method should be called SetTop() or ReplaceTop(). My suggestion is that you want to insert? The same could be said about AddCardOnBottom() and AddCardAtRandom().

---

public void WriteToConsole()
{
  foreach (Card card in Cards)
  {
    Console.WriteLine(card);
  }
}

Keep the UI out of your models. You could override ToString() and produce a line-string instead.

---

class CardSorter strictly speaking it is not a sorter but a comparer.

---

As discussed above, I maybe think that your comparer is somewhat redundant, because the comparison of cards depends on the game rules. I'm not a huge fan of card games, but I can't image games or situations where a deck of cards should be sorted differently than just by Suit and then Kind. But if you insists, you should consider other sorttypes:

  enum CardSortType
  {
    KindOnly, // HH: In some games only the kind matters
    SuitOnly, // HH: I can't image any games where this is used??
    SuitThenKind,
    KindThenSuit,
  }

  class CardSorter : IComparer<Card>
  {
    public CardSorter(CardSortType sortBy = CardSortType.SuitThenKind)
    {
      SortBy = sortBy;
    }

    public CardSortType SortBy { get; } // HH: Make it readonly

    public int Compare(Card x, Card y)
    {
      switch (SortBy)
      {
        case CardSortType.KindOnly:
          return x.Kind.CompareTo(y.Kind);
        case CardSortType.SuitOnly:
          return x.Suit.CompareTo(y.Suit);
        case CardSortType.SuitThenKind:
          if (x.Suit != y.Suit) return x.Suit.CompareTo(y.Suit);
          return x.Kind.CompareTo(y.Kind);
        case CardSortType.KindThenSuit:
          if (x.Kind != y.Kind) return x.Kind.CompareTo(y.Kind);
          return x.Suit.CompareTo(y.Suit);
        default:
          throw new NotImplementedException($"CardOrderMethod {SortBy} is not implemented.");
      }
    }
  }

In the above, I suggest a simpler comparison.

Answer 2

Just a few things not already mentioned by other answers:

Are you sure that you will need DrawRandomCard() ? How many card games have you played where you just draw from a random position in the deck? The only times I can think of this is useful is when you do a magic trick, which doesn't really work the same in code as in real life :) Normally in card games, you shuffle the deck and then draw from top or bottom.

Different card games might want to sort the deck in different ways, as others have mentioned already, but there's more options as well. Does Aces count as high or low? (Or both?) Should the sort order of the suits be [Clubs, Diamonds, Hearts, Spades], or [Clubs, Diamonds, Spades, Hearts], or [Diamonds, Hearts, Spades, Clubs] ? This can be dependent on the type of game, and maybe also a player's preferences. Adding more flexible options for Ace low/high and Suit order to your comparer would be good.

Answer 3

List<T>

You have just learned about List<T> and are eager to use it. Perfectly understandable, but think about what it is designed for and what alternatives are available for this use case. Here is an overview of common Collections in C#

To be fair, there is no common collection available designed for a deck of cards. What you need, is functionality from:

• HashSet<T> - unique items in the deck
• Queue<T> - add card on one end, take card from the other end
• Stack<T> - add card on one end, take card from that end

You have opted to use List<T> and added the above functionality yourself. This is not a bad solution, but you should realise it is not optimized for a deck of unique cards.

Card

You can compare enum values the same way you are comparing instances of Card.

public int CompareTo(Card other)
{ 
    if (Suit > other.Suit)
    {
        return 1;
    }
    if (Suit < other.Suit)
    {
        return -1;
    }
    return Kind > other.Kind ? 1 : -1;
}

The above can be written as:

public int CompareTo(Card other)
{ 
    if (other == null) return -1;
    var index = Suit.CompareTo(other.Suit);
    if (index == 0) index = Kind .CompareTo(other.Kind);
    return index;
}

Since Card is used by CardDeck and the latter uses a List<Card> with unique card items, you should do at least one of either and preferrably both:

• override Equals() and GetHashCode()
• implement IEquatable<Card>

This is because List<T> uses EqualityComparer<T> to check for unique instances (Reference Source).

code snippet (only equality snippets):

class Card : IEquatable<Card>
{
    public void Equals(Card card)
    {
        if (card == null) return false;
        return card.Suit == Suit && card.Kind == kind;
    }

    public override void Equals(object obj)
    {
        if (!(obj is Card card)) return false;
        return card.Suit == Suit && card.Kind == kind;
    }

    public override int GetHashCode()
    {
        // prefer picking two numbers that are co-prime
        var hash = 23;
        hash = hash * 31 + Suit.GetHashCode();
        hash = hash * 31 + Kind.GetHashCode();
    }
}

CardDeck

In your next lesson you will learn about LINQ. You will be able to write the card generator function as follows:

foreach (var card in (from suit in Enum.GetValues(typeof(Suit)).Cast<Suit>()
                      from kind in Enum.GetValues(typeof(Kind)).Cast<Kind>()
                      select new { suit, kind }))
{
    Cards.Add(new Card(card.kind, card.suit));
}

as compared to:

int numSuits = Enum.GetNames(typeof(Suit)).Length;
int numKinds = Enum.GetNames(typeof(Kind)).Length;

for (int suit = 0; suit < numSuits; suit++)
{
    for (int kind = 0; kind < numKinds; kind++)
    {
        Cards.Add(new Card((Kind)kind, (Suit)suit));
    }
}

For shuffling and drawing random cards, consider declaring the Random as a private variable of the deck rather than inside the methods. This avoids the not so random Random behavior.

private readonly Random random = new Random();

Consider inverting code like this:

if (!Cards.Contains(card))
{
    Cards[0] = card;
    return;
}
throw new InvalidOperationException($"Deck already contains card {card}.");

to this:

  if (Cards.Contains(card))
  {
     throw new InvalidOperationException($"Deck already contains card {card}.");
  }
  Cards[0] = card;

Don't pollute your classes with specific utility methods. Write this as a static method or extension method in your test code.

public void WriteToConsole()
{
    foreach (Card card in Cards)
    {
        Console.WriteLine(card);
    }
}

CardSorter

You should prefer CompareTo over comparasion operators.

if (x.Suit > y.Suit)
{
    return 1;
}
if (x.Suit < y.Suit)
{
    return -1;
}
return x.Kind > y.Kind ? 1 : -1;

var index = x.Suit.CompareTo(y.Suit);
if (index == 0) index = x.Kind.CompareTo(y.Kind);
return index;

Also include null comparisons.

if (x == null) return (y == null) ? 0 : 1;
if (y == null) return -1;

General guidelines

• use var as much as you can, specially when the declared type can be inferred from reading the code. var card = new Card(); reads better as Card card = new Card(); or Dictionary<string, List<int>> collection = new Dictionary<string, List<int>>(); vs var collection = new Dictionary<string, List<int>>();
• check arguments against null in public methods

Answer 4

Encapsulation and Mutability

Your Card class is a perfect candidate for being immutable: make Kind and Suit readonly fields or getter-only properties (prefer the later in most cases):

public Kind Kind { get; }
public Suit Suit { get; }

You should also hide away the Deck list in CardDeck: making it readonly and private (or protected, if you are happy to commit to (and document) the internal representation) would probably be most sensible. Prevent the user shooting themselves in the foot by messing with state they shouldn't be able to access.

Card..ctor(Kind, Suit)

Enums in .NET are very simple, and you have no gaurentee that a Kind is actually one of those you have declared. I would consider adding checks here to ensure that an invalid Kind or Suit is not used: catch the mistake early, because it will go wrong somewhere down the line, and the sooner you throw an exception at the consumer, the easier it will be for them to work out where they went wrong.

Comparers

Dfhwze's Answer indirectly addresses this, but your Card.CompareTo(Card) can never return 0: this is very bad. It should check if the cards are equivalent, and return 0 in that instance and that instance only.

As alluded by Henrik Hansen, I would rename CardSorter to CardComparer: it's what everyone will assume, and I would certainly expect a CardSorter to provide a 'Sort' method. Again, the comparisons here don't allow for the same card appearing twice: your code CardDeck class may assume they are never equal, but other people may try to use this class for other purposes. These sorts of things need to be documented.

Misc

• Why should adding an already present card disallowed? Some cards games involve multiple complete 'decks' in a single 'deck'.

• This all needs documention. As a consumer, I need to know that AddCardOnBottom won't accept an already present card.

• As discussed by Henrik, CardDeck.Sort is a bit odd: I would remove it, and force the user to specify the type of sort they want (by using the CardSorter class).

• I would rename CardSorter to CardComparer: it's what everyone will assume, and I would certainly expect a CardSorter to provide a 'Sort' method. Again, the ccomparisons here don't allow for the same card appearing twice: your code may not allow this, but other people may try to use this class for other purposes.

• To reiterative dfhwze 's point, you should not be creating a new Random instance in Shuffle: either encapsulate one in the class which you take as a parameter to the constructor, or allow the Shuffle and other 'Random' methods to take one as a parameter.

• You might consider making the type of your enums explicit (e.g. both fit in a byte), and you could make your Card class an immutable struct with the same semantics, only it would take up less space and reduce the GC overhead.

Answer 5

First off, this is quite good for beginner code. You'll do well if you keep the good habits you've started following.

A few critiques:

class Card : IComparable<Card>

Do you intend this class to be subclassed? If not, seal it.

Are cards comparable by reference? That is, suppose we have two different instances of the queen of spades. Is it possible for the user of the library to tell them apart, or not? If no, then this should be a struct, not a class; force the user to compare by value.

public Kind Kind;
public Suit Suit;

NEVER DO THIS. Never make a public field. First, because it is a bad practice, and second, because now anyone can have a card in hand and change it. Cards are immutable! This should be

public Kind Kind { get; private set; }

And so on.

public int CompareTo(Card other)

This code is very wrong. x.CompareTo(x) must always return zero, always. You never tested that case, otherwise you would have found the bug, so write that test.

If you're going to implement CompareTo it would be a good practice to also implement Equals, operator ==, operator !=, GetHashCode, operator >, operator <, operator <= and operator >=. It is weird that you can compare two cards for rank with CompareTo but not with < or ==.

class CardDeck

Again, seal it.

public List<Card> Cards;

Again, never make a public field. This should be private; it's an implementation detail of the class.

public int CountCardsInDeck => Cards.Count;

Needlessly verbose; just public int Count => Cards.Count is fine.

public Card DrawTopCard()

Throw a better exception if there is no top card.

Random random = new Random();

Newer versions of .NET have fixed this, but in older versions this was a bad practice, creating a Random every time you needed it. Instead, make it a field.

public void WriteToConsole()

This is a weird way to write this functionality. Normally you'd override ToString and then do Console.WriteLine(deck);

class CardSorter : IComparer<Card>
{
    public CardOrderMethod SortBy = CardOrderMethod.SuitThenKind;

Again, seal your classes, and again, no public fields.

But this design is wrong. Don't make one class that can do two things. Make two classes if you have two things to do. Make a SuitThenKindSorter and a KindThenSuitSorter class, not one class that has an if in the middle.

And again, the comparison logic is wrong; you are required to have a comparison where things that are equal are equal. You must never assume that the things being compared are unequal. The contract of the comparison is that it can take any two objects of the type and compare them, not any two different objects.