Poker hand evaluator with score

If you don't play poker this will be hard to follow.

In Hold'em and other Poker games you take the best 5 of 7.

The class Hand selects the best 5 and scores the hand. Hand implements IComparable so hands may be compared. In simulations will create millions of boards to compare hands so speed is a priority.

There are 133784560 unique combinations of 7 cards from 52. When compare this to count of hand ranks from here it is correct.

I have run 4 different hand to hand match ups and gotten the same result as on line tools.

I chose to score the hand in the constructor as every hand needs to get scored. Report the best 5 cards in addition is a lot of overhead and not really required for a most simulations but used in a game play application then need the hand.

A direct hand to hand comparison and just hole cards hero, hole cards villain, and 5 cards on the board would be faster as you don't need to get into scoring the individual cards if the hand ranks are different.

```
public enum HandRank { high = 0, onePr = 1, twoPr = 2, trip = 3, straight = 4, flush = 5, boat = 6, quad = 7, strFlush = 8 }
public enum CardRank { two = 0, three = 1, four = 2, five = 3, six = 4, seven = 5, eight = 6, nine = 7, ten = 8, jack = 9, queen = 10, king = 11, ace = 12 }
public enum CardSuit { spade = 0, club = 1, heart = 2, diamond = 3 }
class Card : IComparable
{
public override bool Equals(Object obj)
{
// Check for null values and compare run-time types.
if (obj == null || GetType() != obj.GetType())
return false;

Card c = (Card)obj;
return (this.Quick == c.Quick);
}
public override int GetHashCode()
{
return this.Quick;
}
public int CompareTo(Object obj)
{
if (obj == null) return 1;

Card other = (Card)obj;
if (other != null)
return this.RankByte.CompareTo(other.RankByte);
else
throw new ArgumentException("Object is not a Card");
}

Implement IComparable and avoid the need for testing type:

public int CompareTo( Card otherCard ) {  };

If you do, MSDN docs say to also implement IEquatable. Have your Object.Equals override simply call this.

public bool Equals( Card otherCard ) {  }

class Hand : Object .. is superfluous. All classes derive from Object by definition.

Be kind to your readers and future you....

By convention class members are defined at the very top of a class followed by constructors.

Puts blank lines between methods.

IsRoyalFlush is more conventional than HaveRoyalFlush. A hand is a flush. A hand does not have a flush.

Don't be lazy with variable names: c. That is excusable if it was only a for loop index counter.

field and parameter names start with lower case by convention. Properties - upper case.

What is a Quick? Very confusing. Underlying types are different in Card and Hand. Card.Quick is magically the rank, suit, and hash code.

Magic numbers everywhere!

Classes are not cohesive. The number of Constructors and the nature of the parameters is a big clue.

public Hand (List cards, Int64 score, HandRank handRank)

A Card should contain/calculate it's own score and rank. The client Hand then just asks for it:

cards[2].Score;   cards[2].Rank;  // properties or methods as appropriate

You said I chose to score the hand in the constructor - then why pass it in? This constructor is depending on the client to provide valid and correct state. You can call one constructor from another. But if the hand evaluation code were not in that other constructor (and it should not be) then this constructor probably goes away altogether.

Then there's Card.Quick. This should not be injected by any outside client code. Use more fool proof constructor parameter, perhaps a CardRank. Then quick is calculated internally and should not be exposed otherwise. If Hand needs it there is GetHashCode - which is also more descriptive of what it is.

Write properties and methods so client code is basically forced, and limited, to doing the right thing. quick is perhaps the best example of the incoherent mix of exposed raw program implementation and poker domain objects and terminology. The same implementation detail is exposed in multiple contexts. This, along with those constructor parameters force the client to understand virtually the entire class' implementation; thus client code is left to figure out what, where, and when to use this mashup and the callee is fully trusting the caller to do it right.

Explicit output for debugging should not be built into the classes. Have the poker driver class do it. Implementing ToString in all classes will help. That along w/ the existing public properties should be enough.

Yeah we use the Debug class along w/ a compiler switch to enable it but that is not license to violate single responsibility.

public Hand(Card[] CardArray, int counter)

What is counter about? debugging? And why does this constructor not care about score and HandRank? How is it that two very different constructors both build valid objects?

Why pass a card array when Hand defines a List?

Use names that use poker terminology. Not CardArray, rather newHand for example.

The scoring/evaluation must be refactored out of this constructor. If you did I suppose you would not be passing score in that other constructor. My initial cut (a little DSL there) would be separate methods for each score type.

Throwing an exception in CompareTo doesn't seem right to me, but not wrong per se. I don't see a practical difference in passing null or casting resulting in null.

P.S.

Oh, I see why you use a variable c. A subliminal reference to some other OO language.

for (int c = 0; c <= 12; c++)

StackExchange Code Review Q#159537, answer score: 5