Poker Hand Evaluator, take 2

Question

This is following up on my previous attempt, which was admittedly done fast and not-so-well. This code attempts to allow comparing two poker hands to determine a winner, not only evaluating a given set of cards. (this was actually edited - see original take 2 here).

I'm abstracting the dirt away into an interface... and yet it doesn't look very clean to me, but maybe it's just because I don't use Tuple<T1,T2> very often. T1 is a bool indicating whether the hand is matched, T2 contains the winning cards.

public interface IPokerHandEvaluator
{
    Tuple<bool, IEnumerable<PlayingCard>> IsPair(IEnumerable<PlayingCard> cards);
    Tuple<bool, IEnumerable<PlayingCard>> IsTwoPair(IEnumerable<PlayingCard> cards);
    Tuple<bool, IEnumerable<PlayingCard>> IsThreeOfKind(IEnumerable<PlayingCard> cards);
    Tuple<bool, IEnumerable<PlayingCard>> IsFourOfKind(IEnumerable<PlayingCard> cards);
    Tuple<bool, IEnumerable<PlayingCard>> IsFlush(IEnumerable<PlayingCard> cards);
    Tuple<bool, IEnumerable<PlayingCard>> IsFullHouse(IEnumerable<PlayingCard> cards);
    Tuple<bool, IEnumerable<PlayingCard>> IsStraight(IEnumerable<PlayingCard> cards);
    Tuple<bool, IEnumerable<PlayingCard>> IsStraightFlush(IEnumerable<PlayingCard> cards);
    Tuple<bool, IEnumerable<PlayingCard>> IsRoyalFlush(IEnumerable<PlayingCard> cards);
}

This is my current implementation - it correctly evaluates all 5-card hands, and I've tested it correctly evaluates a Straight Flush (ace low) with a 7-card hand as well. So it works, but it doesn't make a good marketing for Tuple<T1,T2>:

public class PokerHandEvaluator : IPokerHandEvaluator
{
    private IEnumerable<IGrouping<CardNominalValue, PlayingCard>> GroupByNominalValue(IEnumerable<PlayingCard> cards)
    {
        return cards.GroupBy(card => card.NominalValue);
    }

    private IEnumerable<IGrouping<CardNominalValue, PlayingCard>> Pairs(IEnumerable<PlayingCard> cards)
    {
        return GroupByNominalValue(cards).Where(group => group.Count() == 2);
    }

    private IEnumerable<IGrouping<CardNominalValue, PlayingCard>> Triplets(IEnumerable<PlayingCard> cards)
    {
        return GroupByNominalValue(cards).Where(group => group.Count() == 3);
    }

    private IEnumerable<IGrouping<CardSuit, PlayingCard>> Suits(IEnumerable<PlayingCard> cards)
    {
        return cards.GroupBy(card => card.Suit);
    }

    public Tuple<bool,IEnumerable<PlayingCard>> IsPair(IEnumerable<PlayingCard> cards)
    {
        var pairs = Pairs(cards);

        var result = pairs.Count() == 1 && !Triplets(cards).Any();
        var winningCards = result ? pairs.SelectMany(group => group.Select(card => card)) : null;

        return new Tuple<bool, IEnumerable<PlayingCard>>(result, winningCards);
    }

    public Tuple<bool, IEnumerable<PlayingCard>> IsTwoPair(IEnumerable<PlayingCard> cards)
    {
        var pairs = Pairs(cards);

        var result = pairs.Count() == 2;
        var winningCards = result ? pairs.SelectMany(group => group.Select(card => card)) : null;

        return new Tuple<bool, IEnumerable<PlayingCard>>(result, winningCards);
    }

    public Tuple<bool, IEnumerable<PlayingCard>> IsThreeOfKind(IEnumerable<PlayingCard> cards)
    {
        var triplets = Triplets(cards);

        var result = triplets.Count() == 1 && !Pairs(cards).Any();
        var winningCards = result ? triplets.SelectMany(group => group.Select(card => card)) : null;

        return new Tuple<bool, IEnumerable<PlayingCard>>(result, winningCards);
    }

    public Tuple<bool, IEnumerable<PlayingCard>> IsFourOfKind(IEnumerable<PlayingCard> cards)
    {
        var values = GroupByNominalValue(cards);

        var result = values.Any(group => group.Count() >= 4);
        var winningCards = result ? values.Where(group => group.Count() >= 4).SelectMany(group => group.Select(card => card)) : null;

        return new Tuple<bool, IEnumerable<PlayingCard>>(result, winningCards);
    }

    public Tuple<bool, IEnumerable<PlayingCard>> IsFlush(IEnumerable<PlayingCard> cards)
    {
        var winningSuit = Suits(cards).Where(suit => suit.Count() >= 5);

        var result = winningSuit.Any();
        var winningCards = result ? winningSuit.SelectMany(group => group.Select(card => card)) : null;

        return new Tuple<bool, IEnumerable<PlayingCard>>(result, winningCards);
    }

    public Tuple<bool, IEnumerable<PlayingCard>> IsFullHouse(IEnumerable<PlayingCard> cards)
    {
        var pairs = Pairs(cards);
        var triplets = Triplets(cards);

        var result = pairs.Any() && triplets.Any();
        if (!result) return new Tuple<bool, IEnumerable<PlayingCard>>(false, null);
        var winningPair = pairs.OrderByDescending(group => group.Key).FirstOrDefault().Select(card => card);
        var winningTriplet = triplets.OrderByDescending(group => group.Key).FirstOrDefault().Select(card => card);

        var winningCards = winningPair.Concat(winningTriplet);
        return new Tuple<bool, IEnumerable<PlayingCard>>(true, winningCards);
    }

    public Tuple<bool, IEnumerable<PlayingCard>> IsStraight(IEnumerable<PlayingCard> cards)
    {
        var distinctValues = GroupByNominalValue(cards);
        var isStraightNoAce = IsStraightNoAce(distinctValues);

        if (isStraightNoAce.Item1)
        {
            return new Tuple<bool, IEnumerable<PlayingCard>>(true, isStraightNoAce.Item2);
        }

        var isStraightWithAce = IsStraightWithAce(distinctValues);
        if (isStraightWithAce.Item1.Item1)
        {
            return new Tuple<bool, IEnumerable<PlayingCard>>(true, isStraightWithAce.Item1.Item2);
        }

        return new Tuple<bool, IEnumerable<PlayingCard>>(false, null);
    }

    private enum AceKind
    {
        AceLow,
        AceHigh
    }

    private Tuple<Tuple<bool, IEnumerable<PlayingCard>>, AceKind?> IsStraightWithAce(IEnumerable<IGrouping<CardNominalValue, PlayingCard>> distinctValues)
    {
        // if has no ace, don't bother
        if (!distinctValues.Any(group => group.Key == CardNominalValue.Ace)) return new Tuple<Tuple<bool, IEnumerable<PlayingCard>>, AceKind?>(new Tuple<bool, IEnumerable<PlayingCard>>(false, null), null);

        var ace = distinctValues.Where(group => group.Key == CardNominalValue.Ace).Select(group => group.First()).First();
        var aceRemoved = distinctValues.Where(group => group.Key != CardNominalValue.Ace);

        var ascending = aceRemoved.OrderBy(value => value.Key).Take(4);
        if (ascending.Max(group => group.Key) == CardNominalValue.Five)
        {
            var winningCards = ascending.Select(group => group.First()).Concat(new List<PlayingCard> {ace});
            var result = new Tuple<bool, IEnumerable<PlayingCard>>(true, winningCards);
            return new Tuple<Tuple<bool, IEnumerable<PlayingCard>>, AceKind?>(result, AceKind.AceLow);
        }

        var descending = aceRemoved.OrderByDescending(value => value.Key).Take(5);
        if (descending.Min(group => group.Key) == CardNominalValue.Ten)
        {
            var winningCards = descending.Select(group => group.First());
            var result = new Tuple<bool, IEnumerable<PlayingCard>>(true, winningCards);
            return new Tuple<Tuple<bool, IEnumerable<PlayingCard>>, AceKind?>(result, AceKind.AceHigh);
        }

        return new Tuple<Tuple<bool, IEnumerable<PlayingCard>>, AceKind?>(new Tuple<bool, IEnumerable<PlayingCard>>(false, null), null);
    }

    private Tuple<Tuple<bool, IEnumerable<PlayingCard>>, AceKind?> IsStraightWithAce(IEnumerable<PlayingCard> cards)
    {
        return IsStraightWithAce(GroupByNominalValue(cards));
    }

    private Tuple<bool, IEnumerable<PlayingCard>> IsStraightNoAce(IEnumerable<IGrouping<CardNominalValue, PlayingCard>> distinctValues)
    {
        // if has ace, don't bother
        if (distinctValues.Any(group => group.Key == CardNominalValue.Ace)) return new Tuple<bool, IEnumerable<PlayingCard>>(false, null);

        var sortedValues = distinctValues.OrderBy(group => (int)group.Key);
        var possibleLows = sortedValues.Take(5 - sortedValues.Count() + 1);

        var skippedCards = 0;
        foreach (var possibleLow in possibleLows)
        {
            var theCards = sortedValues.Skip(skippedCards).Take(5).Select(group => group.First());
            var isStraight = theCards.Max(card => card.NominalValue) == possibleLow.Key + 4;

            if (isStraight)
            {
                return new Tuple<bool, IEnumerable<PlayingCard>>(true, theCards);
            }
            skippedCards++;
        }

        return new Tuple<bool, IEnumerable<PlayingCard>>(false, null);
    }

    public Tuple<bool, IEnumerable<PlayingCard>> IsStraightFlush(IEnumerable<PlayingCard> cards)
    {
        var isFlush = IsFlush(cards);
        if (!isFlush.Item1) return new Tuple<bool, IEnumerable<PlayingCard>>(false, null);

        var isStraight = IsStraight(cards);
        if (!isStraight.Item1) return new Tuple<bool, IEnumerable<PlayingCard>>(false, null);

        return new Tuple<bool, IEnumerable<PlayingCard>>(true, isStraight.Item2);
    }

    public Tuple<bool, IEnumerable<PlayingCard>> IsRoyalFlush(IEnumerable<PlayingCard> cards)
    {
        var straightWithAceResult = IsStraightWithAce(cards);
        var result = IsFlush(cards).Item1 && straightWithAceResult.Item1.Item1 && straightWithAceResult.Item2 == AceKind.AceHigh;
        return new Tuple<bool, IEnumerable<PlayingCard>>(result, straightWithAceResult.Item1.Item2);
    }
}

So the PokerGame class is now much more focused at knowing what the rules are and in which order they're evaluated; the dictionary of rules/hands has turned into a private field, there's no need to expose it at all - given the new PokerHand EvaluateHand() method:

public class PokerGame
{
    private readonly IPokerHandEvaluator _evaluator;
    private readonly Dictionary<PokerHand, Func<IEnumerable<PlayingCard>, Tuple<bool, IEnumerable<PlayingCard>>>> _hands;

    public PokerGame(IPokerHandEvaluator evaluator)
    {
        _evaluator = evaluator;
        _hands = new Dictionary<PokerHand, Func<IEnumerable<PlayingCard>, Tuple<bool, IEnumerable<PlayingCard>>>>
                     {
                         { PokerHand.HighCard, cards => new Tuple<bool, IEnumerable<PlayingCard>>(true, null) },
                         { PokerHand.Pair, _evaluator.IsPair },
                         { PokerHand.TwoPair, _evaluator.IsTwoPair },
                         { PokerHand.ThreeOfKind, _evaluator.IsThreeOfKind },
                         { PokerHand.Straight, _evaluator.IsStraight },
                         { PokerHand.Flush, _evaluator.IsFlush },
                         { PokerHand.FullHouse, _evaluator.IsFullHouse },
                         { PokerHand.FourOfKind, _evaluator.IsFourOfKind },
                         { PokerHand.StraightFlush, _evaluator.IsStraightFlush },
                         { PokerHand.RoyalFlush, _evaluator.IsRoyalFlush }
                     };
    }

    public PokerHandEvaluationResult EvaluateHand(IEnumerable<PlayingCard> cards)
    {
        var winningHand = _hands.OrderByDescending(hand => hand.Key)
                                    .FirstOrDefault(hand => hand.Value.Invoke(cards).Item1).Key;

        var redundantCall = _hands[winningHand].Invoke(cards);
        var winningCards = redundantCall.Item2;
        var otherCards = cards.Where(card => !winningCards.Contains(card));

        return new PokerHandEvaluationResult(winningHand, winningCards, otherCards);
    }
}

For this to work I had to add PokerHand.HighCard:

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

The CardSuit enum was reordered, in prevision for when I want to modify this code to compare two hands - the suit would be a tiebreaker:

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

I also added a PokerHandEvaluationResult class:

public class PokerHandEvaluationResult : IComparable<PokerHandEvaluationResult>
{
    public PokerHand Result { get; private set; }
    public IEnumerable<PlayingCard> ResultCards { get; private set; }
    public IEnumerable<PlayingCard> HighCards { get; private set; }

    public PokerHandEvaluationResult(PokerHand result, IEnumerable<PlayingCard> resultCards, IEnumerable<PlayingCard> highCards)
    {
        Result = result;
        ResultCards = resultCards;
        HighCards = highCards;
    }

    public int CompareTo(PokerHandEvaluationResult other)
    {
        int result;
        if (Result == other.Result)
        {
            result = ResultCards == null ? 0 : ResultCards.Max().CompareTo(other.ResultCards.Max());
            if (result == 0) result = HighCards.Max().CompareTo(other.HighCards.Max());
        }
        else
        {
            result = Result.CompareTo(other.Result);
        }

        return result;
    }
}

---

So I'm not sure whether this code is getting better, or if it's taking a direction that will make me wish we had implemented a Black Jack game this week. I think Tuple feels like a hack, that the code is begging for something but I can't put my finger on it. Could it be the enums shooting myself in the foot?

Answer 1

1. The idiomatic way of creating Tuples is to use the Create factory method. This should remove a fair amount of type clutter from the code.

2. Tuples are great when you want to bunch up some values and pass them around together. Unfortunately the Tuple properties Item1, Item2, etc. are not very descriptive. You also use the the same type of Tuple in a lot of places so I think the excessive use of it could warrant its own class.

3. Your PokerGame class is a weird abstraction. A poker game consists of 2 or more players, each of them having a hand and the one with the highest wins. Your PokerGame class evaluates only a single hand. PokerPlayer or so might have been a better name.

Answer 2

I have this in my hand class so you can see a very simple algorithm This calculates it all in one spot For the sake a brevity I left out some parts out

The reason for the new solution is to show the simplicity of the problem instead of making it overly complex. The old solution was doing was doing way to much work

This sets a property HandRank

/// <summary>
/// Possible hands
/// </summary>
public enum HandType
{
    HighCard = 1,
    Pair,
    TwoPair,
    ThreeOfAKind,
    Straight,
    Flush,
    FullHouse,
    FourOfAKind,
    StraightFlush
}

    // create holders for rank and suit counts
    private int[] suitCount = new int[NumSuits];
    private int[] rankCount = new int[RanksPerSuit];

    /// <summary>
    /// Evaluate Hand
    /// Set HandRank. AceLow, OrderedCards
    /// </summary>
    public void EvaluateHand()
    {
        // default to Ace High
        AceIsLow = false;

        for (int index = 0; index < suitCount.Length; index++)
        {
            suitCount[index] = 0;
        }
        for (int index = 0; index < rankCount.Length; index++)
        {
            rankCount[index] = 0;
        }

        // init result to just a Hight Card
        var result = HandType.HighCard;

        // count the ranks and suits
        foreach (var card in Cards)
        {
            suitCount[(int)card.Suit]++;
            rankCount[(int)card.Rank]++;
        }

        // These are used to determine a straight
        var numInaARow = 0;
        var maxNumInARow = 0;

        // loop through each rank count
        for (var rankIndex = 0; rankIndex < RanksPerSuit; rankIndex++)
        {
            // count of cards that are the rank rankIndex
            var count = rankCount[rankIndex];

            // if the count is 1 then increment num in a row
            // if max num in a row is 5 then we have a straight
            if (count == 1)
            {
                // increment num in a row
                numInaARow++;
                if (numInaARow > maxNumInARow)
                {
                    // set max num in a row
                    maxNumInARow = numInaARow;
                }
            }
            else
            {
                // reset counter to 0
                numInaARow = 0;
            }

            // look for pairs, two pairs, three of a kind, full house or four of a kind 
            switch (count)
            {
                case 2:
                    // we have a pair
                    // if pair is already set then this is a second pair
                    if (result == HandType.Pair)
                    {
                        result = HandType.TwoPair;
                    }
                    // if we already have three of a kind then we have a full house
                    else if (result == HandType.ThreeOfAKind)
                    {
                        result = HandType.FullHouse;
                    }
                    else
                    {
                        //  just a pair
                        result = HandType.Pair;
                    }
                    break;

                case 3:
                    // we have three of a kind
                    // if we already have a low pair  we have a full house 
                    result = (result == HandType.Pair) ? HandType.FullHouse : HandType.ThreeOfAKind;
                    break;

                case 4:
                    // we have four of a kind
                    result = HandType.FourOfAKind;
                    break;
            }
        }

        // check for a straight
        var straight = maxNumInARow == CardsPerHand;
        var aceHightStraight = (maxNumInARow == (CardsPerHand - 1)) && (rankCount[(int)Rank.Ace] == 1 && rankCount[(int)Rank.Ten] == 1 && rankCount[(int)Rank.King] == 1);
        if (straight || aceHightStraight)
        {
            result = HandType.Straight;
            if (straight && rankCount[(int)Rank.Ace] == 1)
            {
                AceIsLow = true;
            }
        }

        // check for a flush
        foreach (var count in suitCount)
        {
            switch (count)
            {
                case CardsPerHand:
                    //  if we already have a straight then we have a straight flush
                    result = result == HandType.Straight ? HandType.StraightFlush : HandType.Flush;
                    break;
            }
        }
        // save the result
        HandRank = result;
    }