10
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ This is borderline not working code \$\endgroup\$ – paparazzo Jan 17 '17 at 14:41
  • 5
    \$\begingroup\$ there are lots of "poker hand" questions in CodeReview. I'm sure there are insights there waiting for you to discover them. \$\endgroup\$ – radarbob Jan 17 '17 at 14:43
  • 1
    \$\begingroup\$ Yes, there once was a community challenge to implement a poker hand evaluator, that turned out to be quite popular. You should check community-challenge tag out. \$\endgroup\$ – Nikita B Jan 17 '17 at 19:41
  • \$\begingroup\$ @radarbob Yes I noticed that after I started part one. But as it felt a little bit like cheating when i was doing it to test how i could solve problems myself, thanks though \$\endgroup\$ – Steve Jan 18 '17 at 10:00
10
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for the help! I've implemented that into my code now. \$\endgroup\$ – Steve Jan 17 '17 at 14:17
7
\$\begingroup\$
  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.

  1. 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.

\$\endgroup\$
  • \$\begingroup\$ ah completely forgot about string builder knew there was a better way to do it! Thanks \$\endgroup\$ – Steve Jan 17 '17 at 13:44
  • \$\begingroup\$ 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 \$\endgroup\$ – Malachi Jan 17 '17 at 14:18
  • 3
    \$\begingroup\$ Or even shorter: public override ToString() => string.Join(" ", Cards.Select(c => $"({c.Face} of {c.Suit}'s)")); ;) \$\endgroup\$ – JanDotNet Jan 17 '17 at 14:51
  • \$\begingroup\$ @a-man Given that you control the Hand object and can modify it, why would you create an extension rather than just create a new method in the Hand object? \$\endgroup\$ – 404 Jan 17 '17 at 17:48
  • \$\begingroup\$ how do achieve this kind of behavior with normal methods? I prefer extensions just because of clear syntax. \$\endgroup\$ – a-man Jan 17 '17 at 22:44
6
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ Thanks again for the massive answer and putting the time to write this. I knew my compare to could be improved(never used Icomparable before) but i didn't think that much so I'm very impressed! Also I will have to do a lot more reading up on linq and lambda expressions. \$\endgroup\$ – Steve Jan 18 '17 at 9:36
  • \$\begingroup\$ also I've moved around my check hank rank into the getter so it should always check the current rank of the cards when ever it is used (therefore i removed the set) which should fix the bug you mentioned right? out of interest is there any improvement to be made with the straightcheck method, it seems to be the only one nobody touches but barely uses linq and its quite bulky. \$\endgroup\$ – Steve Jan 18 '17 at 9:46
  • 1
    \$\begingroup\$ @Steve Yes, the bug wont be present if you have your logic in the getter, but that's not a good practice, you're better of with a method that does that for you, also your SortHandbyFace implementation is bad because it creates a new instance of Hand, thus breaking the reference type link between any previously linked variables. But because you are creating new object, imagine you have 10-15 properties in your class, they will all loose value and you will have you assign them again. That's why you should go with implementation that doesn't creates a new instance, mine is a good example. \$\endgroup\$ – Denis Jan 18 '17 at 14:59
  • 1
    \$\begingroup\$ About your straightcheck, you don't want LINQ there, it will become overly complicated. LINQ works with foreach not with for, what does the for has that foreach doesn't? For is meant to index a collection, so you can play around with different indexes, in foreach you can't do that, there is exactly one element Current that you can access at a time, without some other relation. In your for loop you are checking the Current and the Next index, this will be really inefficient in a foreach loop and thus in LINQ expression. It's short and clean, keep it like this. \$\endgroup\$ – Denis Jan 18 '17 at 15:02
  • \$\begingroup\$ great thanks again that really helped my understanding. never though creating a new instance would cause that much issue but now i do \$\endgroup\$ – Steve Jan 19 '17 at 9:45
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ thanks for the suggestion, I'm defiantly going to give it a go to practice interface usage. looks difficult but very useful! \$\endgroup\$ – Steve Jan 17 '17 at 14:07
  • \$\begingroup\$ This is highly bloated and slow. Pair, two pair, trips, quads, and high card can be done in one pass. If is missing a lot. High card alone is not enough determine winning hand. \$\endgroup\$ – paparazzo Jan 17 '17 at 16:05
  • 3
    \$\begingroup\$ @Paparazzi, slow in what context? If you evaluate 10000000 hands per second? Then yes, it is slower. If you evaluate once per match? I'd say that the speed difference is irrelevant, compared to what you gain by having a more structured OOP code, that is way, way easier to unit test and debug, compared to what OP currently have (and he mentioned that he is having troubles getting hands evaluated correctly). In my opinion, testability and readability should come first, then if performance problems are actually encountered, the code can be optimized. \$\endgroup\$ – Nikita B Jan 17 '17 at 16:18
  • \$\begingroup\$ How about the context where you run all 2.9 million hands to see if you get the corrects answer. Then when you step up for best 5 out of 7 cards (actual holdem poker) and need to run 134 million hands (and the hand rank takes longer). My total code to rank is 52 lines of code. Calculate you equity versus you opponents range. \$\endgroup\$ – paparazzo Jan 17 '17 at 20:44
-1
\$\begingroup\$

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()
    {
    }
}
\$\endgroup\$
  • \$\begingroup\$ Down vote care to share the problem? \$\endgroup\$ – paparazzo Jan 17 '17 at 15:11
  • 1
    \$\begingroup\$ I disagree with a notion that hand must be "self-contained". Hand is a collection of cards, it does not have to know the rules of the game being played. And it should not, imho: Hand looks like an abstraction that can be reused in all sorts of other traditional card games and it makes no sense to make it game-specific. I also dislike the idea of hiding complex computation behind property getter. P.S. Oh, sorry, I should have casted my vote after giving an explanation. \$\endgroup\$ – Nikita B Jan 17 '17 at 15:30
  • \$\begingroup\$ (it wasn't me who down voted you) I did see your comment last time but i didn't quite understand the self contained part(i did all the other changes you suggested) but after a bit of thinking i think i understand what (and more importantly why) you meant. my code is too big to put here but I've put the checkhand logic inside get and removed the set. Is that much better? \$\endgroup\$ – Steve Jan 17 '17 at 15:34
  • \$\begingroup\$ @NikitaB OK did not ask to argue. HandRank is only associated with a Hand. Hand needs HandRank to implement IComparable which is critical to actually playing a game of cards. This is a poker classifier. I have written a poker game and poker simulator. \$\endgroup\$ – paparazzo Jan 17 '17 at 15:46
  • \$\begingroup\$ @Steve Your HandRank code is bloated. You should be able to do that in like 1/2 the number of lines you have now. Don't do the abstraction suggested by Nikita as too much code and too slow. \$\endgroup\$ – paparazzo Jan 17 '17 at 16:07

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.