12f51e0e2ea40d6fb0df1c64324962a0

I'm having fun going through the Ruby Koans ( http://github.com/edgecase/ruby_koans ), a series of test cases to be solved. It includes the exercise to write a method "score" for the dice game below. My code works, but doesn't feel "efficient"/idiomatic.

The question is: what are the best practises for a method with all kind of checks? How can I avoid countless if- or case-statements?

require 'edgecase'

# Greed is a dice game where you roll up to five dice to accumulate
# points.  The following "score" function will be used calculate the
# score of a single roll of the dice.
#
# A greed roll is scored as follows:
#
# * A set of three ones is 1000 points
#   
# * A set of three numbers (other than ones) is worth 100 times the
#   number. (e.g. three fives is 500 points).
#
# * A one (that is not part of a set of three) is worth 100 points.
#
# * A five (that is not part of a set of three) is worth 50 points.
#
# * Everything else is worth 0 points.
#
#
# Examples:
#
# score([1,1,1,5,1]) => 1150 points
# score([2,3,4,6,2]) => 0 points
# score([3,4,5,3,3]) => 350 points
# score([1,5,1,2,4]) => 250 points
#
# More scoing examples are given in the tests below:
#
# Your goal is to write the score method.

def score(dice)
  points = 0
  dice_freq = [0, 0, 0, 0, 0, 0, 0]

  # the dice_freq array collects the frequency of each number;
  # the number is the array index, the value is the frequency
  # of the number
  dice.each {|d| dice_freq[d] += 1 }

  # 3 times 1 scores 1000
  if dice_freq[1] >= 3
      points += 1000
      dice_freq[1] -= 3
    end

  # 3 times another value scores 100
  dice_freq.each_with_index do |freq, index|
    if dice_freq[index] >= 3
      points += index * 100
      dice_freq[index] -= 3
    end
  end

  # count remaining 1s and 5s
  points += dice_freq[1] * 100 if dice_freq[1] > 0
  points += dice_freq[5] * 50  if dice_freq[5] > 0

  points
end

class AboutScoringAssignment < EdgeCase::Koan
  def test_score_of_an_empty_list_is_zero
    assert_equal 0, score([])
  end

  def test_score_of_a_single_roll_of_5_is_50
    assert_equal 50, score([5])
  end

  def test_score_of_a_single_roll_of_1_is_100
    assert_equal 100, score([1])
  end

  def test_score_of_mulitple_1s_and_5s_is_the_sum
    assert_equal 300, score([1,5,5,1])
  end

  def test_score_of_single_2s_3s_4s_and_6s_are_zero
    assert_equal 0, score([2,3,4,6])
  end

  def test_score_of_a_triple_1_is_1000
    assert_equal 1000, score([1,1,1])
  end

  def test_score_of_other_triples_is_100x
    assert_equal 200, score([2,2,2])
    assert_equal 300, score([3,3,3])
    assert_equal 400, score([4,4,4])
    assert_equal 500, score([5,5,5])
    assert_equal 600, score([6,6,6])
  end

  def test_score_of_mixed_is_sum
    assert_equal 250, score([2,5,2,2,3])
    assert_equal 550, score([5,5,5,5])
  end

end

Refactorings

No refactoring yet !

8f6f95c4bd64d5f10dfddfdcd03c19d6

Rick DeNatale

September 23, 2009, September 23, 2009 17:28, permalink

No rating. Login to rate!

My solution can be found here:

http://github.com/rubyredrick/ruby_koans/blob/master/koans/about_scoring_project.rb

12f51e0e2ea40d6fb0df1c64324962a0

sohooo

September 24, 2009, September 24, 2009 08:23, permalink

No rating. Login to rate!

@Rick DeNatale

The rule at line 25 indicates that three equal numbers doesn't have to appear in consecutive order to score 3x <number>.
That of course means that the assertion at line 96 (in my code sample; line 88 in yours) had to be edited. Your usage of "each_cons" would be wrong in that case, if we go by the rules. (However, if we only look at the assertions, it would be ok :)

Bcd38c655b6d2870194b0ef41575c75b

alech

September 24, 2009, September 24, 2009 19:34, permalink

No rating. Login to rate!

sohooo, thanks a lot for pointing me in the direction of the Ruby Koans, I am new to Ruby and am enjoying them as well now. Below is my solution. And yes, I believe the assertion does contradict the rules and examples, too ...

def score(dice)
    score = 0
    (1..6).each do |n|
        score += triple_score n if triple_present? dice, n
    end
    score += rest_occurences(dice, 1) * 100
    score += rest_occurences(dice, 5) * 50
    score
end

def occurences(dice, number)
    dice.select { |d| d == number }.size
end

def triple_present?(dice, number)
    occurences(dice, number) >= 3
end

def triple_score(number)
    case number
    when 1
        1000
    else
        100 * number
    end
end

def rest_occurences(dice, number)
    o = occurences(dice, number)
    if o >= 3 then
        o - 3 # 3 of them have been accounted for as a set
    else
        o
    end
end
220a1ce7a99b513ece2aca0f6d4688c7

zetafish

September 24, 2009, September 24, 2009 23:10, permalink

No rating. Login to rate!

Hey sohoo, nice tutorial problems on ruby there. Here a regex solution.

S = [    [/111/, 1000],
         [/222/,  200],
         [/333/,  300],
         [/444/,  400],
         [/555/,  500],
         [/666/,  600],
         [/1/,    100],
         [/5/,     50]
    ]

def re_score(s)
  p, n = S.find{ |x| s =~ x.first }
  p.nil? ? 0 : n + re_score($` + $')
end

def score(dice)
  re_score dice.sort.collect{|n|n.to_s}.reduce(:+)
end
C1f7bc8064161e7408ef62d97bb636ac

Mortice

September 25, 2009, September 25, 2009 13:49, permalink

No rating. Login to rate!

My best attempt, not sure if it can be done in one block...

def score(dice)
  total = 0
  dice.sort!
  dice.each_with_index do |die, index|
    if die == dice[index + 1] and die == dice[index + 2] then
      total += if die == 1 then 1000 else die * 100 end
      3.times { dice.shift }
    end
  end
  dice.inject(total) do |sum, die|
    sum += case die
      when 5 then 50
      when 1 then 100
      else 0
    end
  end  
end
5fa7995da0a33e2c6286f6fe734f39a5

Alex O

October 17, 2009, October 17, 2009 10:03, permalink

No rating. Login to rate!

Here is mine, which is basically a highly condensed version of yours. It follows the specification's logic pretty much exactly, as yours does.

def score(dice)
  count = Array.new(6) {|n| dice.select{|i| i-1 == n}.length}
  score = count[0] / 3 * 1000
  (1..5).each {|i| score += count[i] / 3 * 100 * (i+1) }
  score += count[0] % 3 * 100
  score += count[4] % 3 * 50
end
D41d8cd98f00b204e9800998ecf8427e

Desty Nova

October 31, 2009, October 31, 2009 01:55, permalink

No rating. Login to rate! 
def triple(die); die == 1 ? 1000 : die * 100; end
def single(die); die == 1 ? 100 : die == 5 ? 50 : 0; end
def score(array)
  array.sort!
  score= 0
  until array.empty?
    score+= array[0] == array[2] ? triple(array.slice!(0..2).first) : single(array.slice!(0))
  end
  score
end
83e868f4e3921dc0c452511ddf7a3b84

Spencer Steffen

January 7, 2011, January 07, 2011 19:15, permalink

No rating. Login to rate!

Here's what I came up with using only array methods.. 

def score(dice)
  dice.sort!
  total = 0
  triple = dice.take_while{|i| dice.count(i) >= 3 }
  if triple.length >= 3
    val = triple[0]
    total += val == 1 ? 1000 : 100 * val
    dice.slice!(0, 3)
  end
  dice.each do |val|
    case val
      when 1
        total += 100
      when 5
        total += 50
    end
  end
  total
end
D41d8cd98f00b204e9800998ecf8427e

Shannon Skipper

August 25, 2011, August 25, 2011 18:41, permalink

No rating. Login to rate! 
def score(dice)

  @score = 0
  
  def add points
    @score += points
  end
  
  1.upto 6 do |roll|
    count = dice.count roll
    trips = true if count > 2
    add roll * 100 if trips
    case roll
    when 1
      add count * 100
      add 600 if trips
    when 5
      add count * 50
      add -150 if trips
    end
  end
  
  @score
  
end
D41d8cd98f00b204e9800998ecf8427e

Shannon Skipper

August 25, 2011, August 25, 2011 18:42, permalink

No rating. Login to rate!

Trying for readability.

def score(dice)

  @score = 0
  
  def add points
    @score += points
  end
  
  1.upto 6 do |roll|
    count = dice.count roll
    trips = true if count > 2
    add roll * 100 if trips
    case roll
    when 1
      add count * 100
      add 600 if trips
    when 5
      add count * 50
      add -150 if trips
    end
  end
  
  @score
  
end
03e045053520969398da6fc0992812e8

Shannon Skipper

August 25, 2011, August 25, 2011 18:42, permalink

No rating. Login to rate!

Trying for readability.

def score(dice)

  @score = 0
  
  def add points
    @score += points
  end
  
  1.upto 6 do |roll|
    count = dice.count roll
    trips = true if count > 2
    add roll * 100 if trips
    case roll
    when 1
      add count * 100
      add 600 if trips
    when 5
      add count * 50
      add -150 if trips
    end
  end
  
  @score
  
end
D41d8cd98f00b204e9800998ecf8427e

whee

August 5, 2011, August 05, 2011 19:00, permalink

No rating. Login to rate!

I found using a hash to count made the logic->code translation clearer.

def score(dice)
  counts = Hash.new(0)
  dice.each {|die| counts[die] += 1}

  score = 0

# * A set of three ones is 1000 points                                                                                                                         

  score += 1000 * (counts[1] / 3)
  counts[1] = counts[1] % 3

# * A set of three numbers (other than ones) is worth 100 times the                                                                                            
#   number. (e.g. three fives is 500 points).                                                                                                                  

  counts.each {|die, count|
    score += 100 * die * (count / 3)
    counts[die] = count % 3
  }

# * A one (that is not part of a set of three) is worth 100 points.                                                                                            

  score += 100 * counts[1]
  counts[1] = 0

# * A five (that is not part of a set of three) is worth 50 points.                                                                                            

  score += 50 * counts[5]
  counts[5] = 0

  score
end
D41d8cd98f00b204e9800998ecf8427e

whee

August 5, 2011, August 05, 2011 19:00, permalink

No rating. Login to rate!

I found using a hash to count made the logic->code translation clearer.

def score(dice)
  counts = Hash.new(0)
  dice.each {|die| counts[die] += 1}

  score = 0

# * A set of three ones is 1000 points                                                                                                                         

  score += 1000 * (counts[1] / 3)
  counts[1] = counts[1] % 3

# * A set of three numbers (other than ones) is worth 100 times the                                                                                            
#   number. (e.g. three fives is 500 points).                                                                                                                  

  counts.each {|die, count|
    score += 100 * die * (count / 3)
    counts[die] = count % 3
  }

# * A one (that is not part of a set of three) is worth 100 points.                                                                                            

  score += 100 * counts[1]
  counts[1] = 0

# * A five (that is not part of a set of three) is worth 50 points.                                                                                            

  score += 50 * counts[5]
  counts[5] = 0

  score
end
8553855b83cfe2c35648952247e066c5

Gavin

August 27, 2011, August 27, 2011 06:51, permalink

No rating. Login to rate!

Figured out there is a way of doing this without having to use hashes, sort lists before hand etc.
Also, I noticed a lot of the examples above can't handle when you have something like "1,3,1,1,4,1,1,2,1,1". That should be a score of 2100 yeh? It says "A set of three ones is 1000 points", but there is no restriction mentioned on how many sets of threes could exist.

Aynyway, method below, plus some extra tests

def score(dice)

  total = 0
  (1..6).each do |n|
    count = dice.count(n)
    if count>0 then
      # Handle 1
      total += (count/3 * 1000) + (count%3 * 100) if n==1
      # Handle 5
      total += (count/3 * 500) + (count%3 * 50) if n==5
      # Handle others
      total += (count/3 * 100 * n) if n!=5 && n!=1
    end
  end

  # return the running total
  total	  
end



class AboutScoringProject < EdgeCase::Koan
# Additional Tests
  def test_score_of_two_triple_1_is_2000
    # A lot of online examples assume only one triple
    assert_equal 2000, score([1,1,1,1,1,1])
  end
  
  def test_score_of_two_triple_1_split_by_other_is_2000
	# A lot of online examples assume triples are continguous
    assert_equal 2000, score([1,3,1,1,4,1,1,2,1])
  end
  
  def test_score_of_two_triple_1_split_by_other_and_and_extra_1_is_2100
	# A lot of online examples assume triples are continguous
    assert_equal 2100, score([1,3,1,1,4,1,1,2,1,1])
  end

end
8553855b83cfe2c35648952247e066c5

Gavin

August 27, 2011, August 27, 2011 06:51, permalink

No rating. Login to rate!

Figured out there is a way of doing this without having to use hashes, sort lists before hand etc.
Also, I noticed a lot of the examples above can't handle when you have something like "1,3,1,1,4,1,1,2,1,1". That should be a score of 2100 yeh? It says "A set of three ones is 1000 points", but there is no restriction mentioned on how many sets of threes could exist.

Aynyway, method below, plus some extra tests

def score(dice)

  total = 0
  (1..6).each do |n|
    count = dice.count(n)
    if count>0 then
      # Handle 1
      total += (count/3 * 1000) + (count%3 * 100) if n==1
      # Handle 5
      total += (count/3 * 500) + (count%3 * 50) if n==5
      # Handle others
      total += (count/3 * 100 * n) if n!=5 && n!=1
    end
  end

  # return the running total
  total	  
end



class AboutScoringProject < EdgeCase::Koan
# Additional Tests
  def test_score_of_two_triple_1_is_2000
    # A lot of online examples assume only one triple
    assert_equal 2000, score([1,1,1,1,1,1])
  end
  
  def test_score_of_two_triple_1_split_by_other_is_2000
	# A lot of online examples assume triples are continguous
    assert_equal 2000, score([1,3,1,1,4,1,1,2,1])
  end
  
  def test_score_of_two_triple_1_split_by_other_and_and_extra_1_is_2100
	# A lot of online examples assume triples are continguous
    assert_equal 2100, score([1,3,1,1,4,1,1,2,1,1])
  end

end

Your refactoring





Format Copy from initial code

or Cancel