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). – sepp2k Jan 29 '11 at 22:19

No, this is not good ruby code.

• Do not check for == nil. nil is not specified as a valid value for the method, therefore checking for it and returning 0 might mask other exceptions
• Do not use return statement. If you need to do series of if statements, just use if...elsif, or case
• Do not modify parameters that come into your function. I am referring to dice.sort!
• Do not use recursion when it would be a lot cleaner to do it the straight-forward way

Considering all the above, here is a cleaned up version of the code:

def score(dice)
score = 0

# Below is equivalent to:
#   counts = dice.inject(Hash.new(0)) { |h, x| h[x] += 1; h }
counts = Hash.new(0)
dice.each do |x|
counts[x] += 1
end

(1..6).each do |i|
if counts[i] >= 3
if i == 1
score += 1000
else
score += 100 * i
end

counts[i] = [counts[i] - 3, 0].max
end

if i == 1
score += 100 * counts[i]
elsif i == 5
score += 50 * counts[i]
end
end

score
end

-
Recursion is not the opposite of straight forward. I found the OP's approach quite straight forward - just very repetitive. – sepp2k Jan 29 '11 at 19:34
It's not the opposite of straight forward in general. However, in this case I believe it is. – glebm 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. – Barry Hess Jan 29 '11 at 20:05
What I am talking about is not multiple return points, but the usage of return keyword – glebm Jan 29 '11 at 21:08
A great review. This clued me into a bug I had to go fix after I read it. :) – Michelle Tilley 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
-

## protected by Jamal♦Jan 5 '14 at 23:09

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site.