0
\$\begingroup\$

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");       
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ You should probably put each chained (Replaces and "LINQ"s) method and the each item of the array declaration in GetHandResult on a new line to improve readability. You can still edit this before an answer is posted. \$\endgroup\$ – Xiaoy312 May 23 at 18:36
  • 1
    \$\begingroup\$ Don't you want to compare evaluated hands against eachother? \$\endgroup\$ – dfhwze May 23 at 18:52
  • \$\begingroup\$ You aren't using Random correctly. I would suggest reading through all 4 parts , ericlippert.com/2019/01/31/fixing-random-part-1 \$\endgroup\$ – Brad M May 23 at 19:45
  • \$\begingroup\$ @BradM could you write that refactored random part into a answer? \$\endgroup\$ – fubo May 24 at 10:31
4
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ thank you, the null-coalescing operator is a great approach \$\endgroup\$ – fubo May 24 at 10:30
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ thank you for this, I'll take this into my code, the ace in position 14 makes it easier to compare in the next step so 14 > 13 evaluates the winning hand more natural. First i thought of a enum enum face { two = 2, three = 3 ... ace = 14} \$\endgroup\$ – fubo May 24 at 10:29

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.