Hanabi card game: Correctly structuring the deck code

Question

I'm trying to improve my understanding and ability to write code that uses recommended principles and practices, such as the SOLID principles. To do this, I am implementing the fireworks card game Hanabi.

Hanabi has two deck types, Standard and Advanced (which has an extra set of MultiColour cards). Each card in a deck I'm using a DeckFactory to construct a Deck. I'm looking for feedback on how the code structure could be improved, mostly around the relationship between how the Deck class and how the two derived classes (StandardDeck and AdvancedDeck) relate to each other, and their construction using the DeckFactory.

One of my concerns is that AssembleDeck (in the parent Deck class) needs to be called from each derived class and whether it should be in the Deck class at all.

A second concern is the use of a Stack to hold the cards; it is more appropriate for drawing a card from the deck, but maybe less so for when the deck is shuffled. I'm thinking of using the Fisher-Yates algorithm for shuffling.

I'm also wondering if there is anything in the code structure that needs to be changed so that unit tests are easier to write.

The card object:

public enum Suit { White, Red, Yellow, Green, Blue, MultiColour }
public enum Number { One, Two, Three, Four, Five }

public class FireworksCard {
    public Suit Suit { get; }
    public Number Number { get; }

    public FireworksCard(Suit suit, Number number) {
        Suit = suit;
        Number = number;
    }
}

The Deck structure:

public interface IDeck {
    FireworksCard DrawCard();
    void Shuffle();
}

public abstract class Deck : IDeck {
    protected Stack<FireworksCard> Cards;

    public bool IsEmpty => !Cards.Any();

    public abstract IEnumerable<Suit> Suit { get; }
    public abstract IEnumerable<Number> Numbers { get; }

    protected void AssembleDeck() {
        Cards = new Stack<FireworksCard>();

        // generate a card for every combination
        foreach (var number in Numbers) {
            int cardRepetitions = GetCardRepetitions(number);

            for (int i = 0; i < cardRepetitions; i++) {
                GenerateAllSuitsForNumber(number);
            }
        }
    }

    private int GetCardRepetitions(Number number) {
        switch (number) {
            case Number.One:
                return 3;
            case Number.Five:
                return 1;
            default:
                return 2;
        }
    }

    private void GenerateAllSuitsForNumber(Number number) {
        foreach (var suit in Suits) {
            Cards.Push(new FireworksCard(suit, number));
        }
    }

    public void Shuffle() {
        // Randomly rearrange the order of Cards
    }

    public FireworksCard DrawCard() {
        return Cards.Pop();
    }
}

public class StandardDeck : Deck {
    public override IEnumerable<Suit> Suits => Enum.GetValues(typeof(Suit)).Cast<Suit>().Except(new[] { Suit.MultiColour });
    // What's better, this (Number[])Enum.GetValues(typeof(Number)); or this
    public override IEnumerable<Number> Numbers => Enum.GetValues(typeof(Number)).Cast<Number>();

    public StandardDeck() {
        AssembleDeck();
    }
}

public class AdvancedDeck : Deck {
    public override IEnumerable<Suit> Suits => Enum.GetValues(typeof(Suit)).Cast<Suit>();
    public override IEnumerable<Number> Numbers => Enum.GetValues(typeof(Number)).Cast<Number>();

    public AdvancedDeck() {
        AssembleDeck();
    }
}

The Deck factory:

public enum GameType { Standard, Advanced }

public class DeckFactory {
    public IDeck BuildDeck(GameType gameType) {
        switch (gameType) {
            default:
            case GameType.Standard:
                return BuildStandardDeck();
            case GameType.Advanced:
                return BuildAdvancedDeck();
        }
    }

    private IDeck BuildStandardDeck() {
        return new StandardDeck();
    }

    private IDeck BuildAdvancedDeck() {
        return new AdvancedDeck();
    }
}

Answer 1

Looking at the code snippets, I would say there is no need for Decks to be this complicated while using both interface and inheritance. If the only difference between decks is that Advanced deck has MultiColour cards, then have single Deck class and two different ways to instantiate it. There is no need for inheritance if their behavior is exactly same.

Another thing I would say is that you are focusing on completely wrong thing. You should be working on encoding rules of the game into classes. Not worry about Decks. You should focus on how cards interact with the board and each other.

If I were to do it top-down TDD-style, I would start with Game class, then I would go through rule-book and implement each rule as single/multiple tests on the Game class. If I found out that there is an abstraction to be made, I would extract it out and refactor the code. What you did is to spend quite a lot of time on mostly irrelevant problem and implement it in most complicated way.