Poker Hand evaluation

Question

I want to evaluate Poker Hands an wrote the following Code:

https://dotnetfiddle.net/mxMvq9

Anything wrong / to imporve?

public class Deck
{
    private Random rnd;
    private List<Card> DeckCards { get; set; }
    private int PlayerCount { get; set; }
    public Deck(int playerCount)
    {
        rnd = new Random();
        List<Card> result = new List<Card>();
        for (int i = 2; i <= 14; i++)
        {
            result.Add(new Card(i, Suit.Club));
            result.Add(new Card(i, Suit.Diamond));
            result.Add(new Card(i, Suit.Heart));
            result.Add(new Card(i, Suit.Spade));
        }
        DeckCards = result;
        SetCards();
        CreatePlayerHands(playerCount);
        PlayerCount = playerCount;
    }

    public void CreatePlayerHands(int count)
    {
        if (count < 1 || count > 10)
            throw new IndexOutOfRangeException();
        for (int i = 1; i <= count; i++)
        {
            SetCards(i);
        }
    }

    public void ShowCards()
    {
        Console.WriteLine("Flop:");
        foreach (Card c in DeckCards.Where(x => x.UsedByPlayerID == 0))
        {
            c.ConsoleOutput();
        }
        Console.WriteLine();
        for (int i = 1; i <= PlayerCount; i++)
        {
            HandResult result = GetHandResult(i);
            Console.Write("Player #" + i + "\t(" + result.result + " ");
            foreach (Card cResult in result.resultCards)
            {
                cResult.ConsoleOutput();
            }
            Console.Write(")");
            Console.WriteLine();
            foreach (Card c in DeckCards.Where(x => x.UsedByPlayerID == i))
            {
                c.ConsoleOutput();
            }
            Console.WriteLine();
        }
    }

    private IOrderedEnumerable<Card> GetPlayerSet(int playerID)
    {
        return DeckCards.Where(x => x.UsedByPlayerID == 0 || x.UsedByPlayerID == playerID).OrderByDescending(x => x.Face);
    }

    private void SetCards(int? playerID = null)
    {
        IEnumerable<Card> playerCards = DeckCards.Where(x => !x.UsedByPlayerID.HasValue).OrderBy(x => rnd.Next()).Take(playerID.HasValue ? 2 : 5);
        foreach (Card c in playerCards)
        {
            c.UsedByPlayerID = playerID.HasValue ? playerID : 0;
        }
    }

    public class HandResult
    {
        public HandResult(IEnumerable<Card> rc, Hand h)
        {
            result = h;
            resultCards = rc;
        }
        public Hand result { get; set; }
        public IEnumerable<Card> resultCards { get; set; }
    }

    public HandResult GetHandResult(int playerID)
    {
        IEnumerable<Card> set = GetPlayerSet(playerID).OrderBy(x => x.Face);
        return new[] { GetRoyalFlush(set), GetStraightFlush(set), GetFourOfAKind(set), GetFullHouse(set), GetFlush(set), GetStraight(set), GetThreeOfAKind(set), GetTwoPair(set), GetOnePair(set), GetHighCard(set) }.First(x => x != null);
    }

    private HandResult GetRoyalFlush(IEnumerable<Card> set)
    {
        HandResult straightflush = GetStraightFlush(set);
        if (straightflush != null)
        {
            return straightflush.resultCards.Any(x => x.Face == 14 && x.Face == 13) ? new HandResult(straightflush.resultCards, Hand.RoyalFlush) : null;
        }
        return null;
    }

    private HandResult GetStraightFlush(IEnumerable<Card> set)
    {
        IEnumerable<Card> flush = set.GroupBy(x => x.Suit).Where(x => x.Count() >= 5).SingleOrDefault() ?? Enumerable.Empty<Card>();
        HandResult strightflush =   GetStraight(flush);
        return strightflush != null ? new HandResult(strightflush.resultCards, Hand.StraightFlush) : null;
    }

    private HandResult GetFourOfAKind(IEnumerable<Card> set)
    {
        IEnumerable<Card> fourOfAKind = set.GroupBy(x => x.Face).SingleOrDefault(x => x.Count() >= 4) ?? Enumerable.Empty<Card>();
        return fourOfAKind.Any() ? new HandResult(fourOfAKind, Hand.FourOfAKind) : null;
    }

    private HandResult GetFullHouse(IEnumerable<Card> set)
    {
        HandResult threeOfAKind = GetThreeOfAKind(set);
        if (threeOfAKind != null)
        {
            IEnumerable<Card> remainingCards = set.Where(x => !threeOfAKind.resultCards.Any(y => y.Face == x.Face && y.Suit == x.Suit));
            IEnumerable<Card> highestTwoPair = remainingCards.GroupBy(x => x.Face).Where(x => x.Count() >= 2).OrderByDescending(x => x.Key).FirstOrDefault() ?? Enumerable.Empty<Card>(); ;
            if (highestTwoPair.Any())
            {
                return new HandResult(threeOfAKind.resultCards.Concat(highestTwoPair.Take(2)), Hand.ThreeOfAKind);
            }
        }
        return null;
    }

    private HandResult GetFlush(IEnumerable<Card> set)
    {
        IEnumerable<Card> flush = set.GroupBy(x => x.Suit).Where(x => x.Count() >= 5).SelectMany(x => x.OrderByDescending(y => y.Face).Take(5));
        return flush.Any() ? new HandResult(flush, Hand.Flush) : null;
    }

    private HandResult GetThreeOfAKind(IEnumerable<Card> set)
    {
        IEnumerable<Card> threeOfAKind = set.GroupBy(x => x.Face).OrderByDescending(x => x.Key).FirstOrDefault(x => x.Count() == 3);
        return threeOfAKind != null ? new HandResult(threeOfAKind, Hand.ThreeOfAKind) : null;
    }

    private HandResult GetTwoPair(IEnumerable<Card> set)
    {
        var pairs =  set.GroupBy(x => x.Face).Where(x => x.Count() == 2);
         return  pairs.Count() >= 2 ?  new  HandResult(pairs.OrderByDescending(x => x.Key).Take(2).SelectMany(x => x), Hand.TwoPair):null;
    }

    private HandResult GetOnePair(IEnumerable<Card> set)
    {
        IEnumerable<Card> onePair = set.GroupBy(x => x.Face).FirstOrDefault(x => x.Count() >= 2) ;
        return onePair != null ? new HandResult(onePair, Hand.OnePair) : null;
    }

    private HandResult GetStraight(IEnumerable<Card> set)
    {
        if (set.Any())
        {                
            bool isAceStraight = !new List<int>() { 14, 2, 3, 4, 5 }.Except(set.Select(x => x.Face)).Any();
            if (isAceStraight)
            {
                return  new HandResult( set.Where(x => new List<int>() { 14, 2, 3, 4, 5 }.Contains(x.Face)).GroupBy(x => x.Face).Select(x => x.First()), Hand.Straight);
            }
            int? temp = null;
            int conseductiveIndex = 0;
            for (int i = 0; i < set.Count(); i++)
            {
                if (temp.HasValue)
                {
                    if (temp != set.ElementAt(i).Face - 1)
                    {
                        conseductiveIndex = i;
                    }
                    if (i - conseductiveIndex == 4)
                    {
                        return new HandResult(  set.Skip(conseductiveIndex).Take(5), Hand.Straight);
                    }
                }
                temp = set.ElementAt(i).Face;
            }
        }
        return null;
    }

    private HandResult GetHighCard(IEnumerable<Card> set)
    {
        return new HandResult(set.OrderByDescending(x => x.Face).Take(5), Hand.HighCard);
    }

    public enum Hand { RoyalFlush, StraightFlush, FourOfAKind, FullHouse, Flush, Straight, ThreeOfAKind, TwoPair, OnePair, HighCard }
    public enum Suit { Heart = '\u2665', Diamond = '\u2666', Spade = '\u2660', Club = '\u2663' }

    public class Card
    {
        public Card(int face, Suit suit)
        {
            Suit = suit;
            Face = face;
        }
        public Suit Suit { get; set; }
        public int Face { get; set; }
        public int? UsedByPlayerID { get; set; }

        public void ConsoleOutput()
        {
            Console.OutputEncoding = System.Text.Encoding.UTF8;       
            Console.BackgroundColor = ConsoleColor.White;
            string face = GetFace(Face).PadLeft(2);
            Console.ForegroundColor = Suit == Deck.Suit.Diamond || Suit == Deck.Suit.Heart ? ConsoleColor.Red : ConsoleColor.Black;            
            Console.Write(face + (char)Suit);
            Console.ResetColor();
            Console.Write(" ");
        }

        private string GetFace(int face)
        {
             return face.ToString().Replace("11", "J").Replace("12", "Q").Replace("13", "K").Replace("14", "A");       
        }
    }
}

Answer 1

Performance

Your code tests against all possible hand result kinds. Is this useful or could you abort whenever you get a match? Is performance an actual requirement anway?

 public HandResult GetHandResult(int playerID)
    {
        IEnumerable<Card> set = GetPlayerSet(playerID).OrderBy(x => x.Face);
        return new[] { 
            GetRoyalFlush(set), GetStraightFlush(set), GetFourOfAKind(set),
            GetFullHouse(set), GetFlush(set), GetStraight(set), GetThreeOfAKind(set), 
            GetTwoPair(set), GetOnePair(set), GetHighCard(set) }
        .First(x => x != null);
    }

A faster alternative is to use the Null-collation.

 public HandResult GetHandResult(int playerID)
    {
        var set = GetPlayerSet(playerID).OrderBy(x => x.Face);
        return GetRoyalFlush(set) ?? GetStraightFlush(set) ?? ..
    }

---

Formatting

It is generally considered best practice use var whenever you can.

private HandResult GetFullHouse(IEnumerable<Card> set)
{
    HandResult threeOfAKind = GetThreeOfAKind(set);
    ..
}

private HandResult GetFullHouse(IEnumerable<Card> set)
{
    var threeOfAKind = GetThreeOfAKind(set);
    ..
}

---

Other

Not really worth elaborating upon, since I feel you made a little test case, and aren't using this code for a full API.

• Deck has too much responsibility. I would probably opt for more OO-design. Have classes as Player and Hand. Your Hand-enum could by renamed to HandKind.
• I would refactor out any Console.Write and have a seperate subdomain for rendering stuff.

Answer 2

Let's start with the basics: names and namespaces.

To me the following names have the following meanings:

• Deck: either the set of 52 cards (basically a wrapper around ISet<Card>), or, more likely, the cards which haven't yet been dealt (basically a wrapper around IList<Card>). In the latter case I'd expect it to have methods to deal from the top, and receive cards back and shuffle.
• Card: a Cartesian product of rank and suit. Could probably be a struct. In performance-oriented code, would be just an int.
• Hand: a wrapper around ISet<Card> representing the cards held by a single player
• HandResult: the result of scoring the hand. Probably implements IComparable<HandResult>.
• PokerGame: the top-level object which holds a Deck and the Hands of the players and contains the logic for evaluating a hand. Possibly an abstract class to allow subclasses for variants.

None of these logically belongs in the namespace of another.

Also, I find it misleading to use set for the name of a variable whose type isn't ISet<T>.

---

To expand on one of the points in the previous section, Card.UsedByPlayerID is a big red flag to me. The location of the card is a property of the game state, not a property of the card. The necessity to apply a filter in methods such as

    private IOrderedEnumerable<Card> GetPlayerSet(int playerID)
    {
        return DeckCards.Where(x => x.UsedByPlayerID == 0 || x.UsedByPlayerID == playerID).OrderByDescending(x => x.Face);
    }

should by itself have been a red flag that the design was wrong.

---

        for (int i = 2; i <= 14; i++)
        {
            result.Add(new Card(i, Suit.Club));
            result.Add(new Card(i, Suit.Diamond));
            result.Add(new Card(i, Suit.Heart));
            result.Add(new Card(i, Suit.Spade));
        }

Why 2 to 14? That needs a comment.

It would be cleaner to use a double loop and get the suits from Enum.GetValues(typeof(Suit)).

I'm puzzled by the asymmetry between Face being an int and Suit being an enum.

---

As should be obvious from my previous comments, I would eliminate SetCards in a major refactor, but for completeness:

            c.UsedByPlayerID = playerID.HasValue ? playerID : 0;

Prefer playerID.GetValueOrDefault().

---

    private HandResult GetRoyalFlush(IEnumerable<Card> set)
    {
        HandResult straightflush = GetStraightFlush(set);
        if (straightflush != null)
        {
            return straightflush.resultCards.Any(x => x.Face == 14 && x.Face == 13) ? new HandResult(straightflush.resultCards, Hand.RoyalFlush) : null;
        }
        return null;
    }

x => x.Face == 14 && x.Face == 13 could be optimised to x => false, so I think this is buggy.

---

In some senses the reuse of methods to find more specific results is attractive, although I'd rather have one method of twenty or so lines with a few nested ifs. That would, I think, allow more reuse of logic. For example, 4 of a kind, full house, 3 of a kind, two pairs, pair, and high card all boil down to: group cards by face, sort the groups by size descending then face descending, and check prefixes of that sorted result.

---

    private HandResult GetStraight(IEnumerable<Card> set)
    {
        if (set.Any())
        {                
            bool isAceStraight = !new List<int>() { 14, 2, 3, 4, 5 }.Except(set.Select(x => x.Face)).Any();
            if (isAceStraight)
            {
                return  new HandResult( set.Where(x => new List<int>() { 14, 2, 3, 4, 5 }.Contains(x.Face)).GroupBy(x => x.Face).Select(x => x.First()), Hand.Straight);
            }
            int? temp = null;
            int conseductiveIndex = 0;
            for (int i = 0; i < set.Count(); i++)
            {
                if (temp.HasValue)
                {
                    if (temp != set.ElementAt(i).Face - 1)
                    {
                        conseductiveIndex = i;
                    }
                    if (i - conseductiveIndex == 4)
                    {
                        return new HandResult(  set.Skip(conseductiveIndex).Take(5), Hand.Straight);
                    }
                }
                temp = set.ElementAt(i).Face;
            }
        }
        return null;
    }

For a start, that outer if would be unnecessary if you refactored GetStraightFlush to return null as soon as it found that a straight flush was impossible.

!new List<int>() { 14, 2, 3, 4, 5 }.Except(set.Select(x => x.Face)).Any();

is horrible to read. Inverting the logic to new List<int>() { 14, 2, 3, 4, 5 }.All(face => set.Any(x => x.Face == face)) makes it more comprehensible, although (a) the inefficiency of the test still grates, and (b) the constant list should be pulled out, probably as a readonly static IReadOnlyList<>.

Then the rest of the method is also rather cryptic. IMO it would benefit from a complete rewrite along the lines of

modifiers HandResult GetStraight(IEnumerable<Card> cards)
{
    Card[] representativePerFace = ...; // Place aces in position 0, 2s in position 1, ...
    // Highest straight is TJQKA, lowest is A2345
    for (int i = 9; i >= 0; i--)
    {
        if (Enumerable.Range(i, 5).All(x => representativePerFace[x % 13] != null))
        {
            return new ...
        }
    }
    return null;
}

Of course, that can then be optimised à la Knuth-Morris-Pratt.