Simplify Card class to fix hellacious bug

The logic is some of the Card constructors, notibly build_from_value and
build_from_face_suit_values was overly complicated and under tested. The
result was a bug where the Card#value did not match the corresponding
suit and face value.

This commit changes the calculate of value (simplifies it a lot). The
value should be internal to the Card class but this is a breaking change
if anyone was depending on it to be stable.

lib/ruby-poker/card.rb

@@ -8,49 +8,43 @@ class Card
     's' => 3
   }
   FACE_VALUES = {
-    'L' =>  1,   # this is a magic low ace
-    '2' =>  2,
-    '3' =>  3,
-    '4' =>  4,
-    '5' =>  5,
-    '6' =>  6,
-    '7' =>  7,
-    '8' =>  8,
-    '9' =>  9,
-    'T' => 10,
-    'J' => 11,
-    'Q' => 12,
-    'K' => 13,
-    'A' => 14
+    'L' =>  0,   # this is a low ace
+    '2' =>  1,
+    '3' =>  2,
+    '4' =>  3,
+    '5' =>  4,
+    '6' =>  5,
+    '7' =>  6,
+    '8' =>  7,
+    '9' =>  8,
+    'T' =>  9,
+    'J' => 10,
+    'Q' => 11,
+    'K' => 12,
+    'A' => 13
   }
 
   def Card.face_value(face)
-    face.upcase!
-    if face == 'L' || !FACE_VALUES.has_key?(face)
-      nil
-    else
-      FACE_VALUES[face] - 1
-    end
+    FACE_VALUES[face.upcase]
   end
 
   private
 
-  def build_from_value(value)
-    @value = value
-    @suit  = value / FACES.size()
-    @face  = (value % FACES.size())
+  def build_from_value(given_value)
+    @suit  = given_value / 13
+    @face  = given_value % 13
   end
 
   def build_from_face_suit(face, suit)
     suit.downcase!
     @face  = Card::face_value(face)
     @suit  = SUIT_LOOKUP[suit]
     raise ArgumentError, "Invalid card: \"#{face}#{suit}\"" unless @face and @suit
-    @value = (@suit * FACES.size()) + (@face - 1)
   end
 
-  def build_from_face_suit_values(face, suit)
-    build_from_value((face - 1) + (suit * FACES.size))
+  def build_from_face_suit_values(face_int, suit_int)
+    @face = face_int
+    @suit = suit_int
   end
 
   def build_from_string(card)
@@ -59,7 +53,6 @@ def build_from_string(card)
 
   # Constructs this card object from another card object
   def build_from_card(card)
-    @value = card.value
     @suit = card.suit
     @face = card.face
   end
@@ -88,9 +81,13 @@ def initialize(*args)
     end
   end
 
-  attr_reader :suit, :face, :value
+  attr_reader :suit, :face
   include Comparable
 
+  def value
+    (@suit * 13) + @face
+  end
+
   # Returns a string containing the representation of Card
   #
   # Card.new("7c").to_s                   # => "7c"
@@ -114,14 +111,14 @@ def <=> card2
   # Returns true if the cards are the same card. Meaning they
   # have the same suit and the same face value.
   def == card2
-    @value == card2.value
+    value == card2.value
   end
   alias :eql? :==
 
   # Compute a hash-code for this Card. Two Cards with the same
   # content will have the same hash code (and will compare using eql?).
   def hash
-    @value.hash
+    value.hash
   end
 
   # A card's natural value is the closer to it's intuitive value in a deck

lib/ruby-poker/poker_hand.rb

@@ -39,7 +39,7 @@ def by_suit
     PokerHand.new(@hand.sort_by { |c| [c.suit, c.face] }.reverse)
   end
 
-  # Returns a new PokerHand object with the cards sorted by value
+  # Returns a new PokerHand object with the cards sorted by face value
   # with the highest value first.
   #
   #     PokerHand.new("3d 5c 8h Ks").by_face.just_cards   # => "Ks 8h 5c 3d"
@@ -447,19 +447,31 @@ def arrange_hand(md)
       hand.strip.squeeze(" ")   # remove extra whitespace
   end
 
-  # delta transform creates a version of the cards where the delta
-  # between card values is in the string, so a regexp can then match a
-  # straight and/or straight flush
+  # delta transform returns a string representation of the cards where the
+  # delta between card values is in the string. This is necessary so a regexp
+  # can then match a straight and/or straight flush
+  #
+  # Examples
+  #
+  #   PokerHand.new("As Qc Jh Ts 9d 8h")
+  #   # => '0As 2Qc 1Jh 1Ts 19d 18h'
+  #
+  #   PokerHand.new("Ah Qd Td 5d 4d")
+  #   # => '0Ah 2Qd 2Td 55d 14d'
+  #
   def delta_transform(use_suit = false)
+    # In order to check for both ace high and ace low straights we create low
+    # ace duplicates of all of the high aces.
     aces = @hand.select { |c| c.face == Card::face_value('A') }
-    aces.map! { |c| Card.new(1,c.suit) }
+    aces.map! { |c| Card.new(0, c.suit) }  # hack to give the appearance of a low ace
 
     base = if (use_suit)
       (@hand + aces).sort_by { |c| [c.suit, c.face] }.reverse
     else
       (@hand + aces).sort_by { |c| [c.face, c.suit] }.reverse
     end
 
+    # Insert delta in front of each card
     result = base.inject(['',nil]) do |(delta_hand, prev_card), card|
       if (prev_card)
         delta = prev_card - card.face

test/test_card.rb

@@ -7,10 +7,11 @@ def setup
     @c2 = Card.new("TD")
     @c3 = Card.new("jh")
     @c4 = Card.new("qS")
+    @c5 = Card.new("AC")
   end
 
   def test_class_face_value
-    assert_nil(Card.face_value('L'))
+    assert_equal(0,  Card.face_value('L'))
     assert_equal(13, Card.face_value('A'))
   end
 
@@ -19,17 +20,45 @@ def test_build_from_card
   end
 
   def test_build_from_value
-    assert_equal(@c1, Card.new(7))
+    assert_equal(@c1, Card.new(8))
     assert_equal(@c2, Card.new(22))
-    assert_equal(@c3, Card.new(37))
-    assert_equal(@c4, Card.new(52))
+    assert_equal(@c3, Card.new(36))
+    assert_equal(@c4, Card.new(50))
+    assert_equal(@c5, Card.new(13))
   end
 
   def test_build_from_face_suit
-    assert_equal(7, Card.new('9', 'c').value)
+    assert_equal(8, Card.new('9', 'c').value)
     assert_equal(22, Card.new('T', 'd').value)
-    assert_equal(37, Card.new('J', 'h').value)
-    assert_equal(52, Card.new('Q', 's').value)
+    assert_equal(36, Card.new('J', 'h').value)
+    assert_equal(50, Card.new('Q', 's').value)
+    assert_equal(13, Card.new('A', 'c').value)
+  end
+
+  def test_build_from_value_and_from_face_suit_match
+    ticker = 0
+    Card::SUITS.each_char do |suit|
+      "23456789TJQKA".each_char do |face|
+        ticker += 1
+        from_value = Card.new(ticker)
+        from_face_suit = Card.new(face, suit)
+        assert_equal(from_face_suit, from_value,
+                     "Face and suit #{face + suit} did not match card from value #{ticker}")
+      end
+    end
+  end
+
+  def test_build_from_value_and_from_face_suit_values_match
+    ticker = 0
+    0.upto(3) do |suit|
+      1.upto(13) do |face|
+        ticker += 1
+        from_value = Card.new(ticker)
+        from_face_suit_values = Card.new(face, suit)
+        assert_equal(from_face_suit_values, from_value,
+                     "Face=#{face} and suit=#{suit} did not match card from value #{ticker}")
+      end
+    end
   end
 
   def test_face
@@ -47,10 +76,11 @@ def test_suit
   end
 
   def test_value
-    assert_equal(7, @c1.value)
+    assert_equal(8, @c1.value)
     assert_equal(22, @c2.value)
-    assert_equal(37, @c3.value)
-    assert_equal(52, @c4.value)
+    assert_equal(36, @c3.value)
+    assert_equal(50, @c4.value)
+    assert_equal(13, @c5.value)
   end
 
   def test_natural_value

test/test_poker_hand.rb

@@ -102,7 +102,10 @@ class TestPokerHand < Test::Unit::TestCase
 
     should "recognize a straight" do
       assert @straight.straight?
+      # ace low straight
       assert PokerHand.new("AH 2S 3D 4H 5D").straight?
+      # ace high straight
+      assert PokerHand.new("AH KS QD JH TD").straight?
     end
 
     should "recognize a three of a kind" do