11
\$\begingroup\$

I made a 52 standard deck generator for practicing my C#. Just to let you know I am using the unity editor.

I would your feedback as to whether my code is good/bad or whatever you may desire.

using System;
using System.Collections.Generic;
using UnityEngine;

public class Standard52Deck {
    List<Card> deck = new List<Card>();

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

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

    class Card {
        readonly Ranks rank;
        readonly Suits suit;

        public Card(Ranks rank, Suits suit) {
            this.rank = rank;
            this.suit = suit;
        }

        public override string ToString() {
            return $"{rank} of {suit}";
        }
    }

    IEnumerable<Card> GetSortedCardsBySuits() {
        var rankCount = Enum.GetValues(typeof(Ranks));
        var suitCount = Enum.GetValues(typeof(Suits));
        var deckCount = rankCount.Length * suitCount.Length;

        var index = 0;
        var rankIteration = 0;

        while (index < deckCount) {
            var rank = (Ranks)(index % 13);
            if (rank == 0 && index != 0) {
                rankIteration++;
            }
            var suit = (Suits)(rankIteration);

            index++;
            yield return new Card(rank, suit);
        }
    }

    void CreateSortedCards() {
        foreach (var card in GetSortedCardsBySuits()) {
            deck.Add(card);
        }
    }

    void Start() {
        CreateSortedCards();
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Comments are not for extended discussion; the conversation has been moved to chat. \$\endgroup\$
    – Peilonrayz
    May 26, 2021 at 18:29
  • \$\begingroup\$ My Blackjack example includes Deck, Suit, and Card classes which you might want to check out. \$\endgroup\$
    – Aron
    Jun 10, 2021 at 21:05

4 Answers 4

19
\$\begingroup\$

You have nicely read the rank count into the variable rankCount, but later, you are writing index % 13. It is not immediately clear where this number comes from. Use the variable there too.

Since these numbers never change and are well known for a 52-card deck, you could also simply define them as constants.

const int RankCount = 13;
const int SuitCount = 4;
const int DeckCount = RankCount * SuitCount;  

I usually do not use var for built in types. You do not save a lot of typing by writing int or string instead of var and it is easier to read.


The two integer operators % and / are complementary and play well together.

var rank = (Ranks)(index % RankCount);
var suit = (Suits)(index / RankCount);

Note that the integer division truncates the result. This makes the rankIteration variable superfluous.

Alternatively, you could also use two nested loops, looping over suits and ranks. Using foreach makes all the indexes and suit and rank index calculations as well as the related constants superfluous. Additionally, foreach automatically casts the values to match the type of the loop variable:

IEnumerable<Card> GetSortedCardsBySuits()
{
    foreach (Suits suit in Enum.GetValues(typeof(Suits))) {
        foreach (Ranks rank in Enum.GetValues(typeof(Ranks))) {
            yield return new Card(rank, suit);
        }
    }
}

Using List<T>.AddRange() makes the explicit loop superfluous

void CreateSortedCards()
{
    deck.AddRange(GetSortedCardsBySuits());
}

There is a minor naming problem here. The deck list is created elsewhere. Here it is only initialized. Since the list has a constructor overload List<T>(IEnumerable<T>), we could create and initialize the deck in the class constructor instead (or, if we make CreateSortedCards static, also in an initializer)

private readonly List<Card> deck;

public Standard52Deck()
{
    deck = new List<Card>(GetSortedCardsBySuits());
}

read-only fields can be initialized in an initializer or in a constructor. This also ensures that there is always an initialized card deck.


The canonical way of looping a number range is (instead of the while-loop)

for (int index = 0; index < DeckCount; index++)
{
    ...
}

It unifies and standardizes the declaration and initialization of the loop variable, the loop condition and incrementing the loop variable. Also, it scopes the loop variable locally to the loop. However, I prefer the foreach approach mentioned earlier.


Note that these are only suggestions. There are always different ways to approach a problem.

\$\endgroup\$
20
\$\begingroup\$
class Card {
    readonly Ranks rank;
    readonly Suits suit;
    // ...
}

In this type definition, there is a grammatical mismatch: Ranks rank has a plural type name but a singular variable name.

The types Ranks and Suits should be renamed to Rank and Suit, to make the code more consistent.

\$\endgroup\$
12
\$\begingroup\$

It always seems odd in code to see a constant whose name is a number and for it to have a value different to that number.

You can set a value on an enum and the later ones automatically increment thereafter, so writing:

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

means that 9 == (int) Nine is true, which is less weird than Nine being eight.

\$\endgroup\$
0
5
\$\begingroup\$

Note: The traditional order of suits is (highest to lowest) Spades, Hearts, Diamonds, Clubs.

While this order does not matter for many games, there are also many games for which it does matter. However, even in games where the order does not matter, it is still understood to be the traditional implicit order in almost all of them (the game of Hearts may be an exception).

Therefore, I would suggest changing the order in your suits enum Suits to fit.

\$\endgroup\$
3
  • \$\begingroup\$ By the same token, I'd also consider putting Ace after King. I mean, it depends on the game, but most card games I know have aces high. Might have to make different versions of the class to support either ordering. Aces will probably require special handling in many games no matter what. \$\endgroup\$ May 24, 2021 at 15:30
  • 3
    \$\begingroup\$ @DarrelHoffman Ace is a particular problem because in many card games it's value is dynamic: it can change from high to low depending on circumstances. However it's denomination is 1 and the traditional ordering is numerical which makes it first (i.e., before two). In most card game programs I've seen, the Ace is enumerated first with a value of one. Sometimes, if the Ace is always high, then then the two is enumerated first with an explicit value of 2 and the Ace is last (with an implict value of 14). Since this appears to be intended for use in multiple games, Ace=1 seems right. \$\endgroup\$ May 24, 2021 at 15:59
  • \$\begingroup\$ I find your answer interesting but a deck of playing cards should be oblivious to ranking of suits. That should be left to a Game object, which is not defined here. Consider different games like Poker, Blackjack, FreeCell, or Crazy Eights all treat the suits differently. A Game object would define the rules of a particular game. Suits alphabetically would be C-D-H-S. Microsoft Charmap has them as ♠♣♥♦, or S-C-H-D. And this site has them as H-C-D-S from top to bottom: ambitiouswithcards.com/new-deck-order \$\endgroup\$
    – Rick Davin
    May 27, 2021 at 19:54

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.