# Is this good Ruby? (Ruby Koans' Greed Task)

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 wonderng: 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.

EDIT: The updated method to review (updated upon glebm's feedback)

``````def score(dice)
patterns = {[1,1,1]=>1000, [2,2,2]=>200, [3,3,3]=>300, [4,4,4]=>400,
[5,5,5]=>500,  [6,6,6]=>600, 1=>100, 5=>50}
sorted = dice.sort

triple = patterns[sorted[0..2]]
single = patterns[sorted[0]]
if triple
partial_score = triple
rest = sorted[3..-1]
elsif single
partial_score = single
rest = sorted[1..-1]
else
partial_score = 0
rest = sorted[1..-1]
end

if rest
partial_score + score(rest)
else
partial_score
end
end
``````

The original method to review

``````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. :) –  Brandon Tilley Feb 25 '11 at 7:44

Here's my solution:

``````def score(dice)
score = 0
dice_grouped = dice.group_by { |i| i }
dice_grouped.each do | number, all_of_number |
if all_of_number.size >= 3
if number == 1
score += 1000*number
else
score += 100*number
end
all_of_number.shift(3)
end
end
score += 100*dice_grouped[1].size if dice_grouped[1]
score += 50*dice_grouped[5].size if dice_grouped[5]
return score
end
``````
-

A little refactoring on the second implementation;

``````PATTERNS = {[1,1,1]=>1000, [2,2,2]=>200, [3,3,3]=>300, [4,4,4]=>400,
[5,5,5]=>500,  [6,6,6]=>600, [1]=>100, [5]=>50}

def score(dice)
return 0 if dice.nil? or dice.empty?

sorted = dice.sort
partial_score = PATTERNS[set=sorted.first(3)] || PATTERNS[set=sorted.first(1)] || 0
rest = sorted.drop(set.length)
partial_score + score(rest)
end
``````
-

don't know if all bugs are ironed out but here is what i came up with while trying to learn koans.

``````
def score(dice)
dice_map = Hash[1,0,2,0, 3,0, 4,0, 5,0, 6,0]
dice.collect {|i| dice_map[i] += 1 }
totalscore= dice_map[1]>=3 ? 1000 + (dice_map[1]-3)*100 : dice_map[1]*100
totalscore += dice_map[5]>=3 ? 500 + (dice_map[5]-3)*50 : dice_map[5]*50
dice_map.delete_if {|k,v| k==1 || k==5}
dice_map.each {|k, v| v>=3 ? totalscore += k*100 : totalscore}

end
``````
-

``````def score(dice)
score = 0
1.upto(6).each { |i| score = i == 1 ? 1000 : 100*i unless dice.count(i) < 3 }
[[1, 100], [5, 50]].each { |i,j| score += j * (dice.count(i) % 3) }
score
end
``````
-
Welcome, Mike. When you are answering a question, please bear in mind that a code review should always consist of more than just an alternate solution: it would be great to see your thought process (what did you change, and why did you change it, and why is it better that way). –  codesparkle Oct 11 '12 at 12:32

This is what I just wrote:

``````def score(dice)

totals = Hash.new(0)

dice.each { |x| totals[x] += 1 }

score = 0

totals.each do |x, t|

if t >= 3
score += x == 1 ? 1000 : (x * 100)
t -= 3;
end

if x == 1
score += t * 100
elsif x == 5
score += t * 50
end

end

score

end
``````

Google brought me here when I decided to look for other examples, so I signed finally signed up! I create a new hash to store the totals of each rolled number, then I loop through it. First, I check if the number was rolled 3 or more times. If it was, I add 1000 if it's a 1 and x * 100 for everything else. I then minus 3 from the number of times rolled so we can properly calculate the single values to add (for example, if [1, 1, 1, 1, 1] was rolled, it should output 1,200). If a one was rolled, 100 * t is added. If a 5 was rolled, 50 * t is added.

The only difference between this and other examples is that I'm accounting for rolls above 3 times (specified in GREED_RULES.txt).

I just started learning Ruby a few days ago, so please comment if you have any optimizations. It passed the asserts in the tutorial fine.

-
``````def score(dice)
sum = 0

unless dice.empty?
# Get score for single die
if dice.length == 1 then
sum = 50 if dice.first == 5
sum = 100 if dice.first == 1
else
if dice.length == 3 && dice.uniq.length == 1 then
sum = dice.first == 1 ? 1000 : dice.first * 100
else
if dice.all? { |d| d == 1 || d == 5 } && dice.uniq.length == 2 then
sum = dice.each.inject(0) {|total, d| total + score([d])}
else
dice.sort!
sum = score([dice[-1]]) + score(dice[0...-1])
end
end
end
end

return sum
end
``````
-

Here's mine... somewhat different than the others.

``````def score(dice)
score = 0
dice.uniq.each{ |die|
count = dice.select{ |x| x == die }.count
case die
when 1
score += (count / 3) * 1000
score += count % 3 * 100
when 5
score += (count / 3) * 500
score += count % 3 * 50
else
score += (count / 3) * (die*100)
end
}
score
end
``````
-
In order to make this answer a better answer, you could add explanations about what is different and why is that better or why did you chose to make it that way. –  Hugo Dozois May 4 at 0:59