patternrubyModerate
Ruby Koans' Greed Task
Viewed 0 times
greedkoanstaskruby
Problem
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.
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:
class AboutScoringProject < EdgeCase::Koan
def test_score_of_an_empty_list_is_zero
assert_equal 0,
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]);
endSome 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:
class AboutScoringProject < EdgeCase::Koan
def test_score_of_an_empty_list_is_zero
assert_equal 0,
Solution
There are a few issues with the code:
Here is a version of the code with the advice above applied:
- Do not check for
== nilwhen it is not specified as a valid value for the method. Here,checking for it and returning 0 might mask another problem.
- 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, orcaseinstead of a series ofifstatement.
- Do not modify parameters that come into your function (
dice.sort!).
- 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
endCode Snippets
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
endContext
StackExchange Code Review Q#423, answer score: 11
Revisions (0)
No revisions yet.