Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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)


  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]);

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, score([])

  def test_score_of_a_single_roll_of_5_is_50
    assert_equal 50, score([5])

  def test_score_of_a_single_roll_of_1_is_100
    assert_equal 100, score([1])

  def test_score_of_a_single_roll_of_1_is_100
    assert_equal 200, score([1,1])

  def test_score_of_multiple_1s_and_5s_is_the_sum_of_individual_scores
    assert_equal 300, score([1,5,5,1])

  def test_score_of_single_2s_3s_4s_and_6s_are_zero
    assert_equal 0, score([2,3,4,6])

  def test_score_of_a_triple_1_is_1000
    assert_equal 1000, score([1,1,1])

  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])

  def test_score_of_mixed_is_sum
    assert_equal 250, score([2,5,2,2,3])
    assert_equal 550, score([5,5,5,5])

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

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

  def test_score_of_a_triple_1_is_1000C
    assert_equal 250, score([1,5,1,2,4])
share|improve this question
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
up vote 13 down vote accepted

No, this is not good ruby code.

Let's start with mistakes:

  • 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( { |h, x| h[x] += 1; h }
  counts = 
  dice.each do |x|
    counts[x] += 1

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

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

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

share|improve this answer
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)

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

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

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

  dice_with_value.size * 100

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

  dice_with_value.size * 50

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
share|improve this answer

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 (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.