Beginnings of a Poker hand classifier part 2

Question

Part 1 - Beginnings of a Poker hand classifier

I'm a beginner programmer I've been working on a poker hand classifier to improve my object orientation and programming skills, I've improved and expanded my class from last time and wanted to share and see if what I've done could be improved.

Card Class, Hand Class, enums for suit, handrank and face

  public class Hand : IComparable<Hand>
{
    public List<Card> Cards { get; }
    public PokerHandsRank HandRank { get; set; }

    public Hand(List<Card> cards)
    {
        if (cards.Count() == 5)
        {
            Cards = cards;
            HandRank = PokerHandsRank.HighCard;                    
        }
        else
        {
            throw new Exception("Invalid amount of Cards");
        }  
    }
    public override string ToString()
    {
        return string.Format(
              "(" + Cards[0].Face + " of " + Cards[0].Suit + "s) "
            + "(" + Cards[1].Face + " of " + Cards[1].Suit + "s) "
            + "(" + Cards[2].Face + " of " + Cards[2].Suit + "s) "
            + "(" + Cards[3].Face + " of " + Cards[3].Suit + "s) "
            + "(" + Cards[4].Face + " of " + Cards[4].Suit + "s) "
            );
    }
    public int CompareTo(Hand other)
    {

        if (HandRank == other.HandRank) //if the hand rank is equal, sort the cards by face value and compare the two biggest
        {
            Hand sortThisHand = Program.sortHandbyFace(this);
            Hand sortOtherHand = Program.sortHandbyFace(other);
            if (sortThisHand.Cards[4].Face > sortOtherHand.Cards[4].Face)                
                return 1;                
            else if (sortThisHand.Cards[4].Face < sortOtherHand.Cards[4].Face)                
                return -1;                
            else                
                return 0;                
        }
        else if (HandRank > other.HandRank)            
            return 1;            
        else if (HandRank < other.HandRank)            
            return -1;            
        else
        throw new Exception("Hand rank is not initiated");

    }


}
public class Card
{
    public Face Face { get; }
    public Suit Suit { get; }        
    public Card(Suit suit, Face face)
    {
        Face = face;
        Suit = suit;
    }        
    public override string ToString()
    {
        string card = "(" + Face + " of " + Suit + "s) ";
        return card;
    }
}
public enum Face
{
    Two,
    Three,
    Four,
    Five,
    Six,
    Seven,
    Eight,
    Nine,
    Ten,
    Jack,
    Queen,
    King,
    Ace
}
public enum Suit
{
    Club, Diamond, Heart, Spade
}
public enum PokerHandsRank
{
    HighCard,
    Pair,
    TwoPair,
    ThreeOfKind,
    Straight,
    Flush,
    FullHouse,
    FourOfKind,
    StraightFlush,
    RoyalFlush
}

note: I had a lot trouble with initializing the handrank by using the checkHandRank Method(stack overflow exception) so I had to leave it at the minimum enum of highcard and run the checkhand method in main program class but its definitely not right in my opinion.

CheckHandRank and sortHandByFace Methods

         public static PokerHandsRank CheckHandRank(Hand hand)
    {
        PokerHandsRank flushCheck = CheckHandForFlush(hand);
        PokerHandsRank pairCheck = CheckHandForPairs(hand);
        PokerHandsRank straightCheck = CheckHandForStraight(hand);

        if (flushCheck == PokerHandsRank.Flush && straightCheck == PokerHandsRank.Straight)
        {                
            Hand sortedHand = sortHandbyFace(hand);
            return sortedHand.Cards[4].Face == Face.Ace && sortedHand.Cards[0].Face != Face.Two ? PokerHandsRank.RoyalFlush : PokerHandsRank.StraightFlush;
        }
        if (pairCheck > flushCheck) return pairCheck; // check if pair rank is greater than flush rank (four of a kind or fullhouse) //returns either four of a kind or fullhouse         
        if (flushCheck == PokerHandsRank.Flush)  return flushCheck; //return flush           
        return straightCheck == PokerHandsRank.Straight ? straightCheck : pairCheck; //returns straight or pair value (three of a kind, two pair, pair or highcard)
    }
    public static Hand sortHandbyFace(Hand hand)
    {
        var sortCards = (from cards in hand.Cards
                         orderby cards.Face
                         select cards);

        List<Card> orderedCards = sortCards.ToList();
        Hand sortedHand = new Hand(orderedCards);
        return sortedHand;
    }

check for flush/straights/pairs

  private static PokerHandsRank CheckHandForFlush(Hand hand)

    {
        var suitCount = (from card in hand.Cards
                         group card by card.Suit into g
                         let count = g.Count()
                         orderby count descending
                         select count).Take(1).ToList();

        flushName = hand.Cards[0].Suit;

        return suitCount[0] == 5 ? PokerHandsRank.Flush : PokerHandsRank.HighCard;          
    }
    private static PokerHandsRank CheckHandForStraight(Hand hand)
    {
        int cardsInARowCount = 0;
        Hand orderedHand = sortHandbyFace(hand);

        if (orderedHand.Cards[4].Face == Face.Ace && orderedHand.Cards[0].Face == Face.Two &&
            orderedHand.Cards[1].Face == Face.Three && orderedHand.Cards[2].Face == Face.Four &&
            orderedHand.Cards[3].Face == Face.Five) // check if ace, two, three, four, five straight
        {               
            return PokerHandsRank.Straight;
        }

        for (int i = 0; i < orderedHand.Cards.Count - 1; i++) // check if the next card's face in the hand is the same as the next in the enum face order
        {
            if (orderedHand.Cards[i].Face + 1 == orderedHand.Cards[i + 1].Face)
            {
                cardsInARowCount++;
            }
        }
        return cardsInARowCount == 4 ? PokerHandsRank.Straight : PokerHandsRank.HighCard;
    }
    private static PokerHandsRank CheckHandForPairs(Hand hand)

    { 
        var faceCount = (from card in hand.Cards
                         group card by card.Face
                    into g
                         let count = g.Count()
                         orderby count descending
                         select count).Take(2).ToList(); // take two to check if multiple pairs of pairs, if second in list is 1 there will be two pairs

        switch (faceCount[0])
        {
            case 1: return PokerHandsRank.HighCard;
            case 2: return faceCount[1] == 1 ? PokerHandsRank.Pair : PokerHandsRank.TwoPair;                  
            case 3: return faceCount[1] == 1 ? PokerHandsRank.ThreeOfKind : PokerHandsRank.FullHouse;
            case 4: return PokerHandsRank.FourOfKind;
            default: throw new Exception("something went wrong here");
        }
    }  

If i wanted to get the name value of the pairs would that be possible? I tried using linq but by the time they are sorted into count they lose the card value therefore the face property.

I also have a few deck methods which fill/deal/removeatx/swapcard but they are basic and not particularly relevant but let me know if i should post them.

I plan to continue this program a bit more but this as a good point to review. I've spent 30-45mins testing and random decks seem to be read, written, sorted and compared with other hands well enough. Please let me know how it could be improved or if there are any errors thanks!

Answer 1

public Hand(List<Card> cards)
{
    if (cards.Count() == 5)
    {
        Cards = cards;
        HandRank = PokerHandsRank.HighCard;                    
    }
    else
    {
        throw new Exception("Invalid amount of Cards");
    }  
}

Don't use the Count() method if you are working with an object which implements ICollection<T> because ICollection<T> contains the Count property which will be faster than the Count() method. The Count() method is using a softcast to ICollection<T> via as and a null check which if the resulted object isn't null just accesses the Count property.

By reversing the if condition you can just throw an exception, but you should throw an exception which is as specific as possible. This makes tracking bugs much easier. Either use an ArgumentOutOfRangeException or create a own one.

public Hand(List<Card> cards)
{
    if (cards.Count != 5)
    {
        throw new ArgumentOutOfRangeException("Invalid amount of Cards");
    }

    Cards = cards;
    HandRank = PokerHandsRank.HighCard;   
}  

But do you really want that the Cards could be changed from outside of the object ? Right now you are just assigning the passed List<Card to the Cards property. If you don't want that this could be changed from outside, you should use the overloaded constructor of the List<T> which takes an IEnumerable<T> as an argument like so

public Hand(List<Card> cards)
{
    if (cards.Count != 5)
    {
        throw new ArgumentOutOfRangeException("Invalid amount of Cards");
    }

    Cards = new List<Card>(cards);
    HandRank = PokerHandsRank.HighCard;   
}  

But maybe a ReadOnlyCollection<T> would be better for your property.

Answer 2

1. you can change your ToString method to something like this

     public override string ToString()
     {
      var message = new StringBuilder();
      Cards.ForEach(c => message.Append($"({c.Face} of {c.Suit}'s) "));
      return message.ToString();
     }
   

2. you could simplify your code and make it more readable with extension methods. you already have static methods, just add this before parameter and you can call them in more clean way

   public static Hand SortByFace(this Hand hand)
   {
       return new Hand(hand.Cards.OrderBy(c => c.Face).ToList());
   }
   

and

    Hand orderedHand = sortHandbyFace(hand);

can be written like .

    Hand orderedHand = hand.SortByFace();

but it is matter of personal preference.

3. In general there are very good best practices and design guidelines. Consider following:
   • SOLID
   • DRY
   • KISS
   • YAGNI

Except that you are using lots of if statements, which is actually point to consider some pattern(e.g. Chain of responsibility, Strategy).

replace switch replace if

I am not that much experienced developer,but these are things I always try to follow. Hope this helps.

Answer 3

You already got some pretty good answer, but I'd like to give my 2 cents.

Query expression vs lambda syntax

Query expression syntax is most of the time longer than lambda expression syntax, there is no difference in the performance, so you can just pick the shorter version:

var sortCards = from cards in hand.Cards
    orderby cards.Face
    select cards;

Can become:

var sortCards = hand.Cards.OrderBy(cards => cards.Face);

This one is rather short but you have 2 longer ones:

var suitCount = (from card in hand.Cards
    group card by card.Suit
    into g
    let count = g.Count()
    orderby count descending
    select count).Take(1).ToList();

var faceCount = (from card in hand.Cards
    group card by card.Face
    into g
    let count = g.Count()
    orderby count descending
    select count).Take(2).ToList();

With lambda syntax:

var suitCount = hand.Cards.GroupBy(card => card.Suit)
    .Select(g => new {g, count = g.Count()})
    .OrderByDescending(t => t.count)
    .Select(t => t.count).Take(1).ToList();

var faceCount = hand.Cards.GroupBy(card => card.Face)
    .Select(g => new {g, count = g.Count()})
    .OrderByDescending(t => t.count)
    .Select(t => t.count).Take(2).ToList();

But you have some repetitive logic in here, you can declare a method that extracts it like this:

private static List<int> FilterCards<TValue>(Hand hand, Func<Card, TValue> filter, int count)
{
    return hand.Cards.GroupBy(filter)
        .Select(g => new {g, count = g.Count()})
        .OrderByDescending(t => t.count)
        .Select(t => t.count).Take(count).ToList();
}

And use it like this:

var suitCount = FilterCards(hand, card => card.Suit, 1);
var faceCount = FilterCards(hand, card => card.Face, 2);

Improving public int CompareTo(Hand other)

You are doing too much unnecessary work in your Hand CompareTo method.

This:

if (HandRank == other.HandRank)
{
    Hand sortThisHand = Program.SortHandbyFace(this);
    Hand sortOtherHand = Program.SortHandbyFace(other);
    if (sortThisHand.Cards[4].Face > sortOtherHand.Cards[4].Face)
        return 1;
    if (sortThisHand.Cards[4].Face < sortOtherHand.Cards[4].Face)
        return -1;
    return 0;
}

Can be written in a single line:

if (HandRank == other.HandRank)
{
    return Cards.Max(c => c.Face).CompareTo(other.Cards.Max(c => c.Face));
}

Your next check can also be written in one line:

if (HandRank > other.HandRank)
    return 1;
if (HandRank < other.HandRank)
    return -1;

Like this:

return HandRank.CompareTo(other.HandRank);

Also there is no place for exceptions here, your method will never throw an exception, so the throw() is redundant.

Having that in mind your whole method can be converted to a virtually one liner:

public int CompareTo(Hand other)
{
    return HandRank == other.HandRank
        ? Cards.Max(c => c.Face).CompareTo(other.Cards.Max(c => c.Face))
        : HandRank.CompareTo(other.HandRank);
}

Bug when sorting hands by face

Your current implementation of SortHandbyFace has a bug in it:

hand.HandRank = PokerHandsRank.RoyalFlush;
Hand sortedHand = SortHandbyFace(hand);

What will be the HandRank of sortedHand? The answer is HighCard. Why? Because you are creating a new instance of the card, providing it with some ordered cards but never assign the actual rank back. Your method is also doing too much work to complete the task:

public static Hand SortHandbyFace(Hand hand)
{
    var sortCards = hand.Cards.OrderBy(cards => cards.Face);

    List<Card> orderedCards = sortCards.ToList();
    Hand sortedHand = new Hand(orderedCards);
    return sortedHand;
}

It can all be written on a single line:

public static void SortHandbyFace(Hand hand)
{
    hand.Cards.Sort((card, card1) => card.Face.CompareTo(card1.Face));
}

Example usage:

hand.HandRank = PokerHandsRank.RoyalFlush;
SortHandbyFace(hand);

Answer 4

In "part one" Eric said

LINQ make the code read more like its meaning rather than a bunch of loops and counters that seem completely disconnected from the domain at hand. What are you doing logically? Grouping the cards by face value and counting how many cards are in each group. So make the code actually do that.

Remember, your goal here is to correctly implement the rules. The rules do not say "consider all ordered pairs of cards; assign one point for each identical card in that set of pairs. If the point total is 12 then..." and so on, but that's what you wrote. Which is more likely to be correct, code that resembles the logic of the rules, or code that does some completely other thing that you hope is equivalent to the rules? Which do you suppose will be easier to maintain?

Which in my opinion is a great advice. I think you should go ahead, and actually replace static methods with some sort of abstractions, that would encapsulate evaluation logic for every combination. Basically you need to create a set of poker rules using C# code. For example:

interface IPokerRule
{
    Hand Evaluate(List<Card> cards);
}

where

class Hand : IComparable<Hand>
{
    public Hand(Card highCard, PokerHandsRank rank)
    {
        HighCard = highCard;
        Rank = rank;
    }

    public Card HighCard { get; private set; }
    public PokerHandsRank Rank { get; private set; }

    public int CompareTo(Hand other)
    {
        //you can use Enum.CompareTo instead of '<' and '>'
        var result = Rank.CompareTo(other.Rank);
        if (result == 0)
        {
            //you need to implement IComparable on Card 
             result = HighCard.CompareTo(other.HighCard) 
        }
        return result;
    }
}

Then you can define individual rules:

class HighCardRule : IPokerRule
{
    public HandValue Evaluate(List<Card> cards)
    {   
        return new Hand(cards.Max(), PokerHandsRank.HighCard);
    }
}

class SinglePairRule : IPokerRule
{
    public Hand Evaluate(List<Card> cards)
    {  
        var pair = cards.GroupBy(card => card.Face)
                        .Where(g => g.Count == 2)
                        .SingleOrDefault();

        if (pair == null) return null;
        var highCard = pair.Max();
        return new Hand(highCard, PokerHandsRank.Pair);
    }
}

//etc.

And in order to evaluate a hand you can loop over a collection of rules, and pick the highest combination:

private IPokerRule[] _ruleSet = new IPokerRule[]
                     {
                         new HighCardRule(),
                         new SinglePairRule(),
                         //etc...
                     }

//or you can probably reverse the collection and pick first non-null result
//(depending on how well I understand the rules)
var hand = _ruleSet.Select(rule => rule.Evaluate(cards))
                   .Where(v => v != null)
                   .Max();

I am only slightly familiar with poker rules, so I am sorry, if I am missing some important concept. Feel free, to add more info to Hand class, if HighCard is not enough to evaluate hands that have the same rank. And feel free to modify the "rules" if I got them wrong. But hopefully you got the idea. I think you should give it a try and see for yourself whether the extra code is worth it or not.

Answer 5

As I tried to tell you on first question make Hand more self contained
I ran it as I wanted to see how much slower LINQ is
Ran all possible hands (2.9 million) in 45 second and got the correct answer
The way I do it with arrays is 9x faster and not much more code

public enum Faces { Ace, King, Queen, Jack, Ten, Nine, Eight, Seven, Six, Five, Four, Three, Two };
public enum Suits { Spade, Club, Heart, Diamond };
public enum Ranks { StrFlush, Quads, Boat, Flush, Straight, Trips, TwoPair, OnePair, HighCard };
public class Card : Object  // need to implement Equals
{
    public Faces Face { get; set; }
    public Suits Suit { get; set; }
    public override string ToString()
    {
        return $"({Face} {Suit})";
    }
    public Card (byte b)
    {
        Face = (Faces)(b % 13);
        Suit = (Suits)(b / 13);
    }
}
public static void HandTest()
{
    Stopwatch sw = new Stopwatch();
    sw.Start();
    Dictionary<Ranks, int> handCount = new Dictionary<Ranks, int>();
    Card cardi;
    Card cardj;
    Card cardk;
    Card cardm;
    List<Card> cards;
    Hand hand;

    //test quads - it was broken
    //cards = new List<Card>() { new Card(0), new Card(13), new Card(26), new Card(39), new Card(2) };
    //hand = new Hand(cards);
    //Debug.WriteLine(hand.RankFAST);

    for (byte i = 0; i < 48; i++)
    {
        cardi = new Card(i);
        for (byte j = (byte)(i + 1); j < 49; j++)
        {
            cardj = new Card(j);
            for (byte k = (byte)(j + 1); k < 50; k++)
            {
                cardk = new Card(k);
                for (byte m = (byte)(k + 1); m < 51; m++)
                {
                    cardm = new Card(m);
                    for (byte n = (byte)(m + 1); n < 52; n++)
                    {
                        cards = new List<Card>() { cardi, cardj, cardk, cardm, new Card(n) };
                        hand = new Hand(cards);
                        if (handCount.ContainsKey((Ranks)hand.RankLINQ))
                            handCount[(Ranks)hand.RankLINQ]++;
                        else
                            handCount.Add((Ranks)hand.RankLINQ, 1);
                    }
                }
            }
        }
    }
    sw.Stop();
    foreach (KeyValuePair<Ranks, int> kvp in handCount.OrderBy(x => x.Value))
        Debug.WriteLine("{0} {1}", kvp.Key, kvp.Value.ToString("N0"));
    int count = handCount.Sum(x => x.Value);
    Debug.WriteLine("total {0}   sw (sec) {1}   per ms {2}", count.ToString("N0"), (sw.ElapsedMilliseconds/1000).ToString("N0"), (count/sw.ElapsedMilliseconds).ToString("N0"));
    Debug.WriteLine("");
}
public class Hand : IComparable
{
    public List<Card> Cards { get; private set; } = new List<Card>();
    public Ranks? RankLINQ
    {
        get
        {
            Ranks rank = Ranks.HighCard;
            if (Cards.Count != 5)
            {
                return null;
            }
            var cardCounts = (from  card in Cards                // We're processing each card
                                group card by card.Face          // make a sequence of groups of cards with the same face
                                into g                           // process each group 
                                let count = g.Count()            // get the size of each group
                                orderby count descending         // sort the groups by size, big to small
                                select count).Take(2).ToList();  // get the top two sizes
            switch (cardCounts[0])
            {
                case 1:
                    rank = Ranks.HighCard;
                    break;
                case 2:
                    rank = cardCounts[1] == 1 ? Ranks.OnePair : Ranks.TwoPair;
                    break;
                case 3:
                    rank = cardCounts[1] == 1 ? Ranks.Trips : Ranks.Boat;
                    break;
                case 4:
                    rank = Ranks.Quads;
                    break;
            }
            if (rank == Ranks.Boat || rank == Ranks.Quads)
                return rank;  //flush or striaght not possible
            if (rank == Ranks.HighCard)
            {
                List<Card> cardSorted = Cards.OrderBy(x => x.Face).ToList();              
                bool haveStraight = (cardSorted[4].Face - cardSorted[0].Face == 4);
                if (!haveStraight) haveStraight = (cardSorted[0].Face == Faces.Ace && cardSorted[1].Face == Faces.Five);
                var flushCount = (from card in Cards               // We're processing each card
                                    group card by card.Suit          // make a sequence of groups of cards with the same face
                                    into g                           // process each group 
                                    let count = g.Count()            // get the size of each group
                                    orderby count descending         // sort the groups by size, big to small
                                    select count).FirstOrDefault();
                if (flushCount == 5 && haveStraight)
                    return Ranks.StrFlush;
                if (flushCount == 5)
                    return Ranks.Flush;
                if (haveStraight)
                    return Ranks.Straight;
            }
            return rank;                   
        }
    }
    public int CompareTo(object obj)
    {
        if (RankLINQ == null)
        {
            throw new ArgumentNullException();
        }
        if (obj == null)
        {
            throw new ArgumentNullException();
        }
        if (obj is Hand)
        {
            Hand otherHand = (Hand)obj;
            if (otherHand.RankLINQ == null)
            {
                throw new ArgumentNullException();
            }
            return ((int)this.RankLINQ).CompareTo((int)otherHand.RankLINQ);
            // this is really more complex as Kings Up beats Tens Up  
            // how I do this is an intger for stength
        }
        else
        {
            throw new ArgumentNullException();
        }
    }
    public override string ToString()
    {
        StringBuilder sb = new StringBuilder();
        foreach (Card c in Cards)
            sb.Append(c.ToString() + " ");
        return sb.ToString();
    }
    public Hand(List<Card> cards)
    {
        Cards = cards; //.OrderBy(x => x.Face).ThenBy(x => x.Suit).ToList();
    }
    public Hand()
    {
    }
}