HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyModerate

Ruby Koans' Greed Task

Submitted by: @import:stackexchange-codereview··
0
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.

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:

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:

  • 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.



  • 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.



  • 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
end

Code 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
end

Context

StackExchange Code Review Q#423, answer score: 11

Revisions (0)

No revisions yet.