Ruby Greed Koan - How can I improve my if/then soup?_问答_开发者

I'm working my way through the Ruby Koans in order to try and learn Ruby, and so far, so good. I've gotten to the greed koan, which at the time of this writing is 183. I've got a working solution, but I feel like I've cobb开发者_运维问答led together just a bunch of if/then logic and that I'm not embracing Ruby patterns.

In the following code, are there ways you would point me to more fully embracing Ruby patterns? (My code is wrapped in "MY CODE [BEGINS|ENDS] HERE" comments.

# 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.

# MY CODE BEGINS HERE

def score(dice)

  # set up basic vars to handle total points and count of each number
  total = 0
  count = [0, 0, 0, 0, 0, 0]

  # for each die, make sure we've counted how many occurrencess there are
  dice.each do |die|
    count[ die - 1 ] += 1
  end

  # iterate over each, and handle points for singles and triples
  count.each_with_index do |count, index|
    if count == 3
      total = doTriples( index + 1, total )
    elsif count < 3
      total = doSingles( index + 1, count, total )
    elsif count > 3
      total = doTriples( index + 1, total )
      total = doSingles( index + 1, count % 3, total )
    end
  end

  # return the new point total
  total

end

def doTriples( number, total )
  if number == 1
    total += 1000
  else
    total += ( number ) * 100
  end
  total
end

def doSingles( number, count, total )
  if number == 1
    total += ( 100 * count )
  elsif number == 5
    total += ( 50 * count )
  end
  total
end

# MY CODE ENDS HERE

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

Thanks so much to any help you can give as I try to get my head around Ruby.

Wow! There are a lot of really cool approaches being done here. I like everybody's creativity. However, I have a pedagogical problem with all of the answers presented here. ("Pedagogy is the study of … the process of teaching." -- Wikipedia)

It is obvious from the first several koans (back in about_asserts.rb) that the Path to Enlightenment does not require any prior/outside knowledge of Ruby. It also seems fairly clear that the Path doesn't even require prior programming experience. So from an educational standpoint, this koan must be answerable using only the methods, language constructs, and programming techniques taught in earlier koans. That means:

no Enumerable#each_with_index
no Enumerable#count
no Enumerable#sort
no Hash.new(0) specifying a default value
no Numeric#abs
no Numeric#divmod
no recursion
no case when
etc

Now, I'm not saying that you are not allowed to use these things in your implementation, but the koan mustn't require using them. There must be a solution that only uses constructs introduced by prior koans.

Also, since the template was just

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

it seemed implied that the solution should not define other methods or classes. That is, you should only replace the # You need to write this method line.

Here is a solution that fits my philosophical requirements:

def score (dice)
    sum = 0
    (1..6).each do |i|
        idice = dice.select { |d| d == i }
        count = idice.size

        if count >= 3
            sum += (i==1 ? 1000 : i*100)
        end
        sum += (count % 3) * 100   if i == 1
        sum += (count % 3) *  50   if i == 5
    end
    sum
end

The methods/constructs here are introduced in the following koan files:

Enumerable#each    about_iteration.rb
Enumerable#select  about_iteration.rb
Array#size         about_arrays.rb
a ? b : c          about_control_statements.rb
%                  about_control_statements.rb

Related StackOverflow Questions:

Ruby Koans 182. Refactor help
https://codereview.stackexchange.com/questions/423/is-this-good-ruby-ruby-koans-greed-task

A student asked Joshu, "How can I write an algorithm to calculate the scores for a dice game?"

Joshu struck the student with his stick and said: "Use a calculator."

def score(dice)
  score = [0, 100, 200, 1000, 1100, 1200][dice.count(1)]
  score += [0, 50, 100, 500, 550, 600][dice.count(5)]
  [2,3,4,6].each do |num|
      if dice.count(num) >= 3 then score += num * 100 end
  end
  score
end

I went through and passed each of the tests one at a time. Not sure this is a very "ruby" solution, but I do like that it's obvious what each section is doing and that there are no excess declarations of values

def score(dice)
  ## score is set to 0 to start off so if no dice, no score
  score = 0
  ## setting the 1000 1,1,1 rule
  score += 1000 if (dice.count(1) / 3) == 1
  ## taking care of the single 5s and 1s here
  score += (dice.count(5) % 3) * 50
  score += (dice.count(1) % 3) * 100
  ## set the other triples here
  [2, 3, 4, 5, 6].each do |num|
    score += num * 100 if (dice.count(num) / 3 ) == 1
  end
  score
end

Looks OK. I might have written some things slightly differently, say:

def do_triples number, total
  total + (number == 1 ? 1000 : number * 100)
end

If you want to do something that few languages other than Ruby can do, I suppose the following might be justifiable under DIE and DRY, on alternate Tuesdays, but I don't think those Ruby maxims were really intended to apply to common subexpression elimination. Anyway:

def do_triples number, total
  total +
  if number == 1
    1000
  else
    number * 100
  end
end

def do_triples number, total
  if number == 1
    1000
  else
    number * 100
  end + total
end

Here's what I did. Looks pretty similar to a few of the older replies. I would love to find some ingenious usage of inject for this one (mikeonbike's one is niiiice).

def score(dice)
  total = 0

  # handle triples scores for all but '1'
  (2..6).each do |num|
    total += dice.count(num) / 3 * num * 100
  end

  # non-triple score for '5'
  total += dice.count(5) % 3 * 50

  # all scores for '1'
  total += dice.count(1) / 3 * 1000 + dice.count(1) % 3 * 100

  total
end

You can condense this down to fewer lines but the readability of the algorithm gets lost so I ended up with this:

def score(dice)
  result = 0;

  (1..6).each do |die|
    multiplier = die == 1 ? 1000 : 100
    number_of_triples = dice.count(die) / 3
    result += die * multiplier * number_of_triples
  end

  result += 100 * (dice.count(1) % 3)

  result += 50 * (dice.count(5) % 3)
end

And if you're using 1.8.6, you'll have to use backports or add the count method to Array yourself:

class Array
  def count(item)
    self.select { |x| x == item }.size
  end
end

Here is the answer I went with after about four iterations and trying to take advantage of the Ruby constructs I'm learning doing the koans:

def score(dice)
  total = 0
  (1..6).each { |roll| total += apply_bonus(dice, roll)}
  return total
end

def apply_bonus(dice, roll, bonus_count = 3)
  bonus = 0
  bonus = ((roll == 1 ? 1000 : 100) * roll) if (dice.count(roll) >= bonus_count)
  bonus += 50 * (dice.count(5) % bonus_count) if (roll == 5)
  bonus += 100 * (dice.count(1) % bonus_count)  if (roll == 1)
  return bonus
end

Yet another answer :)

def score(dice)
  score = 0
  for num in 1..6
    occurrences = dice.count {|dice_num| dice_num == num}
    score += 1000 if num == 1 and occurrences >= 3
    score += 100 * (occurrences % 3) if num == 1
    score += 100 * num if num != 1 and occurrences >= 3
    score += 50 * (occurrences % 3) if num == 5
  end
  score
end

This is the simplest and most readable solution that I came up with. This also accounts for a few situations not in the tests, such as a roll of six 5's or six 1's.

def score(dice)
  score = 0
  (1..6).each { |d|
    count = dice.find_all { |a| a == d }
    score = ( d == 1 ? 1000 : 100 ) * d if count.size >= 3
    score += (count.size - 3) * 50 if (count.size >= 4) && d == 5
    score += (count.size - 3) * 100 if (count.size >= 4) && d == 1  
    score += count.size * 50 if (count.size < 3) && d == 5
    score += count.size * 100 if (count.size < 3) && d == 1
  }
  score
end

I opted to use the size method instead of the count method as count isn't supported by all versions of Ruby and the koans had not tested count up to this test.

def score(dice)
  total = 0
  sets = dice.group_by{|num| num }

  sets.each_pair do |num, values|
    number_of_sets, number_of_singles = values.length.divmod(3)
    number_of_sets.times { total += score_set(num) }
    number_of_singles.times { total += score_single(num) }
  end

  total
end

def score_set(num)
  return 1000 if num == 1
  num * 100
end

def score_single(num)
  return 100 if num == 1
  return 50 if num == 5
  0
end

This was my eventual solution after initially having a similar if/then/else mess on my first attempt.

def score(dice)
  score = 0
  dice.uniq.each do |roll| 
    score += dice.count(roll) / 3 * (roll == 1 ? 1000 : 100*roll)
    score += dice.count(roll) % 3 * (roll == 1 ? 100 : (roll == 5 ? 50 : 0))
  end
  score
end

I'd say you have it looking very Ruby-like already. The only thing that doesn't look very Rubyish to me would be the use of camelCase method names instead of snake_case, but of course that's a personal convention and I haven't read the koans myself.

Other than that, your example wouldn't be improved much by using case/when or any other solution for that matter. Aim for anything less than 3 elseif operations, anything more than that and you'd probably want to hunt for a better solution.

You could shorten [0, 0, 0, 0, 0, 0] to [0] * 6 but aside from the camelCase @injekt mentioned it looks fine to me. I'd be quite happy to see this in a code review.

Also I suppose your doTriples and doSingles don't really need their temporary variables.

def doTriples( number, total )
  if number == 1
    total + 1000
  else
    total + ( number ) * 100 # be careful with precedence here
  end
end

You may want to change

  # for each die, make sure we've counted how many occurrencess there are
  dice.each do |die|
    count[ die - 1 ] += 1
  end

into a hash, such as

count = Hash.new(0)
dice.each do |die|
  count[die] += 1
end

or even

count = {} # Or Hash.new(0)
grouped_by_dots = dice.group_by {|die| die}
1.upto(6) do |dots| # Or grouped_by_dots.each do |dots, dice_with_those_dots|
  dice_with_those_dots = grouped_by_dots.fetch(dots) {[]}
  count_of_that_dots = dice_with_those_dots.length
  count[dots] = count_of_that_dots
end

That way you don't have to have index + 1 littered throughout your code.

It'd be nice if Ruby had a count_by method built in.

My 2 cents. Having new methods for singles/doubles seems like a roundabout way of doing something very simple.

def score(dice)

  #fill initial throws
  thrown = Hash.new(0)
  dice.each do |die|
    thrown[die]+=1
  end

  #calculate score
  score = 0
  faces.each do |face,amount|
    if amount >= 3
      amount -= 3
      score += (face == 1 ? 1000 : face * 100)
    end
    score += (100 * amount) if (face == 1)
    score += (50 * amount) if (face == 5)
  end

  score
end

Well,

Here's my solution:

def score(dice)
    total = 0

    #Iterate through 1-6, and add triples to total if found 
    (1..6).each { |roll| total += (roll == 1 ? 1000 : 100 * roll) if dice.count(roll) > 2 }

    #Handle Excess 1's and 5's
    total += (dice.count(1) % 3) * 100 
    total += (dice.count(5) % 3) * 50
end

Once I found the "count" method for an array, this exercise was pretty easy.

Here is my answer. I do not know if it is good or not, but at least, it looks clear :)

RULEHASH = { 
    1 => [1000, 100],
    2 => [100,0],
    3 => [100,0],
    4 => [100,0],
    5 => [100,50],
    6 => [100,0] 
}

def score(dice)
    score = 0
    RULEHASH.each_pair do |i, rule|
        mod = dice.count(i).divmod(3)
        score += mod[0] * rule[0] * i + mod[1] * rule[1]
    end
    score
end

My solution is not ruby-like style. Just for fun and shortest code. We can set rules through hash p.

def score(dice)
  p = Hash.new([100,0]).merge({1 => [1000,100], 5 => [100,50]})
  dice.uniq.inject(0) { |sum, n| sum + dice.count(n) / 3 * n * p[n][0] + dice.count(n) % 3 * p[n][1] }
end

My answer uses a "lookup table" approach...

def score(dice)
  tally = (1..6).inject(Array.new(7,0)){|a,i| a[i] = dice.count(i); a}
  rubric = {1 => [0,100,200,1000,1100,1200], 5 => [0,50,100,500,550,600]}
  score = rubric[1][tally[1]] + rubric[5][tally[5]]
  [2,3,4,6].each do |i| score += 100 * i if dice.count(i) >= 3 end
  score
end

Mine was similar to a couple of others posted here.

score = 0
[1,2,3,4,5,6].each {|d| 
  rolls = dice.count(d)
  score = (d==1 ? 1000 : 100)*d if rolls >= 3
  score += 100*(rolls % 3) if d == 1 
  score += 50*(rolls % 3) if d == 5 
}
score

Me and my girlfriend were going through these rubykoans this weekend and I had quite a bit of fun golfing on this and trying many different solutions. Here is a reasonably short data-driven solution:

SCORES = [[1000, 100], [200, 0], [300, 0], [400, 0], [500, 50], [600, 0]]

def score(dice)
  counts = dice.group_by(&:to_i).map { |i, j| [i-1, j.length] }
  counts.inject(0) do |score, (i, count)|
    sets, singles = count.divmod 3

    score + sets * SCORES[i][0] + singles * SCORES[i][1]
  end
end

Here is my obligatory one-liner (and perhaps FP version):

SCORES = [[1000, 100], [200, 0], [300, 0], [400, 0], [500, 50], [600, 0]]

def score(dice)
  dice.group_by(&:to_i).inject(0) {|s,(i,j)| s + j.size / 3 * SCORES[i-1][0] + j.size % 3 * SCORES[i-1][1]}
end

I also went some weird routes as well:

SCORES = [[1000, 100], [200, 0], [300, 0], [400, 0], [500, 50], [600, 0]]
def score(dice)
  dice.group_by(&:to_i).inject(0) do |s, (i,j)| 
    s + j.size.divmod(3).zip(SCORES[i-1]).map {|a,b| a*b }.reduce(:+)
  end
end

All programmers should be screwing around with little problems like this...It is like performing morning stretches :)

def score(dice)
    result = 0
    result += 1000 * (dice.find_all{|e| e == 1}).length.divmod(3)[0]
    result += 100 * (dice.find_all{|e| e == 1}).length.divmod(3)[1]
    result += 50 * (dice.find_all{|e| e == 5}).length.divmod(3)[1]
    (2..6).each {|value| result += value*100 * (dice.find_all{|e| e == value}).length.divmod(3)[0]}
    return result
end

And what about this solution? Thanks for the feedback!

def score(dice)
  count = Hash.new(0)
  dice.each do |die|
    count[die] += 1
  end
  total = 0
  count.each_pair { |die, set| total += set < 3 ? single_value(die,set) : triple_value(die,set)}
  total
end

def single_value(die,set)
  value = 0
  value += (set * 100) if die == 1
  value += (set * 50) if die == 5
  value
end

def triple_value(die,set)
  value = 0
  diff = set - 3
  value += single_value(die,diff)
  value += die == 1 ? 1000 : die * 100
  value
end

I used a slightly different method to others here, and (naturally) it's one that I see as preferable. It's very DRY and uses ruby methods fairly extensively to avoid manual loops and branches as much as possible. Should be relatively obvious, but essentially what is happening is we loop through each unique dice roll, and use iterative erosion of the number of occurences of that roll to add the appropriate points to an aggregate total score.

def score(dice)
  score = 0 # An initial score of 0.

  throw_scores = { 1 => 10, 2 => 2, 3 => 3, 4 => 4, 5 => 5, 6 => 6 }
    # A hash to store the scores for each dice throw

  dice.uniq.each { |throw| # for each unique dice value present in the "hand"

    throw_count = (dice.select { |item| item == throw }).count
      # use select to store the number of times this throw occurs

    while throw_count > 0 
      # iteratively erode the throw count, accumulating 
      # points as appropriate along the way.

      if throw_count >= 3
        score += throw_scores[throw] * 100
        throw_count -= 3
      elsif throw == 1 || throw == 5
        score += throw_scores[throw] * 10
        throw_count -= 1
      else
        throw_count -= 1
      end
    end
  }
  return score
end

And another one, just for the fun of it:

def score(dice)
  result = 0
  dice.uniq.each { |k|
    result += ((dice.count(k) / 3) * 1000 + (dice.count(k) % 3) * 100) if k == 1
    result += ((dice.count(k) / 3) * 100 * k + (dice.count(k) % 3) * ( k == 5 ? 50 : 0 )) if k != 1
  }
  result
end

Here's my opinion. All other solutions here try to be clever. There's a place for learning clever tricks, but it's even more important to learn to write clear and maintainable code. The main problem I see with all of these solutions is that it's very difficult to discern the scoring rules from the code. Can you read your solution and make sure that it's correct in your head? Then imagine someone asks you to add a new scoring rule, or remove one. Can you quickly point to the place where the rule must be added or removed?

Here's my solution. I'm sure it can be improved, but look at the shape of the "score" function. This is the sort of code that I would not mind to maintain.

class Array
  def occurrences_of(match)
    self.select{ |number| match == number }.size
  end

  def delete_one(match)
    for i in (0..size)
      if match == self[i]
        self.delete_at(i)
        return
      end
    end
  end
end

def single_die_rule(match, score, dice)
  dice.occurrences_of(match) * score
end

def triple_rule(match, score, dice)
  return 0 if dice.occurrences_of(match) < 3
  3.times { dice.delete_one match }
  score
end

def score(dice)
  triple_rule(1, 1000, dice) +
  triple_rule(2, 200, dice) +
  triple_rule(3, 300, dice) +
  triple_rule(4, 400, dice) +
  triple_rule(5, 500, dice) +
  triple_rule(6, 600, dice) +
  single_die_rule(1, 100, dice) +
  single_die_rule(5, 50, dice)
end

I'm gonna have to go with:

def score(dice)
    # some checks
    raise ArgumentError, "input not array" unless dice.is_a?(Array)
    raise ArgumentError, "invalid array size" unless dice.size <= 5
    raise ArgumentError, "invalid dice result" if dice.any? { |x| x<1 || x>6 }

    # setup (output var, throws as hash)
    out = 0
    freqs = dice.inject(Hash.new(0)) { |m,x| m[x] += 1; m }

    # 3-sets
    1.upto(6) { |i| out += freqs[i]/3 * (i == 1 ? 10 : i) * 100 }

    # one not part of 3-set
    out += (freqs[1] % 3) * 100

    # five not part of 3-set
    out += (freqs[5] % 3) * 50

    out
end

Because most of the solutions presented so far lack basic checks. And some of them are fairly unreadable (in my book) and not very idiomatic.

Granted, the 3-set condition could be made more readable by splitting into two clauses:

    # 3-sets of ones
    out += freqs[1]/3 * 1_000
    # 3-sets of others
    2.upto(6) { |i| out += freqs[i]/3 * i * 100 }

but that's IMO mostly about personal preference.

Coming from Perl, my instinct is to use a hash:

def score(dice)
  # You need to write this method
  score = 0
  count = Hash.new(0)

  for die in dice
    count[die] += 1

    is_triple = (count[die] % 3 == 0)
    if die == 1 then
      score += is_triple ? 800 : 100
    elsif die == 5 then
      score += is_triple ? 400 : 50
    elsif is_triple
      score += 100 * die
    end
  end

  return score
end

This has the advantage that it makes a single pass over dice. I could probably have used an Array in place of the Hash.

I grouped the dice by face, then looped over these groups, first scoring the threes, then individual dice. This is how I would score the game were I playing IRL

def score(dice)
    points = 0
    dice.group_by {|face| face}.each do |face,group|
        while group.size >= 3
            if face == 1
                # A set of three ones is 1000 points
                points += 1000
            else
                # A set of three numbers (other than ones) is worth 100 times the number.
                points += 100 * face
            end
            group.pop(3)
        end
        group.each do |x|
             # A one (that is not part of a set of three) is worth 100 points.
            points += 100 if x==1
            # A five (that is not part of a set of three) is worth 50 points.
            points += 50 if x==5 
        end
    end
    return points
end

That's how I roll

Late to the party, but wanted to take a shot at answering only using knowledge introduced thus far in the Koans. Specifically, I don't use Enumerable#count like most others have.

This seems very straightforward to me, but if anyone happens along, I'd be happy to hear about an optimizations you may have.

And what can I say? I like taking advantage of array indexing.

def score(dice)
  return 0 if dice.empty? # Immediately recognize an empty roll

  # Create an array to hold the scores for each die face
  totals = []
  7.times { totals << 0 }

  # Handle each roll and calculate new score
  dice.each do |roll|
    if roll == 5
      # If we have seen two 5s thus far, make the score 500 for 5s, otherwise add 50
      totals[roll] == 100 ? totals[roll] = 500 : totals[roll] += 50
    elsif roll == 1
      # If we have seen two 1s thus far, make the score 1000 for 5s, otherwise add 100
      totals[roll] == 200 ? totals[roll] = 1000 : totals[roll] += 100
    else
      # If we see any other number three times, score is the number times 100
      totals[roll] == 2 ? totals[roll] = roll * 100 : totals[roll] += 1
    end
  end

  # Count up the scores for each die face; if score is less than 50, then it's just zero
  return totals.inject(0) { |sum, points| points >= 50 ? sum += points : sum }
end