Poker Hand Evaluator Challenge

Question

This week's review challenge is a poker hand evaluator. I started by enumerating the possible hands:

public enum PokerHands
{
    Pair,
    TwoPair,
    ThreeOfKind,
    Straight,
    Flush,
    FullHouse,
    FourOfKind,
    StraightFlush,
    RoyalFlush
}

Then I thought I was going to need cards... and cards have a suit...

public enum CardSuit
{
    Hearts,
    Diamonds,
    Clubs,
    Spades
}

...and a nominal value:

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

So I had enough of a concept to formally define a PlayingCard:

public class PlayingCard 
{
    public CardSuit Suit { get; private set; }
    public PlayingCardNominalValue NominalValue { get; private set; }

    public PlayingCard(CardSuit suit, PlayingCardNominalValue nominalValue)
    {
        Suit = suit;
        NominalValue = nominalValue;
    }
}

At this point I had everything I need to write my actual Poker hand evaluator - because the last time I implemented this (like, 10 years ago) it was in VB6 and that I'm now spoiled with .net, I decided to leverage LINQ here:

public class PokerGame
{
    private readonly IDictionary<PokerHands, Func<IEnumerable<PlayingCard>, bool>> _rules;
    public IDictionary<PokerHands, Func<IEnumerable<PlayingCard>, bool>> Rules { get { return _rules; } }

    public PokerGame()
    {
        // overly verbose for readability

        Func<IEnumerable<PlayingCard>, bool> hasPair = 
                              cards => cards.GroupBy(card => card.NominalValue)
                                            .Count(group => group.Count() == 2) == 1;

        Func<IEnumerable<PlayingCard>, bool> isPair = 
                              cards => cards.GroupBy(card => card.NominalValue)
                                            .Count(group => group.Count() == 3) == 0
                                       && hasPair(cards);

        Func<IEnumerable<PlayingCard>, bool> isTwoPair = 
                              cards => cards.GroupBy(card => card.NominalValue)
                                            .Count(group => group.Count() >= 2) == 2;

        Func<IEnumerable<PlayingCard>, bool> isStraight = 
                              cards => cards.GroupBy(card => card.NominalValue)
                                            .Count() == cards.Count()
                                       && cards.Max(card => (int) card.NominalValue) 
                                        - cards.Min(card => (int) card.NominalValue) == 4;

        Func<IEnumerable<PlayingCard>, bool> hasThreeOfKind = 
                              cards => cards.GroupBy(card => card.NominalValue)
                                            .Any(group => group.Count() == 3);

        Func<IEnumerable<PlayingCard>, bool> isThreeOfKind = 
                              cards => hasThreeOfKind(cards) && !hasPair(cards);

        Func<IEnumerable<PlayingCard>, bool> isFlush = 
                              cards => cards.GroupBy(card => card.Suit).Count() == 1;

        Func<IEnumerable<PlayingCard>, bool> isFourOfKind = 
                              cards => cards.GroupBy(card => card.NominalValue)
                                            .Any(group => group.Count() == 4);

        Func<IEnumerable<PlayingCard>, bool> isFullHouse = 
                              cards => hasPair(cards) && hasThreeOfKind(cards);

        Func<IEnumerable<PlayingCard>, bool> hasStraightFlush = 
                              cards =>isFlush(cards) && isStraight(cards);

        Func<IEnumerable<PlayingCard>, bool> isRoyalFlush = 
                              cards => cards.Min(card => (int)card.NominalValue) == (int)PlayingCardNominalValue.Ten
                                       && hasStraightFlush(cards);

        Func<IEnumerable<PlayingCard>, bool> isStraightFlush = 
                              cards => hasStraightFlush(cards) && !isRoyalFlush(cards);


        _rules = new Dictionary<PokerHands, Func<IEnumerable<PlayingCard>, bool>>
                     {
                         { PokerHands.Pair, isPair },
                         { PokerHands.TwoPair, isTwoPair },
                         { PokerHands.ThreeOfKind, isThreeOfKind },
                         { PokerHands.Straight, isStraight },
                         { PokerHands.Flush, isFlush },
                         { PokerHands.FullHouse, isFullHouse },
                         { PokerHands.FourOfKind, isFourOfKind },
                         { PokerHands.StraightFlush, isStraightFlush },
                         { PokerHands.RoyalFlush, isRoyalFlush }
                     };
    }
}

Answer 1

public enum PokerHands

This type should be called in singular (PokerHand). When you have a variable of this type, it represents a single hand, not some collection of hands. Your other enums are named correctly in this regard.

public enum CardSuit
public enum PlayingCardNominalValue

You should be consistent. Either start both types with PlayingCard or both with Card. I prefer the former, because this is a playing card library, there is not much chance of confusion with credit cards or other kinds of cards.

private readonly IDictionary<PokerHands, Func<IEnumerable<PlayingCard>, bool>> _rules;
public IDictionary<PokerHands, Func<IEnumerable<PlayingCard>, bool>> Rules { get { return _rules; } }

I would use auto-property with private setter (like you did in PlayingCard) here. It won't enforce the readonly constraint, but I think shorter code is worth that here.

Also, this pretty dangerous code, any user of this class can modify the dictionary. If you're using .Net 4.5, you could change the type to IReadOnlyDictionary (and if you wanted to avoid modifying by casting back to IDictionary, also wrap it in ReadOnlyDictionary).

One more thing: I question whether IDictionary is actually the right type here. I believe that the common operation would be to find the hand for a collection of cards, not finding out whether a given hand matches the cards.

// overly verbose for readability

I agree that the lambdas are overly verbose, but I'm not sure it actually helps readability. What I don't like the most is all of the GroupBy() repetition. What you could do is to create an intermediate data structure that would contain the groups by NominalValue and anything else you need and then use that in your lambdas.

Func<IEnumerable<PlayingCard>, bool> isStraight = 
                      cards => cards.GroupBy(card => card.NominalValue)
                                    .Count() == cards.Count()
                               && cards.Max(card => (int) card.NominalValue) 
                                - cards.Min(card => (int) card.NominalValue) == 4;

What is the cards.Count() supposed to mean? Don't there always have to be 5 cards? The second part of this lambda seems to indicate that.

Answer 2

The Design may be Incomplete

• The readability concern in the PokerGame class can be addressed by encapsulation instead of syntactical slight of hand. Push details down is the mantra.
• Addresses the concern about a fungible Dictionary by enveloping (encapsulating) it.

Would a HandEvaluator class be helpful?

• Maybe the HandEvaluator class could be exposed through CardDeck, i.e. the PokerGame talks to the CardDeck only; anyway ...
• The "grunt work" of isPair, etc. could go in the HandEvaluator.
• Sure, the PokerGame may need to know if there is a pair, but ask and return a PokerHand to the general question: PokerHand myHand = HandEvaluator.WhatDoIHave(myHand);

Would a Hand class be helpful?

public class Hand {
    public PokerHand Hand {get; set;}
    public Suit Suit {get; set;}

    public List<Card> Cards {get; set;}

    public override string ToString() {
        return string.Format("A {0} of {1}", Hand.ToString(), Suit.ToString());
    }
}

CardDeck.WhatDoIHave(JoesHand);
Console.Writeline("{0} has {1}", player1.Name, JoesHand);

• Now a Hand is defined, encapsulated, and expressive.
• We can now see that the enum PokerHand falls short of really defining a "hand of poker"
• I suspect that the _rules dictionary becomes superfluous. Seems to me all it does is "translate" the enum value.

Just One more thing(s)

• Are the CardSuit in the right order? Clubs = 1, Diamonds, Hearts, Spades. This is a tie breaker when, for example, both two players have a royal flush - which has happened once in a pro poker tourney.
• Highest Card is missing from enum PokerHands
• Make determining the winner look like magic. See below.

List<Hand> playerHands; 
// ... play a hand, then this ...

playerHands.Sort();  // the winner is on top.

// because in the `Hand` class...

public Hand : IComparable<Hand> {
    public int CompareTo (Hand other) {
        // Assuming HandEvaluator has already done it's thing.

        if (other == null) return 1;
        if (this.Hand > other.Hand) return 1;
        if (this.Hand == other.Hand && this.Suit > other.Suit) return 1;
        if (this.Hand == other.Hand && this.Suit < other.Suit) return -1;
        if (this.Hand < other.Hand) return -1;
        return 0;
    }
}