1

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();
    }
}
5
  • What's your actual question? Nov 6, 2016 at 6:59
  • @PhilipKendall I'm trying to find out if the code structure could be improved upon, mostly around the relationship between how the Deck class and the two derived classes relate to each other, and how they are constructed using the DeckFactory. I'll add this to the main question.
    – Ayb4btu
    Nov 6, 2016 at 7:06
  • 3
    Your question might be better suited for Code Review (but don't crosspost).
    – Doc Brown
    Nov 6, 2016 at 7:56
  • @DocBrown I'm always a little confused as to where these sorts of questions are best suited. Are there ways of transferring them to another site?
    – Ayb4btu
    Nov 6, 2016 at 7:58
  • @Ayb4btu: I think your question could be on topic on both sites. It is just a suggestion because you might get better answers there. Leave it here if the answers you get here are pleasing you. For migration to another site, flag your question for moderator invention, you then can leave a short statement asking for migration.
    – Doc Brown
    Nov 6, 2016 at 8:03

1 Answer 1

3

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.

6
  • So the two different ways to instantiate the deck would be to have a Deck constructor with a single GameType parameter, and do away with the factory? Or do you recommend a different way of doing this?
    – Ayb4btu
    Nov 6, 2016 at 7:42
  • @Ayb4btu The way I would do it is to have static CreateBasicDeck and CreateAdvancedDeck on Deck class. Both of which will return plain Deck class.
    – Euphoric
    Nov 6, 2016 at 7:44
  • The Deck constructor would then have two parameters? One each for the Suit and Number lists? Also, how would I make it so that Deck can only be created as a standard or advanced deck, or is this not a good idea? A quick code example might be helpful.
    – Ayb4btu
    Nov 6, 2016 at 7:52
  • @Ayb4btu Yes, basically. And if you make Deck constructor private, then it will only can be created from Create methods, guaranteeing it can only be created as Basic and Advanced.
    – Euphoric
    Nov 6, 2016 at 7:57
  • After your suggestion of a static create method I looked at private constructors. I think what threw me was that MSDN says that they are generally only used in classes that only contain static members. But I guess this is an exception.
    – Ayb4btu
    Nov 6, 2016 at 8:04

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.