Weekend Challenge: Ruby Poker Hand Evaluation

Question

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
  attr_reader :cards

  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.

Answer 1

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
   
     attr_reader :suit, :value
   
     # 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
     def spades?
       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.any? &:spades? #=> true, he has spades
   
   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
     attr_reader :hands
   
     # 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
   • yada yada yada

As I said, your example is already great and I hope you appreciate me sharing my ideas with you!