Ruby Koans - Scoring Project

about_scoring_project.rb

require File.expand_path(File.dirname(__FILE__) + '/edgecase')

# Greed is a dice game where you roll up to five dice to accumulate
# points.  The following "score" function will be used calculate the
# score of a single roll of the dice.
#
# 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 fives is 500 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:
#
# Your goal is to write the score method.

class RollCounter
  
  def self.countNonSetRollsForDieFace(rolls, dieFace)
    rolls.count(dieFace) % 3
  end
  
  def self.countSetsOfThreeForDieFace(rolls, dieFace)
    rolls.count(dieFace) / 3
  end
end

class GreedScoreCalculator
    
  def self.calculateScoreForDieFace(dieFace, numberOfRolls)
    if (dieFace == 1)
      numberOfRolls * 100
    elsif (dieFace == 5)
      numberOfRolls * 50
    end
  end
  
  def self.calculateScoreForSetOfThree(dieFace, numberOfSets)
    if (dieFace == 1)
      numberOfSets * 1000;
    else
      numberOfSets * dieFace * 100;
    end
  end  
end

class Greed
  
  def self.calculateScore(rolls)
        
    score = (1..6).inject(0) do |result, dieFace| 
      result + GreedScoreCalculator.calculateScoreForSetOfThree(dieFace, 
                                                                RollCounter.countSetsOfThreeForDieFace(rolls, dieFace))
    end
    
    return score +
            GreedScoreCalculator.calculateScoreForDieFace(1, RollCounter.countNonSetRollsForDieFace(rolls, 1)) +
            GreedScoreCalculator.calculateScoreForDieFace(5, RollCounter.countNonSetRollsForDieFace(rolls, 5))
  end
end

def score(dice)  
  return Greed.calculateScore(dice)
end

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_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

end

I've been fixated on which are the "new/proper" Ruby methods to use without worrying about non-procedural style at all :) This latest update changes that.

I'm guessing I still need to tackle calculateScoreForSetsOfThree though...

Hmm - I wish there was a way to reply directly to a comment :)

---

With your inject, you are using the pattern:

inject() { |accum, item| accum.add(item); accum }

This is often a flag that there is an abstraction missing for what you are doing here. This is an incredible common pattern that often stems from primitive obsession.

---

Is this still an issue within the class? I suppose we could extract this part into another class that just keeps track of the DiceRolls, and have another class that deals with just scoring...

Here's a thought: do you actually need to pre-process your list of throws into the hash? Do you need to count all the faces before you start processing? What if you counted the faces as you needed them?

Regarding the abstraction, it is more the question of what this class does:
1 - counts the number of times a face appears
2 - runs the rules for scoring sets of 3 (more than one red flag here)
3 - runs the rules for scoring sets of non-3 (more than one red flag here)

I think I'm getting closer to what you're suggesting :) Also, tried to incorporate some of the "state-less" discussions from Chicago -- there doesn't seem to be any need to force the RollCounter to be initialized with the rolls when I can pass them in each time.

I did some reading on the class methods in Ruby vs. static methods in Java/C# and feel they might be appropriate here. What do you think?

My method names are starting to look like Objective-C ones though..pretty long heh.

Getting better. Still a few weird parts.

However, one immediate thing about inject:
If you don't pass a parameter, then it will use the first value in your collection as the first value. So, you can get rid of the #inject(0) and replace it with a simple #inject

If I understand your inject comment, I would write:

(0..6).inject

instead of:

(1..6).inject(0)

That feels a little odd, given that there is no "0" die face. But perhaps having a simpler statement makes up for that fact?

I think the abstractions are starting to gel a little better...
RollCounter - counts die based on face
GreedScoreCalculator - calculates score based on face
Greed - calculates score for roll

I think (heh) that at least the classes only do "one" thing each. Or at least their methods are closely aligned. And there are no methods past 3 "lines" which is an improvement from the start.

I could see the class methods being weird; simple enough to change them...what else feels wrong?

Really appreciate all the comments :)

new to ruby but though it worth firing over my version of the score method, comments welcome...

def score(dice) # You need to write this method

return 0 if dice==[]

one = two = three = four = five = six = result = 0

dice.each do |x|
case x
when 1; one += 1
when 2; two += 1
when 3; three += 1
when 4; four += 1
when 5; five += 1
when 6; six += 1
end
end

if one > 2
result = 1000
one = one - 3
end

if two > 2
result = 100 * 2
elsif three > 2
result = 100 * 3
elsif four > 2
result = 100 * 4
elsif five > 2
result = 100 * 5
five = five - 3
elsif six > 2
result = 100 * 6
end

if one > 0
result = result + (100 * one)
end

if five > 0
result = result + (50 * five)
end

return result

end

new to ruby but though it worth firing over my version of the score method, comments welcome...

def score(dice) # You need to write this method

return 0 if dice==[]

one = two = three = four = five = six = result = 0

dice.each do |x|
case x
when 1; one += 1
when 2; two += 1
when 3; three += 1
when 4; four += 1
when 5; five += 1
when 6; six += 1
end
end

if one > 2
result = 1000
one = one - 3
end

if two > 2
result = 100 * 2
elsif three > 2
result = 100 * 3
elsif four > 2
result = 100 * 4
elsif five > 2
result = 100 * 5
five = five - 3
elsif six > 2
result = 100 * 6
end

if one > 0
result = result + (100 * one)
end

if five > 0
result = result + (50 * five)
end

return result

end

I personnally your solution overkill, but we can argue that it depends how one apporach the excercise.
My take on this as a ruby beginner is here. Comments welcome :)
https://gist.github.com/906429

sebrech: thanks for the link :) I haven't looked at refactoring this code in ages; I do agree your solution is simpler :)