Ruby Koans 182. Refactor help

Question

Can you help me refactor the solution I came up with for Ruby Koans #182? This is the koan in which you write a score method to calculate points for the Greed game. Following code works and all tests pass.

However, it feels long and un-ruby like. How can I make it better?

def score(dice)
rollGreedRoll = Hash.new
rollRollCount = Hash.new
(1..6).each do |roll|
    rollGreedRoll[roll] = roll == 1 ? GreedRoll.new(1000, 100) :
            GreedRoll.new(  100 * roll, roll == 5 ? 50 : 0)
    rollRollCount[roll] = dice.count { |a| a == roll }
end


  score =0 


  rollRollCount.each_pair do |roll, rollCount|
    gr = rollGreedRoll[roll]  
    if rollCount < 3
        score += rollCount * gr.individualPoints
    else
        score += gr.triplePoints + ((rollCount - 3) * gr.individualPoints)

    end 
  end

  return score
end

class GreedRoll
    attr_accessor :triplePoints
    attr_accessor :individualPoints

    def initialize(triplePoints, individualPoints)
        @triplePoints = triplePoints
        @individualPoints = individualPoints
    end
end

Would be more on-topic here: http://codereview.stackexchange.com/ — Merlyn Morgan-Graham, Jul 18 '11 at 20:08
Thanks, I did not know about the codereview site. However, it looks like it is still in beta. — Alper, Jul 18 '11 at 20:12

score 6 · Accepted Answer · edited Dec 31 '11 at 00:56

6

I've put up a walkthrough of refactorings at https://gist.github.com/1091265. Final solution looks like:

def score(dice)
  (1..6).collect do |roll|
    roll_count = dice.count(roll)
    case roll
      when 1 : 1000 * (roll_count / 3) + 100 * (roll_count % 3)
      when 5 : 500 * (roll_count / 3) + 50 * (roll_count % 3)
      else 100 * roll * (roll_count / 3)
    end
  end.reduce(0) {|sum, n| sum + n}
end

note: .reduce is a synonym for .inject

edited Dec 31 '11 at 00:56

chizzle

4,142
5
18
18

answered Jul 19 '11 at 03:56

Nelson Morris

76
1

Looks great. Thanks for taking the time to share your refactorings. – Alper Jul 19 '11 at 13:52
I'm a crappy programmer. Can you please give me an example of what the "dice" parameter would be, and what "dice.count(roll)" will give you? I understand the use of (1..6).collect etc. I'm confused because it seems to me that (1..6).collect and the dice parameter are doing the same thing. Note, I understand the longer answer (by Eric Hutzleman) but yours is very dense for me. Would appreciate if you can elaborate with comments. – Leahcim Aug 26 '12 at 04:03

score 3 · Answer 2 · answered Jul 18 '11 at 20:13

3

You can put the rollRollCount inside the first "each", can't you? Then you don't have to iterate over the (1..6) twice.

answered Jul 18 '11 at 20:13

Wes Gamble

777
10
29

score 2 · Answer 3 · answered Jul 19 '11 at 05:29

Here's another take on it, extracting the method into its own class. A little long winded, but easy to read and understand:

def score(dice)
  GreedScore.new(dice).calculate
end

And the implementation:

class GreedScore
  def initialize(dice)
    @values = dice.sort
  end

  def calculate
    @score = 0
    score_triples
    score_singles
    @score
  end

  private

  def score_triples
    (1..6).each do |match|
      if @values.count(match) >= 3
        @score += match * (match == 1 ? 1000 : 100)
        @values = @values.drop(3)
      end
    end
  end

  def score_singles
    @values.each do |value|
      @score += 100 if value == 1
      @score += 50 if value == 5
    end
  end
end

score 1 · Answer 4 · answered Jun 26 '12 at 21:44

Here was my approach. I used a hash for performance, assuming the number of dice can be large. Plus I love using inject wherever possible.

def score(dice)
  tally = 0

  return tally if dice.length == 0

  hash = dice.inject(Hash.new(0)) { |h,v| h[v] += 1; h }

  (1..6).collect do |roll|
    case roll
    when 5
      tally += (hash[roll] / 3) * 500 + (hash[roll] % 3) * 50
    when 1
      tally += (hash[roll] / 3) * 1000 + (hash[roll] % 3) * 100
    else
      tally += (hash[roll] / 3) * roll * 100
    end
  end

  ap "dice = #{dice}, " + "hash = #{hash}, " + "tally = #{tally}"

  tally
end

Ruby Koans 182. Refactor help

4 Answers4

Linked