Poker Hand Evaluator Challenge

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, bool>> _rules;
public IDictionary, bool>> Rules { get { return _rules; } }

public PokerGame()
{
// overly verbose for readability

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

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

Func, bool> isTwoPair =
cards => cards.GroupBy(card => card.NominalValue)

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, bool>> _rules;
public IDictionary, 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, 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.

StackExchange Code Review Q#36841, answer score: 5