# Weekend Challenge: Ruby Poker Hand Evaluation

I'm late to this weekend challenge (sorry), but since it's all in good fun, I hope that's ok. I'm no poker player, though, so I may have completely overlooked something.

The Hand class does the evaluation, and calculates (among other things) a score array which is ordered for possible comparison with another poker hand. First item is the hand's score (0-8), and the following 1-5 items are the tie-breaking values/kickers. E.g. a three-of-a-kind hand might be scored as

[3, 5, 9, 6] # base score, value of tripled card, kicker, kicker


Or, for comparison purposes, consider two two-pair hands

player1.score #=> [2, 7, 5, 3]
player2.score #=> [2, 7, 5, 8]


Both players have two pairs of 7s and 5s, but player 2 wins by having the higher kicker.

The (known and intentional) limitations are:

• 5 cards per hand only (i.e. no communal cards, etc.)
• No support for jokers/wildcards
• No validation of the cards

It does take into account high and low aces when determining straights, but otherwise it's not terribly flexible. (Of course, you can sidestep the "5 cards only" limitation by just brute-force checking 5-card combinations one at a time using, say, Array#combination, but that's another story.)

I haven't looked at how this challenge has been solved in other languages, so perhaps there are some tricks I'm missing. But really, the point was mostly to see how far I could get with a fairly functional approach and array/enumerable methods. The code's mostly one-line methods, so it went OK, I think.

Haven't bothered with optimization yet, but (if nothing else) a bunch of values can be memoized with a smattering of ||=. However, I'm more interested in critiques of the overall approach (I just like ? methods, ok?!) and possible alternatives (either overall or for specific parts)

The full code (including tests and more verbose comments) is in this gist; below are the principal classes (see further notes below)

ACE_LOW  = 1
ACE_HIGH = 14

# Use Struct to model a simple Card class
Card = Struct.new :suit, :value

# This class models and evaluates a hand of cards
class Hand

RANKS = {
straight_flush:  8,
four_of_a_kind:  7,
full_house:      6,
flush:           5,
straight:        4,
three_of_a_kind: 3,
two_pair:        2,
pair:            1
}.freeze

def initialize(cards)
raise ArgumentError unless cards.count == 5
@cards = cards.freeze
end

# The hand's rank as an array containing the hand's
# type and that type's base score
def rank
RANKS.detect { |method, rank| send :"#{method}?" } || [:high_card, 0]
end

# The hand's type (e.g. :flush or :pair)
def type
rank.first
end

# The hand's base score (based on rank)
def base_score
rank.last
end

# The hand's score is an array starting with the
# base score, followed by the kickers.
def score
[base_score] + kickers
end

# Tie-breaking kickers, ordered high to low.
def kickers
repeat_values + (aces_low? ? aces_low_values.reverse : single_values)
end

# If the hand's straight and flush, it's a straight flush
def straight_flush?
straight? && flush?
end

# Is a value repeated 4 times?
def four_of_a_kind?
repeat_counts.include? 4
end

# Three of a kind and a pair make a full house
def full_house?
three_of_a_kind? && pair?
end

# If the hand only contains one suit, it's flush
def flush?
suits.uniq.count == 1
end

# This is the only hand where high vs low aces comes into play.
def straight?
aces_high_straight? || aces_low_straight?
end

# Is a card value repeated 3 times?
def three_of_a_kind?
repeat_counts.include? 3
end

# Are there 2 instances of repeated card values?
def two_pair?
repeat_counts.count(2) == 2
end

# Any repeating card value?
def pair?
repeat_counts.include? 2
end

# Actually just an alias for aces_low_straight?
def aces_low?
aces_low_straight?
end

# Does the hand include one or more aces?
def aces?
values.include? ACE_HIGH
end

# The repeats in the hand
def repeats
cards.group_by &:value
end

# The number of repeats in the hand, unordered
def repeat_counts
repeats.values.map &:count
end

# The values that are repeated more than once, sorted by
# number of occurrences
def repeat_values
repeated = repeats.map { |value, repeats| [value.to_i, repeats.count] }
repeated = repeated.reject { |value, count| count == 1 }
repeated = repeated.sort_by { |value, count| [count, value] }.reverse
repeated.map(&:first)
end

# Values that are not repeated, sorted high to low
def single_values
repeats.select { |value, repeats| repeats.count == 1 }.map(&:first).sort.reverse
end

# Ordered (low to high) array of card values (assumes aces high)
def values
cards.map(&:value).sort
end

# Unordered array of card suits
def suits
cards.map(&:suit)
end

# A "standard" straight, treating aces as high
def aces_high_straight?
straight_values_from(values.first) == values
end

# Special case straight, treating aces as low
def aces_low_straight?
aces? && straight_values_from(aces_low_values.first) == aces_low_values
end

# The card values as an array, treating aces as low
def aces_low_values
cards.map(&:value).map { |v| v == ACE_HIGH ? ACE_LOW : v }.sort
end

private

# Generate an array of 5 consecutive values
# starting with the from value
def straight_values_from(from)
(from...from + 5).to_a
end
end


### Notes and edits

As a rule, I consider aces high (value of 14), and only count them as low (value of 1) when checking for an aces-low straight. That is, an ace Card instance will have a value of 14, but in the context of an aces-low straight, a Hand instance will report it as 1.

Hand and Card instances are considered immutable (though, technically, cards aren't immutable, since I'm using Struct, but that's only for the purposes of this challenge; otherwise I'd define a "proper" class)

Looking at the code again, here are my own concerns (beyond the limitations noted above):

• Some methods return unordered arrays, some sort from high to low, and yet others from low to high. Might be nice to make this more consistent.
• The straight-checking is pretty naïve: Generate 5 consecutive numbers and see if they match the card values. I considered enumerating the values in various ways instead, but a simple == comparison with a generated array seemed more straightforward than what I could come up with.
• There's some repetition required in the RANKS hash keys and the method names, but I found it cleaner than the alternatives I played with.
• Looks like we need more ruby coders... Commented Dec 17, 2013 at 1:17
• @retailcoder Yeah :( Kinda strange actually, given how much emphasis the Ruby world puts on code quality, conventions, idioms and all that. I've been answering a lot of questions, which is why I'd like someone to critique me for once - maybe I'm doing it wrong! But hey, you may get to keep your 50 rep if no one steps up, so it ain't all bad :) Commented Dec 17, 2013 at 2:12
• Nope, the 50 rep is gone already, but that's fine - but I want to award that bounty manually (I want that hat!) so you might want to self-review it, just in case :) Commented Dec 17, 2013 at 2:14
• @retailcoder Ah, right, rep is withdrawn immediately. Well, I'll happily take your money-- I mean rep, but not only as a last resort: If no one appears in time for the bounty, I'll self-review, because you deserve a new hat! :) Commented Dec 17, 2013 at 2:33
• There should be a hat for kill-your-own-zombie - oh, there is one: Sock Puppet :) Commented Dec 17, 2013 at 2:36

I think your example is well structured, I hope my review does it some justice. There are your concerns and some additional points I want to discuss:

You'll find a working implementation of the following code here

1. General coding style

Is nice and consistent. I particularly like your use of question marks to denote methods returning a boolean value.

• Maybe you could omit braces also from method definitions, since you omit them in method calls where possible.
• You left RANKS, your attributes and the initializer without documentation. I think particularly RANKS and the initializer would benefit from it.
2. Repetition in RANKS and instance methods

I think this is ok, and I couln't come up with a better or more readable way.

3. The flush method

They could be written more elegantly using Enumerable#one?

 # If the hand only contains one suit, it's flush
def flush?
suits.uniq.one?
end

4. Decision to make cards a Struct

I like the use of constants in your code to improve readability and maintainability. One thing that caught my eye was that ACE_LOW and ACE_HIGH are global constants, while RANKS is not (which is good). I think this design flaw comes from your decision to make Card a Struct object, and the often deriving lack of logic in these kind of objects. Lets change that and make Card a first class citizen: Your code will benefit.

class Card
include Comparable

# Value to use as ace low
ACE_LOW = 1

# Value to use as ace high
ACE_HIGH = 14

# initialize the card with a suit and a value
def initialize suit, value
super()
@suit = suit
@value = value
end

# Return the low card
def low_card
ace? ? Card.new(suit, ACE_LOW) : self
end

# Return if the card is an ace high
def ace?
value == ACE_HIGH
end

def ace_low?
value == ACE_LOW
end

# Return if the card has suit spades
end

# Return if the card has suit diamonds
def diamonds?
suit == :diamonds
end

# Return if the card is suit hearts
def hearts?
suit == :hearts
end

# Return if the card has suit clubs
def clubs?
suit == :clubs
end

# Compare cards based on values and suits
# Ordered by suits and values - the suits_index will be introduced below
def <=> other
if other.is_a? Card
(suit_index(suit) <=> suit_index(other.suit)).nonzero? || value <=> other.value
else
value <=> other
end
end

# Allow for construction of card ranges across suits
# the suits_index will be introduced below
def succ
if ace?
i = suit_index suit
Card.new(Deck::SUITS[i + 1] || Deck::SUITS.first, ACE_LOW)
else
Card.new(suit, value + 1)
end
end

def successor? other
succ == other
end

def straight_successor? other
succ === other
end

# Compare cards for equality in value
def == other
if other.is_a? Card
value == other.value
else
value == other
end
end
alias :eql? :==

# overwrite hash with value since cards with same values are considered equal
alias :hash :value

# Compare cards for strict equality (value and suit)
def === other
if other.is_a? Card
value == other.value && suit == other.suit
else
false
end
end

private

# If no deck, this has to be done with an array of suits
# gets the suit index
def suit_index suit
Deck::SUITS.index suit
end
end


This would make the following improvements to the code in Hand possible:

class Hand

# ...

# Tie-breaking kickers, ordered high to low.
def kickers
same_of_kind + (aces_low? ? aces_low.reverse : single_cards)
end

# If the hand's straight and flush, it's a straight flush
def straight_flush?
straight? && flush?
end

# Is a value repeated 4 times?
def four_of_a_kind?
same_of_kind? 4
end

# Three of a kind and a pair make a full house
def full_house?
same_of_kind?(3) && same_of_kind?(2)
end

# If the hand only contains one suit, it's flush
def flush?
suits.uniq.one?
end

# This is the only hand where high vs low aces comes into play.
def straight?
aces_high_straight? || aces_low_straight?
end

# Is a card value repeated 3 times?
def three_of_a_kind?
collapsed_size == 2 && same_of_kind?(3)
end

# Are there 2 instances of repeated card values?
def two_pair?
collapsed_size == 2 && same_of_kind?(2)
end

# Any pair?
def pair?
same_of_kind? 2
end

def single_cards
cards.select{|c| cards.count(c) == 1 }
end

# Does the hand include one or more aces?
def aces?
cards.any? &:ace?
end

# Ordered (low to high) array of card values (assumes aces high)
def values
cards.map(&:value).sort
end

# Unordered array of card suits
def suits
cards.map &:suit
end

# A "standard" straight, treating aces as high
def aces_high_straight?
all_successors? cards.sort_by(&:value)
end

# Special case straight, treating aces as low
def aces_low_straight?
aces? && all_successors?(aces_low)
end
alias :aces_low? :aces_low_straight?

# The card values as an array, treating aces as low
def aces_low
cards.map(&:low_card).sort
end

private

# Are there n cards same of kind?
def same_of_kind?(n)
!!cards.detect{|c| cards.count(c) == n }
end

# How many cards vanish if we collapse the cards to single values
def collapsed_size
cards.size - cards.uniq.size
end

# map the cards that are same of kind
def same_of_kind
2.upto(4).map{|n| cards.select{|c| cards.count(c) == n }.reverse }.sort_by(&:size).reverse.flatten.uniq
end

# Are all cards succeeding each other in value?
def all_successors?(cards)
cards.all?{|a| a === cards.last || a.successor?(cards[cards.index(a) + 1]) }
end

end


Also

• it would contain the constants ACE_HIGH and ACE_LOW in Card
• changing a card would be impossible. Values of cards as Struct in frozen cards array can still be modified since the structs respond to value= and suit=
5. The Hand initializer

I think it would be nicer to have cards as a splat argument. To initialize with an array unnecessarily decreases readability. Also, the ArgumentError you raise is not very descriptive, which might lead to some confusion. All in all, this is how my improvements would look like:

def initialize(*cards)
raise ArgumentError.new "wrong number of cards (#{cards.count} for 5)" unless cards.count == 5
@cards = cards.freeze
end


Depending on context of use, an additional sanity check may also be necessary: Check if you really receive 5 instances of Card.

The following points are suggestions where you could go from here

1. Make Hand a subclass of Array

A hand of cards is also an array of cards - The resemblance allows you to subclass Array with Hand. This will allow you to use the Enumerable and Array DSL directly on Hand, which could benefit you if you were to take this code any further (Think about a Deck and a Game class)

Also, the way I'll do it will give you default sorting which should eliminate your problem with unsorted returns, plus you can call sort for suit-and-value based sorting.

Additional sanity checks might be in order as soon as you decide to not freeze Hand on initialization:

• Check if push, unshift, insert and << get an instance of Card as argument
• Check if push, unshift, insert and << don't add too many cards to a hand

So, what does that make possible? Let's refactor:

class Hand < Array

# .. RANKS

def initialize(*cards)
raise ArgumentError.new "There must be 5 cards" unless cards.count == 5
super(cards)
sort_by! &:value # This will give you a nicely sorted hand by default
freeze
end

# The hand's rank as an array containing the hand's
# type and that type's base score
def rank
RANKS.detect { |method, rank| send :"#{method}?" } || [:high_card, 0]
end

# The hand's type (e.g. :flush or :pair)
def type
rank.first
end

# The hand's base score (based on rank)
def base_score
rank.last
end

# The hand's score is an array starting with the
# base score, followed by the kickers.
def score
([base_score] + kickers.map(&:value))
end

# Tie-breaking kickers, ordered high to low.
def kickers
same_of_kind + (aces_low? ? aces_low.reverse : single_cards.reverse)
end

# If the hand's straight and flush, it's a straight flush
def straight_flush?
straight? && flush?
end

# Is a value repeated 4 times?
def four_of_a_kind?
same_of_kind? 4
end

# Three of a kind and a pair make a full house
def full_house?
same_of_kind?(3) && same_of_kind?(2)
end

# If the hand only contains one suit, it's flush
def flush?
suits.uniq.one?
end

# single cards in the hand
def single_cards
select{ |c| count(c) == 1 }.sort_by(&:value)
end

# This is the only hand where high vs low aces comes into play.
def straight?
aces_high_straight? || aces_low_straight?
end

# Is a card value repeated 3 times?
def three_of_a_kind?
collapsed_size == 2 && same_of_kind?(3)
end

# Are there 2 instances of repeated card values?
def two_pair?
collapsed_size == 2 && same_of_kind?(2)
end

# Any repeating card value?
def pair?
same_of_kind?(2)
end

# Does the hand include one or more aces?
def aces?
any? &:ace?
end

# Ordered (low to high) array of card values (assumes aces high)
def values
map(&:value).sort
end

# Ordered Array of card suits
def suits
sort.map &:suit
end

# A "standard" straight, treating aces as high
def aces_high_straight?
all?{|card| card === last || card.successor?(self[index(card) + 1]) }
end
alias :all_successors? :aces_high_straight?

# Special case straight, treating aces as low
def aces_low_straight?
aces? && aces_low.all_successors?
end
alias :aces_low? :aces_low_straight?

# The card values as an array, treating aces as low
def aces_low
Hand.new *map(&:low_card)
end

private

# Are there n cards of the same kind?
def same_of_kind?(n)
!!detect{|card| count(card) == n }
end

def same_of_kind
2.upto(4).map{|n| select{|card| count(card) == n }.reverse }.sort_by(&:size).reverse.flatten.uniq
end

# How many cards vanish if we collapse the cards to single values
def collapsed_size
size - uniq.size
end

end


Together with the Card class, this gives you a nice DSL you can build on:

hand = Hand.new Card.new(:spades, 14), Card.new(:diamonds, 14), Card.new(:hearts, 14), Card.new(:clubs, 14), Card.new(:clubs, 14)
hand.all? &:ace? #=> true, this guy is obviously cheating

hand.count &:ace? #=> 5


Just for fun, if you would have a Deck class which would also subclass Array

class Deck < Array
# the hands this deck creates

# You can install any order here, Bridge, Preferans, Five Hundred
SUITS = %i(clubs diamonds hearts spades).freeze

# Initialize a deck of cards
def initialize
super (Card.new(SUITS.first, 1)..Card.new(SUITS.last, 14)).to_a
shuffle!
end

# Deal n hands
def deal! hands=5
@hands = hands.times.map {|i| Hand.new *pop(5) }
end

# ... and so on
end

deck = Deck.new
deck.deal!

deck.hands.sort_by &:rank #see who's winning

hand = deck.hands.first

# Select cards left in the deck that could be helpful to this hand
deck.select do |card|
hand.any?{|card_in_hand| (card_in_hand..card_in_hand.succ).include? card }
end

2. Where to go further

• Implement Comparable on Hand
• Get rid of freeze on Hand so we can exchange cards for some type of games