4
\$\begingroup\$

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 }
                     };
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ What are you trying to do? Determine what hand a person has? \$\endgroup\$ – dreza Dec 7 '13 at 8:54
  • 1
    \$\begingroup\$ Your evaluator doesn't have an evaluate method. \$\endgroup\$ – abuzittin gillifirca Dec 7 '13 at 9:46
  • 1
    \$\begingroup\$ Also is buggy: new PokerGame().Rules[PokerHands.Straight](new PlayingCard[]{ new PlayingCard(CardSuit.Hearts, PlayingCardNominalValue.Ace), new PlayingCard(CardSuit.Clubs, PlayingCardNominalValue.Two), new PlayingCard(CardSuit.Clubs, PlayingCardNominalValue.Three), new PlayingCard(CardSuit.Clubs, PlayingCardNominalValue.Four), new PlayingCard(CardSuit.Clubs, PlayingCardNominalValue.Five), }) == false \$\endgroup\$ – abuzittin gillifirca Dec 7 '13 at 9:51
  • \$\begingroup\$ @abuzittingillifirca oh shoot, I only accounted the Ace as a 14!! \$\endgroup\$ – Mathieu Guindon Dec 7 '13 at 13:58
  • 1
    \$\begingroup\$ Down-voted this question by request \$\endgroup\$ – Simon Forsberg Dec 8 '13 at 20:00
5
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ I didn't hard-code the 5 because depending on the actual game cards.Count() could also be 7. Thanks for the review, I admit I posted quite too early this week (the minute all tests passed), it shows. \$\endgroup\$ – Mathieu Guindon Dec 7 '13 at 14:00
  • 1
    \$\begingroup\$ @retailcoder So, 2,3,4,5,6,8,9 is not a straight, but 2,2,3,4,5,6,6 is? I think that's a bug. \$\endgroup\$ – svick Dec 7 '13 at 14:09
  • \$\begingroup\$ Ugh. I shouldn't have rushed this. TBH I'm not very happy with this code, it's not my best possible code and I feel like saying "scratch that, I'll post again when it meets my quality bar" - I didn't even do a single pass of refactoring, it's really the 1st "draft"... mainly I wanted to post my solution before @rolfl posted his; and it's not a race. :/ \$\endgroup\$ – Mathieu Guindon Dec 7 '13 at 14:26
5
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I do have a CardDeck, I edited it out because it wasn't relevant to the evaluation of the Poker hands :) Check out the take 2 follow-up post.. \$\endgroup\$ – Mathieu Guindon Dec 7 '13 at 21:14
  • \$\begingroup\$ @retailcoder, I agree. I fixed my answer \$\endgroup\$ – radarbob Dec 7 '13 at 21:27
  • \$\begingroup\$ Rewrote CompareTo. Removed reference to HandEvaluator. Should be comparing ONLY in the context of the given class. A design flaw otherwise. \$\endgroup\$ – radarbob Dec 7 '13 at 21:55
  • 4
    \$\begingroup\$ Suits do not break tie. Highest hands that only differ in suits split the pot. There might be variants of course. \$\endgroup\$ – abuzittin gillifirca Dec 8 '13 at 7:39
  • 1
    \$\begingroup\$ There is no casino or cardroom anywhere in the country that uses suits to break ties between poker hands. Tied hands split the pot, period. Suits are used for things like deciding who pays a bring-in, who deals first, who gets the extra chip in a split pot, and other things. For these uses, the suit ranking you have listed (C, H, D, S) is correct. \$\endgroup\$ – Lee Daniel Crocker Oct 28 '14 at 22:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.