Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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, 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
share|improve this question
1  
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

3 Answers 3

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

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

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.

share|improve this answer

protected by Jamal Jan 5 at 23:09

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

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.