current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
up vote 4 down vote favorite

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!

share|improve this question
up vote 5 down vote 
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.

share|improve this answer
    
Thanks a lot for the help! I've implemented that into my code now. – Steve 6 mins ago
up vote 3 down vote

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();
}
share|improve this answer
    
ah completely forgot about string builder knew there was a better way to do it! Thanks – Steve 39 mins ago
    
This is a very good answer, would you please explain the review a little more in your Answer so that people new to C# and Linq might be able to learn from your answer as well? Thank you – Malachi 5 mins ago
up vote 2 down vote

I think, you should replace static methods with some sort of abstraction, that would encapsulate evaluation logic for every combination. For example:

interface IPokerRule
{
    HandValue Evaluate(Hand hand);
}

where

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

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

    //you compare the value of hands, not the hands themselves
    public int CompareTo(HandValue 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(Hand hand)
    {   
        return new HandValue(hand.Cards.Max(), PokerHandsRank.HighCard);
    }
}

class SinglePairRule : IPokerRule
{
    public HandValue Evaluate(Hand hand)
    {  
        var pair = hand.Cards.GroupBy(card => card.Face)
                             .Where(g => g.Count == 2)
                             .SingleOrDefault();

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

//etc.

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

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 handValue = _ruleSet.Select(rule => rule.Evaluate(hand))
                        .Where(v => v != null)
                        .Max();

I am only slightly familiar with poker rules, so I am sorry, if I am missing some important concept.

share|improve this answer
    
thanks for the suggestion, I'm defiantly going to give it a go to practice interface usage. looks difficult but very useful! – Steve 16 mins ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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