# Poker Hand Evaluator Challenge

Posted on

Problem

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,
}
``````

…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()
{

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 }
};
}
}
``````

Solution

``````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 `enum`s 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.

## 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;
}
}
``````