21
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Looks like we need more ruby coders... \$\endgroup\$ – Mathieu Guindon Dec 17 '13 at 1:17
  • 1
    \$\begingroup\$ @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 :) \$\endgroup\$ – Flambino Dec 17 '13 at 2:12
  • \$\begingroup\$ 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 :) \$\endgroup\$ – Mathieu Guindon Dec 17 '13 at 2:14
  • 1
    \$\begingroup\$ @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! :) \$\endgroup\$ – Flambino Dec 17 '13 at 2:33
  • \$\begingroup\$ There should be a hat for kill-your-own-zombie - oh, there is one: Sock Puppet :) \$\endgroup\$ – Mathieu Guindon Dec 17 '13 at 2:36
9
+50
\$\begingroup\$

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!

\$\endgroup\$
  • 1
    \$\begingroup\$ Now THAT's a review!! (will certainly upvote when I reload at 12AM UTC) \$\endgroup\$ – Mathieu Guindon Dec 17 '13 at 19:29
  • \$\begingroup\$ @retailcoder Your code was quite inspiring :) \$\endgroup\$ – Beat Richartz Dec 17 '13 at 19:32
  • \$\begingroup\$ Are you hat-hunting? \$\endgroup\$ – Mathieu Guindon Dec 17 '13 at 21:16
  • 1
    \$\begingroup\$ @retailcoder Thank you very much - I'll wear that one with pride! \$\endgroup\$ – Beat Richartz Dec 17 '13 at 21:25
  • 3
    \$\begingroup\$ @BeatRichartz Excellent review, thank you! I agree with all your points, basically. This was of course just a Weekend Challenge, so I won't take it further myself, but you've done some nice work there. I don't quite know if I'd subclass Array. I thought about it, and while it makes sense on many levels I also found it a bit "blunt". I'd probably prefer a wholly custom class with a very focussed API to a specialization of a generic class. But either way will work fine, so it's practically just a matte of taste. Again, thanks for the review; great stuff! \$\endgroup\$ – Flambino Dec 18 '13 at 12:00

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.