Greed Dice Scoring Game expanded - Python Koans

Question

I've been trying to level up my python skills (more experienced in other languages) by working through the python3 version of Python Koans. After completing the "greed dice scoring project" exercise, I decided to make it a bit more interesting by allowing for rolls of greater than 5. More on that after the original description:

---

Original for reference: about_scoring_project.py.

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

---

Modifications:

• Allow more than five dice rolls
  • multiple sets are now possible

The following test illustrates these changes:

assertEqual(1000 + 100 + 600*2, score([1, 1, 1, 1, 6, 6, 6, 6, 6, 6]))

Score details: 1000(one set of 1's) + 100(single 1) + 600(two sets of 6's)

---

Review interests

Given that this is part of Python Koans, I'm not looking for any critique of the problem or it's related setup / testing structure. The problem comes with an empty def score() and unit tests (I've also added tests to cover my changes to accepting more than 5 rolls).

Having said that I'm mainly interested in feedback related to:

• pythonic(ality?) -- anything out of place to regular pythonistas?
• naming
• style / readability
• performance considerations
• quality of solution -- is this solution overly complicated?
• overall code quality

---

about_scoring_project.py -- gist for convenience

#!/usr/bin/env python
# -*- coding: utf-8 -*-
from runner.koan import Koan
import math
import functools
import collections
SET = 3
def score(dice_rolls):
 if not dice_rolls:
 return 0
 rolls_to_counts = collections.Counter(dice_rolls)
 rolls_to_scoring_counts = _get_rolls_to_scoring_counts(rolls_to_counts)
 total_points = 0
 total_points += _calc_set_points(rolls_to_scoring_counts)
 total_points += _calc_nonset_points(rolls_to_scoring_counts)
 return total_points
def _get_rolls_to_scoring_counts(rolls_to_counts):
 '''Return a dict of rolls to scoring counts.
 ScoringCounts is a namedtuple: ScoringCounts(sets, nonsets)
 containing the set count and nonset count as values for each roll key.
 '''
 rolls_to_scoring_counts = {}
 ScoringCounts = collections.namedtuple('ScoringCounts', 'sets nonsets')
 for roll in rolls_to_counts:
 cur_roll_count = rolls_to_counts[roll]
 if _roll_has_a_set(cur_roll_count):
 set_count = math.floor(cur_roll_count / SET)
 nonset_count = cur_roll_count % SET
 else:
 set_count = 0
 nonset_count = cur_roll_count
 rolls_to_scoring_counts[roll] = ScoringCounts(set_count, nonset_count)
 return rolls_to_scoring_counts
def _roll_has_a_set(roll_count):
 return roll_count >= SET
def _calc_set_points(rolls_to_scoring_counts):
 def _accumulate_set_points(accum, roll):
 SET_ROLL_TO_POINTS = {
 1: 1000,
 2: 2 * 100,
 3: 3 * 100,
 4: 4 * 100,
 5: 5 * 100,
 6: 6 * 100
 }
 return accum + SET_ROLL_TO_POINTS[roll] * rolls_to_scoring_counts[roll].sets
 return functools.reduce(_accumulate_set_points, rolls_to_scoring_counts, 0)
def _calc_nonset_points(rolls_to_scoring_counts):
 def _accumlate_nonset_points(accum, roll):
 ROLL_TO_POINTS = {
 1: 100,
 2: 0,
 3: 0,
 4: 0,
 5: 50,
 6: 0
 }
 return accum + ROLL_TO_POINTS[roll] * rolls_to_scoring_counts[roll].nonsets
 return functools.reduce(_accumlate_nonset_points, rolls_to_scoring_counts, 0)
# If interested in getting tests to run, check the project https://github.com/gregmalcolm/python_koans/tree/master/python3
# For ref:
# class Koan(unittest.TestCase):
# pass
class AboutScoringProject(Koan):
 def test_score_of_an_empty_list_is_zero(self):
 self.assertEqual(0, score([]))
 def test_score_of_a_single_roll_of_5_is_50(self):
 self.assertEqual(50, score([5]))
 def test_score_of_a_single_roll_of_1_is_100(self):
 self.assertEqual(100, score([1]))
 def test_score_of_multiple_1s_and_5s_is_the_sum_of_individual_scores(self):
 self.assertEqual(300, score([1, 5, 5, 1]))
 def test_score_of_single_2s_3s_4s_and_6s_are_zero(self):
 self.assertEqual(0, score([2, 3, 4, 6]))
 def test_score_of_a_triple_1_is_1000(self):
 self.assertEqual(1000, score([1, 1, 1]))
 def test_score_of_other_triples_is_100x(self):
 self.assertEqual(200, score([2, 2, 2]))
 self.assertEqual(300, score([3, 3, 3]))
 self.assertEqual(400, score([4, 4, 4]))
 self.assertEqual(500, score([5, 5, 5]))
 self.assertEqual(600, score([6, 6, 6]))
 def test_score_of_mixed_is_sum(self):
 self.assertEqual(250, score([2, 5, 2, 2, 3]))
 self.assertEqual(550, score([5, 5, 5, 5]))
 self.assertEqual(1150, score([1, 1, 1, 5, 1]))
 def test_ones_not_left_out(self):
 self.assertEqual(300, score([1, 2, 2, 2]))
 self.assertEqual(350, score([1, 5, 2, 2, 2]))
 def test_score_of_rolls_larger_than_5(self):
 self.assertEqual(0, score([2, 2, 3, 4, 6, 6]))
 def test_score_of_larger_than_5_with_set(self):
 self.assertEqual(600, score([2, 2, 3, 4, 6, 6, 6]))
 self.assertEqual(600, score([2, 2, 3, 6, 6, 6, 6]))
 def test_score_of_larger_than_5_with_multiple_set(self):
 self.assertEqual(600*2, score([2, 2, 3, 4, 6, 6, 6, 6, 6, 6]))
 self.assertEqual(1000 + 600*2, score([1, 1, 1, 4, 6, 6, 6, 6, 6, 6]))
 self.assertEqual(1000 + 100 + 600*2, score([1, 1, 1, 1, 6, 6, 6, 6, 6, 6]))

Answer 1

Overall, the code looks clean and readable, so here are some basics nits and tips for the code snippet in your question.

1. Don't create new variables for everything. Sometimes you don't need them.

   total_points = 0
   total_points += _calc_set_points(rolls_to_scoring_counts)
   total_points += _calc_nonset_points(rolls_to_scoring_counts)
   return total_points
   

   Why not just do this?

   return _calc_set_points(rolls_to_scoring_counts) + _calc_nonset_points(rolls_to_scoring_counts)
   

2. While we're addressing this function, lets discuss code convention. You're using lower snake case, and your naming conventions are consistent, which is good (I suggest taking a look at the PEP8 style guide for a concrete set of rules around Pythonic code). However, some variable names are a bit too long. Descriptive variable names are good, but providing redundant information is not.

   def score(dice_rolls):
    if not dice_rolls:
    return 0
    rolls_to_counts = collections.Counter(dice_rolls)
    rolls_to_scoring_counts = _get_rolls_to_scoring_counts(rolls_to_counts)
    total_points = 0
    total_points += _calc_set_points(rolls_to_scoring_counts)
    total_points += _calc_nonset_points(rolls_to_scoring_counts)
    return total_points
   

   Why not rewrite it this way?

   def score(rolls):
    if not rolls:
    return 0
    counts = collections.Counter(rolls)
    scoring_counts = _get_rolls_to_scoring_counts(counts)
    return _calc_set_points(scoring_counts) + _calc_nonset_points(scoring_counts)
   

   This is much more concise and conveys the same amount of information.

3. Underscore prefixed function names.

   def _get_rolls_to_scoring_counts(rolls_to_counts):
   

   Prefixing with an underscore is generally a developer convention in Python indicating that the method is private. Given the context of your function, I don't think you intend for your code to be a module whose functions are imported and used elsewhere. Having the underscore prefix seems unnecessary to me.

4. Too much abstraction?

   def _roll_has_a_set(roll_count):
    return roll_count >= SET
   

   Defining this seems overkill to me, since you can do this operation in your code.

5. Constants

   ROLL_TO_POINTS = {
    1: 100,
    2: 0,
    3: 0,
    4: 0,
    5: 50,
    6: 0
   }
   

   Extract this to the top of the file as a global constant, like your SET variable.

(削除) 6. Unneeded import.

 return functools.reduce(_accumlate_nonset_points, rolls_to_scoring_counts, 0)

Python has a built-in reduce() function. (削除ここまで)

These are just some basic nits and optimizations. Other than that, I don't see any major glaring errors. Happy coding!

Python Experience: +100

LEVEL UP!

• jsuth: I appreciate the feedback. Regarding 1, I think storing a running total is more readable and adaptable to change. Imagine the game rules change and require some intermediate logic with the the runningtotal - the return statement would need to changed. For 4, to me, roll_has_a_set is much more quickly understood compared to roll_count >= SET. This might be overkill for many but I tend to optimize for readability - perhaps to a fault. For 5, does it really make sense to put these constants in global scope if they aren't used outside of their function?
• omgimanerd: 1) Sure, but you can reasonably assume the rules won't change. Even if they do, it's a quick and easy change. 4) Sure, clarity is always good. It's an optional suggestion. 5) Refactoring it outside means it'll only be initialized once, as opposed to being initialized every time the function runs.
• jsuth: 6) In python 3, reduce() was removed as a builtin, so it seems functools is required here.
• omgimanerd: Ah okay, for some reason I assumed he was using Python 2.7. I'll edit.

Answer 2

Searching the sets

Your first function searches for the sets, so why not call it that. Apart from that, there is little reason so define a namedtuple as you can just return a tuple and use unpacking when calling it

using // (floor division) and some dict comprehension, splitting the dice roll in found sets and remaining dice becomes a lot more easy. dict.get(key, default) can be very handy too to prevent having to check if something is in

def search_simple_sets(dice_rolls: dict):
 set_length = 3
 sets = {dice: dice_rolls.get(dice, 0) // set_length for dice in range(1, 7)}
 remaining_dice = {dice: dice_rolls.get(dice, 0) - count * set_length for dice, count in sets.items()}
 return sets, remaining_dice

Calculating the score

For keeping the scores, I would suggest using a simple dict. Which can be made a lot simpler than manually defining each score. You can just use the built-in sum instead of the functools.reduce and yet again, dict.get() to the rescue

def score_simple(dice_roll: list):
 dice_roll = collections.Counter(dice_roll)
 sets, remaining_dice = search_simple_sets(dice_rolls)
 # define the scores
 set_scores = {i: i*100 for i in range(2, 7)}
 set_scores[1] = 1000 
 dice_score = {1:100, 5: 50}
 # calculating
 set_total = sum(set_scores.get(dice_set, 0) * count for dice_set, count in sets.items())
 remaining_total = sum(dice_score.get(dice, 0) * count for dice, count in remaining_dice.items())
 return set_total + remaining_total

Arbitrary sets

If you do it like this, adding arbitrary sets can be not too difficult. Do keep in mind that the order in which the set_scores is iterated over can affect the result

def score_arbitrary(dice_roll: list):
 dice_roll = collections.Counter(dice_roll)
# define the scores
# the allowed_sets are tuples
 set_scores = {(i,) * 3: i*100 for i in range(2, 7)}
 set_scores[(1, 1, 1,)] = 1000
 set_scores[(2, 3, 4,)] = 999
 dice_score = {1:100, 5: 50}
 sets, remaining_dice = search_arbitrary_sets(dice_roll, set_scores)
 set_total = sum(set_scores.get(dice_set, 0) * count for dice_set, count in sets.items())
 remaining_total = sum(dice_score.get(dice, 0) * count for dice, count in remaining_dice.items())
 return set_total + remaining_total
def dice_rolls_contains(dice_rolls: dict, dice_set: dict):
 return all(dice_rolls.get(key, 0) >= dice_set[key] for key in dice_set)
def search_arbitrary_sets(dice_rolls: dict, allowed_sets):
 sets_found = collections.Counter()
 remaining_dice = collections.Counter(dice_rolls)
 for dice_set in allowed_sets:
 while dice_rolls_contains(remaining_dice, collections.Counter(dice_set)):
 sets_found[dice_set] += 1
 remaining_dice -= collections.Counter(dice_set)
 return sets_found, remaining_dice