Having coded in Java and C# for quite some years, I'm currently learning Ruby. I'm working my way through the Ruby Koans tutorial. At some point, you are to implement a method that calculates the game-score of a dice-game called Greed.

I came up with this recursive Java/C#-like method. It passes all the supplied unit tests, so technically it's correct.

Now I'm wondering: Is this good Ruby code? If not, how would a "Rubyist" write this method? And possibly: Why? I'm also not so happy about the amount of duplicate code but can't think of a better Rubyish way.

def score(dice)   #dice is an array of numbers, i.e. [3,4,5,3,3]
return 0 if(dice == [] || dice == nil)

dice.sort!

return 1000 + score(dice[3..-1]) if(dice[0..2] == [1,1,1])
return 600 + score(dice[3..-1]) if(dice[0..2] == [6,6,6])
return 500 + score(dice[3..-1]) if(dice[0..2] == [5,5,5])
return 400 + score(dice[3..-1]) if(dice[0..2] == [4,4,4])
return 300 + score(dice[3..-1]) if(dice[0..2] == [3,3,3])
return 200 + score(dice[3..-1]) if(dice[0..2] == [2,2,2])
return 100 + score(dice[1..-1]) if(dice[0] == 1)
return 50 + score(dice[1..-1]) if(dice[0] == 5)
return 0 + score(dice[1..-1]);
end


Some background (if needed)

# Greed is a dice game where you roll up to five dice to accumulate
# points. 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 fours is 400 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 scoring examples are given in the tests below:

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_a_single_roll_of_1_is_100
assert_equal 200, score([1,1])
end

def test_score_of_multiple_1s_and_5s_is_the_sum_of_individual_scores
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

def test_score_of_a_triple_1_is_1000A
assert_equal 1150, score([1,1,1,5,1])
end

def test_score_of_a_triple_1_is_1000B
assert_equal 350, score([3,4,5,3,3])
end

def test_score_of_a_triple_1_is_1000C
assert_equal 250, score([1,5,1,2,4])
end
end

• The second revision of the code looks good to me. Using a hash is a nice idea (though it doesn't make use of the fact that for [x,x,x] with x != 1 the score is x*100, but I guess for 5 numbers doing so would be more noise than helpful). Jan 29 '11 at 22:19

There are a few issues with the code:

1. Do not check for == nil when it is not specified as a valid value for the method. Here,checking for it and returning 0 might mask another problem.
2. Do not use return statements unless necessary. In ruby, almost everything is an expression, and methods return the value of the last expression. Here you can use if...elsif, or case instead of a series of if statement.
3. Do not modify parameters that come into your function (dice.sort!).
4. Do not use recursion if it makes the code less readable.

Here is a version of the code with the advice above applied:

def score(dice)
score = 0
counts = dice.each_with_object(Hash.new(0)) { |x, h| h[x] += 1 }
(1..6).each do |i|
if counts[i] >= 3
score += (i == 1 ? 1000 : 100 * i)
counts[i] = [counts[i] - 3, 0].max
end
score += counts[i] * (i == 1 ? 100 : 50)
end
score
end

• Recursion is not the opposite of straight forward. I found the OP's approach quite straight forward - just very repetitive. Jan 29 '11 at 19:34
• It's not the opposite of straight forward in general. However, in this case I believe it is. Jan 29 '11 at 19:42
• Since coming to ruby, I've actually dropped my rule of "one and only one return" per method. I often have more than a single return statement in a method. Jan 29 '11 at 20:05
• What I am talking about is not multiple return points, but the usage of return keyword Jan 29 '11 at 21:08
• A great review. This clued me into a bug I had to go fix after I read it. :) Feb 25 '11 at 7:44

@glebm has some very good points. I want to also introduce a different style. Here is how I would approach this problem.

def score dice
dice.group_by(&:to_i).inject(0) do |score, combo|
score + combos_score(*combo) + ones_score(*combo) + fives_score(*combo)
end
end

def combos_score dice_value, dice_with_value
number_of_bonues = [dice_with_value.size - 2, 0].max

bonus_for(dice_value) * number_of_bonues
end

def bonus_for dice_value
dice_value == 1 ? 1000 : dice_value * 100
end

def ones_score dice_value, dice_with_value
return 0 if dice_value != 1 || dice_with_value.size > 2

dice_with_value.size * 100
end

def fives_score dice_value, dice_with_value
return 0 if dice_value != 5 || dice_with_value.size > 2

dice_with_value.size * 50
end


I like that

1. Logic for each scoring scenario is isolated together
2. There isn't a need to build a special Hash that would calculate the score.
3. Use of the built in Enumerable#group_by to grab similar die together
4. Small methods that are easy to test