8
\$\begingroup\$

I tried a Yahtzee kata in Python the other day. I'm not particularly experienced in Python (C# is my main language) so I'd be interested in any feedback as to how I could have done things better. In particular, I wonder whether my Count and HighestRepeated methods could be a little more elegant.

import unittest

#helpers
def Count(dice, number):
    return len([y for y in dice if y == number])

def HighestRepeated(dice, minRepeats):
    unique = set(dice)
    repeats = [x for x in unique if Count(dice, x) >= minRepeats]
    return max(repeats) if repeats else 0

def OfAKind(dice, n):
    return HighestRepeated(dice,n) * n

def SumOfSingle(dice, selected):
    return sum([x for x in dice if x == selected])

#strategies
def Chance(dice):
    return sum(dice)

def Pair(dice):
    return OfAKind(dice, 2)

def ThreeOfAKind(dice):
    return OfAKind(dice, 3)

def FourOfAKind(dice):
    return OfAKind(dice, 4)

def SmallStraight(dice):
    return 15 if tuple(sorted(dice)) == (1,2,3,4,5) else 0

def LargeStraight(dice):
    return 20 if tuple(sorted(dice)) == (2,3,4,5,6) else 0

def Ones(dice):
    return SumOfSingle(dice,1)

def Twos(dice):
    return SumOfSingle(dice,2)

def Threes(dice):
    return SumOfSingle(dice,3)

def Fours(dice):
    return SumOfSingle(dice,4)

def Fives(dice):
    return SumOfSingle(dice,5)

def Sixes(dice):
    return SumOfSingle(dice,6)

def Yahtzee(dice):
    return 50 if len(dice) == 5 and len(set(dice)) == 1 else 0

class YahtzeeTest(unittest.TestCase):
    testCases = (
        ((1,2,3,4,5), 1, Ones),
        ((1,2,3,4,5), 2, Twos),
        ((3,2,3,4,3), 9, Threes),
        ((3,2,3,4,3), 0, Sixes),
        ((1,2,3,4,5), 0, Pair), # no pairs found
        ((1,5,3,4,5), 10, Pair), # one pair found
        ((2,2,6,6,4), 12, Pair), # picks highest
        ((2,3,1,3,3), 6, Pair), # only counts two
        ((2,2,6,6,6), 18, ThreeOfAKind), 
        ((2,2,4,6,6), 0, ThreeOfAKind), # no threes found
        ((5,5,5,5,5), 15, ThreeOfAKind), # only counts three
        ((6,2,6,6,6), 24, FourOfAKind), 
        ((2,6,4,6,6), 0, FourOfAKind), # no fours found
        ((5,5,5,5,5), 20, FourOfAKind), # only counts four
        ((1,2,5,4,3), 15, SmallStraight),
        ((1,2,5,1,3), 0, SmallStraight),
        ((6,2,5,4,3), 20, LargeStraight),
        ((1,2,5,1,3), 0, LargeStraight),
        ((5,5,5,5,5), 50, Yahtzee),
        ((1,5,5,5,5), 0, Yahtzee), 
        ((1,2,3,4,5), 15, Chance),
        )

    def testRunAll(self):
        for (dice, expected, strategy) in self.testCases:
            score = strategy(dice)
            self.assertEquals(expected, score, "got {0} expected {1}, testing with {2} on {3}".format(score, expected, strategy.__name__, dice))
        print 'ran {0} test cases'.format(len(self.testCases))

if __name__ == '__main__':
    unittest.main()
\$\endgroup\$

1 Answer 1

8
\$\begingroup\$
import unittest

#helpers
def Count(dice, number):

The python style guide recommends lowercase_with_underscores for global functions.

    return len([y for y in dice if y == number])

This can be written as

    return dice.count(number)

def HighestRepeated(dice, minRepeats):
    unique = set(dice)
    repeats = [x for x in unique if Count(dice, x) >= minRepeats]
    return max(repeats) if repeats else 0

This will be somewhat clearer if you find the highest repeated count and then check it against minRepeats. You can also use the counter class:

counts = collections.Counter(dice)
element, count = counts.most_common()[0]
return count if minRepeats <= count else 0


def OfAKind(dice, n):
    return HighestRepeated(dice,n) * n

def SumOfSingle(dice, selected):
    return sum([x for x in dice if x == selected])

Could be written as

    return collections.Counter(dice)[selected]

#strategies
def Chance(dice):
    return sum(dice)

def Pair(dice):
    return OfAKind(dice, 2)

def ThreeOfAKind(dice):
    return OfAKind(dice, 3)

def FourOfAKind(dice):
    return OfAKind(dice, 4)

def SmallStraight(dice):
    return 15 if tuple(sorted(dice)) == (1,2,3,4,5) else 0

Shouldn't a small straight consist of 4 elements in a row?

def LargeStraight(dice):
    return 20 if tuple(sorted(dice)) == (2,3,4,5,6) else 0

Can't a large straight start with 1?

def Ones(dice):
    return SumOfSingle(dice,1)

def Twos(dice):
    return SumOfSingle(dice,2)

def Threes(dice):
    return SumOfSingle(dice,3)

def Fours(dice):
    return SumOfSingle(dice,4)

def Fives(dice):
    return SumOfSingle(dice,5)

def Sixes(dice):
    return SumOfSingle(dice,6)

Writing so many functions that differ only in a parameter they pass to another function is odd.

def Yahtzee(dice):
    return 50 if len(dice) == 5 and len(set(dice)) == 1 else 0

Can't you assume that there are always 5 dice? Also, why don't you use OfAKind?

class YahtzeeTest(unittest.TestCase):
    testCases = (
        ((1,2,3,4,5), 1, Ones),
        ((1,2,3,4,5), 2, Twos),
        ((3,2,3,4,3), 9, Threes),
        ((3,2,3,4,3), 0, Sixes),
        ((1,2,3,4,5), 0, Pair), # no pairs found
        ((1,5,3,4,5), 10, Pair), # one pair found
        ((2,2,6,6,4), 12, Pair), # picks highest
        ((2,3,1,3,3), 6, Pair), # only counts two
        ((2,2,6,6,6), 18, ThreeOfAKind), 
        ((2,2,4,6,6), 0, ThreeOfAKind), # no threes found
        ((5,5,5,5,5), 15, ThreeOfAKind), # only counts three
        ((6,2,6,6,6), 24, FourOfAKind), 
        ((2,6,4,6,6), 0, FourOfAKind), # no fours found
        ((5,5,5,5,5), 20, FourOfAKind), # only counts four
        ((1,2,5,4,3), 15, SmallStraight),
        ((1,2,5,1,3), 0, SmallStraight),
        ((6,2,5,4,3), 20, LargeStraight),
        ((1,2,5,1,3), 0, LargeStraight),
        ((5,5,5,5,5), 50, Yahtzee),
        ((1,5,5,5,5), 0, Yahtzee), 
        ((1,2,3,4,5), 15, Chance),
        )

    def testRunAll(self):
        for (dice, expected, strategy) in self.testCases:
            score = strategy(dice)
            self.assertEquals(expected, score, "got {0} expected {1}, testing with {2} on {3}".format(score, expected, strategy.__name__, dice))
        print 'ran {0} test cases'.format(len(self.testCases))

You are duplicating some of the logic of the unit test system here. I recommend using nose tests which would allow this one function to generate a number of tests each of which is considered an independent test by the unit testing system. Then you wouldn't need to spend so much effort.

I'd also consider writing functions for each of your tests rather then putting them in a huge list. The list technique is really nice but its best used for calling the same function. Using it to test so many different functions is odd.

if __name__ == '__main__':
    unittest.main()
\$\endgroup\$
4
  • \$\begingroup\$ Thanks, some really good suggestions there - I knew there had to be a better way to do my count function. As far as I can tell, the large and small straights are what the kata wiki specifies - although it may have got the yahtzee rules wrong. I'll give nose a try - I was looking for an equivalent to the NUnit TestCase attribute which I find very useful. \$\endgroup\$
    – Mark Heath
    Jun 21, 2011 at 5:46
  • \$\begingroup\$ You only score a Yahtzee if you roll five of a kind. Assuming these functions are used to score the remaining dice after saving some to be scored, they need to correctly score fewer than five dice. \$\endgroup\$ Jun 21, 2011 at 7:52
  • \$\begingroup\$ @Mark, yeah your source does seem to indicate those rules for straights. It looks like it results from a misunderstanding of the original rules. However, the rules for a real straight might be a more interesting implementation challenge. \$\endgroup\$ Jun 21, 2011 at 22:04
  • \$\begingroup\$ @David, that's not how I remember Yahtzee working. (i.e. you can only score one category at a time.) But in that case yes. \$\endgroup\$ Jun 21, 2011 at 22:05

Not the answer you're looking for? Browse other questions tagged or ask your own question.