# Python greed roll

I am doing the Python Koans to practise Python a bit more and I got to the greed dice project. It is a very simple dice game that scores a set of up to five rolls.

These are the rules:

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


score([5])
score([1, 5, 5, 1])
score([3, 3, 3])


This is my implementation:

def score(dice):
result = 0
if len(dice) <= 5:
dice_copy = sorted(dice)
dice_dict = dict((i, dice_copy.count(i)) for i in dice_copy)
if dice_dict.get(1) >= 3:
result += 1000
dice_dict[1] -= 3
for number in dice_dict:
if dice_dict.get(number) >= 3:
result += number * 100
dice_dict[number] -= 3
if 1 in dice_dict:
result += dice_dict[1] * 100
if 5 in dice_dict:
result += dice_dict[5] * 50

return result


I am looking for:

• comments and tips on my code
• suggestions of Python-style ways of doing this
• improvement and optimizations of my code
• ideas of how to implement this in more interesting, original or interesting ways

I sorted the list before creating the dictionary because I think that would make the dictionary creation faster, but I ran some tests and found out that sorting the list actually makes the program slightly slower. I suppose my intention would only be effective in larger lists. Is my guess right?

The call to sorted doesn't help; .count doesn't know the dictionary is sorted and so can't utilize that fact. The call to sorted just slows things down.

One alternative is to use a loop like

dice_dict = {}
for die in dice:
if die in dice_dict:
dice_dict[die] += 1
else:
dice_dict[die] = 1


which operates in a single pass. This is encapsulated in the Counter class which does the same thing with just Counter(dice).

if dice_dict.get(1) >= 3 is undefined if there are no 1s in the input; .get will return None and None >= 3 returns an arbitrary result. You should use dice_dict.get(1, 0) >= 3, which defaults the argument to 0.

However, if using Counter every value is implicitly defaulted to 0, so you can just do dice_dict[1] >= 3.

Your next loop could also be fixed this way:

for number in dice_dict:
if dice_dict[number] >= 3:
result += number * 100
dice_dict[number] -= 3


but you should just use dice_dict.items():

for number, count in dice_dict.items():
if count >= 3:
result += number * 100
dice_dict[number] -= 3


You can support >= 5 items with a touch of math:

for number, count in dice_dict.items():
result += (count // 3) * number * 100
dice_dict[number] %= 3


// is integer division.

You can merge the first check with the second as so:

for number, count in dice_dict.items():
value = 1000 if number == 1 else number * 100

result += (count // 3) * value
dice_dict[number] %= 3


Your <= 5 check can then be removed, but if you want to keep it you should throw an error instead of silently giving the wrong result.

With Counter you don't need the if x in check; they will default to 0.

This gives:

def score(dice):
result = 0
dice_dict = Counter(dice)

for number, count in dice_dict.items():
value = 1000 if number == 1 else number * 100

result += (count // 3) * value
dice_dict[number] %= 3

result += dice_dict[1] * 100
result += dice_dict[5] * 50

return result


You can remove the mutation of dice_dict by doing modulo outside of the loop:

def score(dice):
result = 0
dice_dict = Counter(dice)

for number, count in dice_dict.items():
value = 1000 if number == 1 else number * 100
result += (count // 3) * value

result += (dice_dict[1] % 3) * 100
result += (dice_dict[5] % 3) * 50

return result


Is it guaranteed that the rolls are of up to 5 die? Otherwise the code doesn't work well. Also, count works without the sorting, so you could do directly

dice_dict = dict((i, dice.count(i)) for i in dice)


However, in this cases, it's just better to just go ahead and use the standard python library:

import collections
...
dice_dict = collections.Counter(dice)

• In the last paragraph I explained why I sorted it and I also asked about my guess. Dec 29, 2014 at 19:59
• I've just updated the answer, there's an even better approach :) Dec 29, 2014 at 20:03
• Can you explain why? Dec 29, 2014 at 20:03
• The collections.Counter is particularly designed for this. It's probably optimized for these matters, but I haven't looked too much under the hood of python to explain why or to be 100% sure of that. Dec 29, 2014 at 20:04

I sorted the list before creating the dictionary because I think that would make the dictionary creation faster, but I ran some tests and found out that sorting the list actually makes the program slightly slower.

Why do you think it would be faster? Here i loops through all the items in the list, and sorting does not change the number of items. Neither is count able to skip any work, even if it called with the same argument multiple times in succession.

dict((i, dice_copy.count(i)) for i in dice_copy)


Sorting takes some time, so of course it makes the program slower.

I suppose my intention would only be effective in larger lists. Is my guess right?

With larger lists the use of count becomes a serious bottleneck, because it scans the whole list every time. Thus obtaining all the counts takes O(n2) time. Really only a single sweep through the list is necessary. collections.Counter will do it for you.

Note that these rules

# * A set of three ones is 1000 points
# * A one (that is not part of a set of three) is worth 100 points.


could be expressed in a slightly simpler way:

# * A one is worth 100 points.
# * A set of three ones is worth an extra 700 points


You could follow that idea to simplify the program logic.

An alternative to using several if statements would be to use dictionaries to represent the scores table.

from collections import Counter

DIE_POINTS = {1: 100, 5: 50}
TRIPLET_POINTS = {1: 1000 - 3 * DIE_POINTS[1],
2: 200,
3: 300,
4: 400,
5: 500 - 3 * DIE_POINTS[5],
6: 600}

def score(dice):
result = 0
if len(dice) <= 5:
counts = Counter(dice)
for number, count in counts.items():
result += count * DIE_POINTS.get(number, 0)
if count >= 3:
result += TRIPLET_POINTS[number]
return result

• @Veedrac Yes, but DIE_POINTS is a regular dictionary here. Dec 30, 2014 at 10:33