9
\$\begingroup\$

I'm new to C# but not 100% new to development. I wanted to try writing a Card Shuffle + Card Dealing algorithm using as least as possible inbuilt functions or any libraries and no internet use to practice my C# skills.

Here is my code:

class cardShuffle
{
  private string[] deck = new string[] {"Ace-of-Clubs", "Ace-of-Hearts", "Ace-of-Spades", "Ace-of-Diamonds",
                         "King-of-Clubs", "King-of-Hearts", "King-of-Spades", "King-of-Diamonds",
                         "Quenn-of-Clubs", "Quenn-of-Hearts", "Quenn-of-Spades", "Quenn-of-Diamonds","Jack-of-Clubs", "Jack-of-Hearts", "Jack-of-Spades", "Jack-of-Diamonds",
                         "10-of-Clubs", "10-of-Hearts", "10-of-Spades", "10-of-Diamonds",
                         "9-of-Clubs", "9-of-Hearts", "9-of-Spades", "9-of-Diamonds",
                         "8-of-Clubs", "8-of-Hearts", "8-of-Spades", "8-of-Diamonds",
                         "7-of-Clubs", "7-of-Hearts", "7-of-Spades", "7-of-Diamonds",
                         "6-of-Clubs", "6-of-Hearts", "6-of-Spades", "6-of-Diamonds",
                         "5-of-Clubs", "5-of-Hearts", "5-of-Spades", "5-of-Diamonds",
                         "4-of-Clubs", "4-of-Hearts", "4-of-Spades", "4-of-Diamonds",
                         "3-of-Clubs", "3-of-Hearts", "3-of-Spades", "3-of-Diamonds",
                         "2-of-Clubs", "2-of-Hearts", "2-of-Spades", "2-of-Diamonds",}; 

  public void Start(int numberOfShuffles)
  {
    var shuffledDeck = new string[52];

    for(int i = 1; i <= numberOfShuffles; i++)
    {
      shuffledDeck = Shuffle(deck);
    }

    DealCards(shuffledDeck);
  }
  private string[] Shuffle(string[] deck)
  {
    Random randomNum = new Random();

    for (int i = deck.Length - 1; i > 0; --i)
    {
      int j = randomNum.Next(i + 1);
      string temp = deck[i];
      deck[i] = deck[j];
      deck[j] = temp;
    }
    return deck;
  }

  private void DealCards(string[] shuffledDeck)
  {
    string[] player1 = new string[13]; 
    string[] player2 = new string[13];
    string[] player3 = new string[13];
    string[] player4 = new string[13];

    const int maxCardsInHand = 13;
    int currentCardsP1 = 0;
    int currentCardsP2 = 0;
    int currentCardsP3 = 0;
    int currentCardsP4 = 0;

    int indexP1 = 0;
    int indexP2 = 0;
    int indexP3 = 0;
    int indexP4 = 0;

    for(int i = 0; i < deck.Length; i++)
    {
      if(currentCardsP1 < maxCardsInHand)
      {
        player1[indexP1] = shuffledDeck[i];
        currentCardsP1++;
        indexP1++;
      }else if(currentCardsP2 < maxCardsInHand)
      {
        player2[indexP2] = shuffledDeck[i];
        currentCardsP2++;
        indexP2++;
      }else if(currentCardsP3 < maxCardsInHand)
      {
        player3[indexP3] = shuffledDeck[i];
        currentCardsP3++;
        indexP3++;
      }else if(currentCardsP4 < maxCardsInHand)
      {
        player4[indexP4] = shuffledDeck[i];
        currentCardsP4++;
        indexP4++;
      }
    }
    Console.WriteLine("--------------------------------------------------------------------------------------");
    Console.WriteLine(string.Format("{0, -20} | {1, -20} | {2, -20} | {3, -20}", "Player1 - Cards", "Player2 - Cards", "Player3 - Cards", "Player4 - Cards"));
    Console.WriteLine("--------------------------------------------------------------------------------------");

    for (int j = 0; j < 13; j++)
    {
      Console.WriteLine(string.Format("{0, -20} | {1, -20} | {2, -20} | {3, -20}", player1[j], player2[j], player3[j], player4[j]));
    }
  }
}

I'm using net6.0

cardShuffle shuffle = new cardShuffle();

shuffle.Start(1);

With the start Method, it takes an integer of how many times you want to shuffle the deck. If you put in 0 the output will be nothing. Please keep that in mind it's not a bug, that's intended.

\$\endgroup\$

1 Answer 1

16
\$\begingroup\$

Modeling the real world

You've clearly drawn inspiration from the real world. That's a good thing. A lot of programming and the design that goes into it can be modeled based off of real life.

However, sometimes there are better ways to algorithmically approach something that doesn't quite work in real life. It's okay to not see this on the first pass when you write your code, but you should re-evaluate your code a second time to look for improvements.

Most if not all of the below review points are things you could have found on a second pass of your code. With experience, you'll learn to spot these yourself.


Defining your cards and deck

private string[] deck = new string[] {"Ace-of-Clubs", "Ace-of-Hearts", ...

Programming is all about finding reusable repeating patterns. While there are 52 unique cards in a deck, they can more easily be defined by their value (ace, king, ...) and their suit (hearts, clubs, ...)

Given that we are using a closed set of options, enums are the way to go here.

public enum CardValue { Ace, King, ... } // Complete this yourself
public enum CardSuit { Hearts, Clubs, Spades, Diamonds }

A card can then be defined as a combination of these values

public struct Card
{
    public CardValue Value { get; }
    public CardSuit Suit { get; }

    public Card(CardValue value, CardSuit suit)
    {
        Value = value;
        Suit = suit;
    }
}

I've intentionally designed it so that a card cannot be changed after it has been created, to avoid mistakes where you magically change one card into another.

I'm skipping over the struct vs class explanation here because it's not the main focus of the answer; Google this for yourself to learn when to use what.

You can then create cards like so:

var kingOfClubs = new Card(CardValue.King, CardSuit.Clubs);

And to generate a full deck, you can just iterate over the available values in the enums:

public IEnumerable<Card> CreateDeck()
{
    var deck = new List<Card>();

    foreach (var value in Enum.GetValues<CardValue>())
        foreach (var suit in Enum.GetValues<CardSuit>())
            deck.Add(new Card(value, suit));

    return deck;
}

Note that because we are now using a Card type, a lot of your string[] code will have to change to using a Card-based collection type. It could be Card[] but I would advise moving towards IEnumerable<Card> where possible. It's a more generalized approach that can account for both lists and arrays.


Shuffling multiple times

for(int i = 1; i <= numberOfShuffles; i++)
{
    shuffledDeck = Shuffle(deck);
}

Shuffling multiple times makes sense in the real world, because when we shuffle, we only move the cards around somewhat. For example, in a riffle shuffle the cards are interleaved, but the order of the cards (in either half of the deck) remains the same. This is an imperfect random, and experienced dealers are able to keep track of the new card order; which is why we shuffle several times to make it impossible for humans to keep track of the card order.

The computer, however, does not cheat (unless you tell it to). It can randomly shuffle all cards around on the first go; so there is no need to shuffle more than once.

Shuffling multiple times is the equivalent of rolling a die several times and only using the last roll. The previous rolls were simply irrelevant and could have been skipped.


Dealing cards

private void DealCards(string[] shuffledDeck)
{
    // long code omitted
}

1

There is a lot of repetition in your code here, which can be reduced drastically. Because you're using arrays, you're having to keep track of the "next empty" index where you want to add the next card. Had you been using a list, you could've simply called the .Add() method without needing to bother specifying where to add it (it automatically gets added at the end of the list).

Similary, because you are using arrays instead of lists, it's hard to keep track of how "full" your players' hands are. Had you been using lists, you wouldn't have needed to keep track of these separate integers, because you could just use the .Count property of a list.

Short example of how a list would be much easier to use here:

var myHand = new List<Card>();

myHand.Add(new Card(...));
myHand.Add(new Card(...));
myHand.Add(new Card(...));

Console.WriteLine($"You are holding {myHand.Count} cards");

This alone would reduce your method's size by 75% (this is my personal guesstimate) by cutting out all the index tracking and array juggling logic. I'm skipping the rewrite of your logic here as there are other optimizations I want to discuss.

2

Initially, I suspected you were dealing your cards round-robin style (P1, P2, P3, P4, P1, P2, P3, P4, P1, ...), but this is not the case. When you step through your code, you'll see that the first 13 cards are dealt to P1, then the next 13 are dealt to P2, and so on.

If this is the intended approach, then this can be simplified using LINQ. The simplest solution would be:

IEnumerable<Card> P1 = deck.Take(13);
IEnumerable<Card> P2 = deck.Skip(13).Take(13);
IEnumerable<Card> P3 = deck.Skip(2 * 13).Take(13);
IEnumerable<Card> P4 = deck.Skip(3 * 13).Take(13);

This could be made better by iteratively creating your decks, but this would require you to create a list of player hands, instead of having separate variables for each player hand. Depending on your logic, that may or may not be desirable, which is why I'm skipping it here.

If you intend to deal your cards round-robin style; I suggest you reconsider. This kind of dealing in the real world is another form of random shuffling to ensure that players cannot reliably track which card goes to which player. However, given that we trust our own shuffling logic to be truly random (let's sidestep a discussion on "true" random here), there's no need to further obfuscate the card order.

I would omit round robin dealing because it has no real benefit here. However, if you prefer to model real world dealing strategies (whatever your reason may be), round robin dealing can be done easier using LINQ and modulo operations.

In essence, if we calculate index % 4 for each card's index in the list, we get a value from 0 to 3 which indicates which player should receive the card. Note that the 4 is the number of players. If you have 2 players, you would use % 2, and so on.

int playerCount = 4;
var playerHands = deck
                   // Select each card with its index attached)
                   .Select((card, index) => new { Index = index, Card = card })
                   // Sort them into sublists based on the index modulo calculation
                   .GroupBy(x => x.Index % playerCount)
                   // Reconvert back to only cards without indexes
                   .Select(g => g.Select(x => x.Card));

var P1 = playerHands[0];
var P2 = playerHands[1];
var P3 = playerHands[2];
var P4 = playerHands[3];

I suggest reading up on the modulo operator for more details. There's some interesting math here that is very commonly used in card games and similar cyclic data structures.

I suggest reading up on the GroupBy LINQ method for more details. There's more to say on this topic than I can cover in this answer.


Printing the result

First and foremost, you should be separating this logic into a method of its own. Whether you deal your cards and whether you print the player hands are two very different actions and you want to be able to take one without taking the other. As a simple example, you wouldn't want to immediately report all the player hands because then every player knows what the other player is holding.

I'm not going to go deep into how you should display your data, because it is so very contextual and prone to change. I suspect your intention here was only to verify the results of your logic, and the code you wrote suffices for that purpose.

One tip though, since I've changed your code to use a Card type, you can print the values as a string there:

public struct Card
{
    public CardValue Value { get; }
    public CardSuit Suit { get; }

    public Card(CardValue value, CardSuit suit)
    {
        Value = value;
        Suit = suit;
    }

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

$"{Value} of {Suit}" will yield results like Seven of Clubs, Ace of Spades, ... which nicely matches the names we give to playing cards.

\$\endgroup\$
11
  • 13
    \$\begingroup\$ @DaLegacy: LINQ is part of the .Net framework, it is not an external library. It is as much part of .Net as e.g. Random and Console are, which you both use. \$\endgroup\$
    – Flater
    Commented Feb 7, 2022 at 13:57
  • 13
    \$\begingroup\$ Very good answer and I agree with most of it. But the advice on sorting is incorrect; using OrderBy(random) is a bad idea. The question seems to implement the Durstenfeld shuffle correctly, which is considered the gold standard of understandable, unbiased shuffles. \$\endgroup\$
    – Nayuki
    Commented Feb 7, 2022 at 20:45
  • 1
    \$\begingroup\$ If you think the OrderBy will be more efficient than swapping, that tells me you have no idea how OrderBy works \$\endgroup\$ Commented Feb 8, 2022 at 1:16
  • 6
    \$\begingroup\$ The original shuffle algorithm performs a uniform Fisher-Yates shuffle in O(n) time. Is a random OrderBy shuffle uniform, and what is its runtime complexity? (I suspect the answers are "no" and "O(n log n)".) \$\endgroup\$
    – Mark
    Commented Feb 8, 2022 at 2:08
  • 6
    \$\begingroup\$ Also jumping in to say the OP's shuffle strategy is superior to OrderBy (if implemented correctly). From a functionality perspective, OrderBy potentially slight bias if two items get the same random value, ordering will be preserved. From a performance perspective, LINQ usually creates garbage and overhead, that OP's method avoids. Additionally, consider using RandomNumberGenerator if 'true' randomness is desired. \$\endgroup\$
    – NPSF3000
    Commented Feb 8, 2022 at 16:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.