Greed in Ruby Koans

Question

I've finished the greed problem in Ruby Koans after much confusion. This is my first programming language and I have been doing fine, but then this problem really tripped me up for some reason, perhaps because it is one of the first problems where I need to be creating my own method. I eventually came up with this - can you give me your feedback about it?

Is it totally inefficient, whats wrong with it? How would you improve it, and how should I as a novice approach my education - should I settle for the way my mind works right now when I think about problem solving or should I try to understand more complex approaches to the code?

def score(dice)
  total = 0
  count = (1..6).each do |n|
  # for each die, make sure we've counted how many occurrencess there are
  die_value = dice.each { |d| d == n }
   # iterate over each, and handle points for singles and triples
  point_totals = die_value.size
  total += 1000 if die_value.size == 3 and d == 1
  total + 1000 + (point_totals - 3)*100 if die_value.size > 3 and d == 1
  total += 500 if die_value.size == 3 and d == 5
  total + 500 + (point_totals - 3)*50 if die_value.size > 3 and d == 5
  end

Answer 1

Your code is broken, since it doesn't pass the koan's test suite. It doesn't do what it's supposed to do. We typically don't review broken code on CodeReview, but I'll give it a go regardless (since the alternative would probably be to move it back to SO from whence it came).

First, as tadman pointed out in the comments, + is not the same as +=. So two of your lines don't do anything at all, because you're just adding some numbers, but you're not storing the result anywhere.

Secondly, die_value ends up being the same as dice. The each iterator doesn't do anything to the dice array; it doesn't filter or transform it in any way. It does some comparisons along the way, but they don't have any impact on anything.

You also don't use point_totals for anything.

And you store the result of the (1..6).each expression in count, which is what gets returned. But, as mentioned above, each doesn't really do anything to the array (or range, in this case), that you call it on. So your method always returns the range. It simply doesn't return a score at all.

So once we remove those pointless lines, we get this, which is equivalent to your code:

def score(dice)
  1..6
end

That's obviously not right. Let's at least get closer by returning the total instead:

def score(dice)
  total = 0
  (1..6).each do |n| # no need for the count variable
    total += 1000 if dice.size == 3 and d == 1
    total += 500 if dice.size == 3 and d == 5
  end
  total
end

But that's obviously not right either, and does indeed fail the koan's built-in tests. It doesn't handle single 1s and 5s, and it can't handle triples of values other than 1 or 5, and it can't handle multiple triples of any kind.

What you're probably looking for is the select method, which does filter the array you call it on:

n = 2 # as an example

[1,2,2,3].each { |d| d == n }   # => [1,2,2,3] (no change)
[1,2,2,3].select { |d| d == n } # => [2,2] (filtered)

You'll also want to take a look at reduce which is a cleaner option than declaring total = 0, and modifying it from within an each block. Basically, you don't want code to have side-effects; that is, to modify things outside its immediate environment.

And there's group_by which could make the dice-counting simpler.

dice = [1, 2, 3, 2] # an example
groups = dice.group_by { |value| value } # => {1: [1], 2: [2,2], 3: [3]}

Now there's no need to loop through all the 1..6 values, when we can instead look at what dice value are actually present.

By the way, the above can be shortened to dice.group_by(&:to_i) (i.e. group by the result of calling to_i on each item in the array).

Also, what if the dice values you get are seven 1s? That's two triples and one single value (2 * 1000 + 1 * 100 = 2100 points), but your approach (if it had any impact in your code, which it doesn't) only really checks for one triple, and treats what remains as singles. So it would arrive at a score of 1 * 1000 + 4 * 100 = 1400.
So you need check how many triples there are, not just check for one.

Taking the above, we can use reduce to figure out the score

def score(dice)
  dice.group_by(&:to_i).reduce(0) do |score, (value, dice)|
    triples, singles = dice.count.divmod(3) # get the number of triples and singles, i.e. integer division and remainder
    if value == 1
      score += 1000 * triples + 100 * singles
    else
      score += 100 * value * triples
      score += 50 * singles if value == 5
    end
    score
  end
end

We can further simplify that if we use the fact that the scores for triples/singles of 1 are basically 10x the scores for anything else. So instead of the if-else branches, we can do this:

def score(dice)
  dice.group_by(&:to_i).reduce(0) do |score, (value, dice)|
    value = 10 if value == 1 # if value is 1, exchange it for 10
    triples, singles = dice.count.divmod(3)
    score += 10 * singles * value if value % 5 == 0 # only multiples of 5 (aka 5 and 10) award points for singles
    score += 100 * triples * value # all values award points for triples
  end
end