8
\$\begingroup\$

I've written the following function on Python (3.3) to take a list of 6 integers and return the maximum score for a game of Farkle. While the results are correct I don't feel it's a very Pythonic implementation. I'd like to hear from the community on how I could make it better and more Pythonic. Any and all tips are most welcome!

"""
    written and tested in Python 3.3.6
"""

from random import randint

def score(my_roll, sides=6):
    """
        Calculate a Farkle score using the traditional scoring method, which is:
            4 1s:     2,000 points
            3 1s:     1,000 points
            Single 1: 100 points
            Single 5: 50 points
            Triple of any non-1 number:    100 x number showing
            Quadruple of any non-1 number: Double the triple score

        Notes:
            - Doubles score nothing (unless a 1 or a 5) as above
            - All scoring dice must be rolled in a single turn (i.e. they
            are not additive over turns)
            - Rolling all 6 will be two sets and scored accordingly
                [4] + [2]
                [3] + [3]
                [2] + [2] + [2]

        Examples:
            1,1,1,5,5,5 ==> 1500 (1000 for 1s + 250 for 3 5s)
            1,1,1,1,6,6 ==> 2000 (2000 for 4 1s)
            5,3,6,5,3,3 ==> 400 (300 for 3 3s + 100 for 2 5s)
            1,2,2,3,3,5 ==> 150 (100 for a 1 and 50 for a 5)
    """
    #create a table to hold the count of each die roll
    dice_array = [0] * sides
    score = 0

    #add up the number appearances of each die roll and store it in the table
    for dice in my_roll:
        dice_array[dice-1] += 1

    """
        based on the above scoring, determine the MAXIMUM score; in actual Farkle the
        player would choose which die to 'bank' and which to re-roll
    """
    for (i, count) in enumerate(dice_array):
        dice = i + 1  #this makes it easier to keep track of the die we're on

        if dice == 1:
            if count == 6: score += 2200
            if count == 5: score += 2100            
            if count == 4: score += 2000
            if count == 3: score += 1000
            if count in [1, 2]: score += (count * 100)
        else:
            if count >= 4: score += (dice * 200)
            if count >= 3: score += (dice * 100)
            if (dice == 5 and count != 3): score += (count * 50)                             

    return score

#test cases
while True:
    roll = input("Enter 6 values (1-6) separated by a space: ")
    roll = [random.randint(1,6) for i in range(0, 6)] if roll == "" else [int(i) for i in roll.split(' ')]
    print(str(score(roll)) + " : " + str(roll))
\$\endgroup\$
  • 2
    \$\begingroup\$ My only comment is that your examples seem to contradict themselves. 1,1,1,5,5,5 should be 1500, for example (1000 for triple 1s and 500 for triple 5s). You specifically call out quadruple 1s as being 2000, but ALL quadruples are double their triple score. \$\endgroup\$ – Adam Smith Jan 6 '16 at 19:53
  • \$\begingroup\$ Cheers for the sharp eye on the example! The rules I was going with, that I linked to, say that it's only 1,000 for 4 1s (no other numbers): A roll of a 1 is worth 100 points. A roll of a 5 is worth 50 points. Three (3) dice rolled at the same time with the same value is worth 100 times the face value, for example: three 2’s rolled is 200 points and three 5’s rolled is 500 points. ** One exception to this rule is that three 1’s rolled is 1,000 points rather than 100 points. \$\endgroup\$ – Matt O'Neill Jan 6 '16 at 21:27
  • \$\begingroup\$ I was writing my code for this hoping it would come out simple and ended with this mess: pastebin.com/fk3J79Sf , well this is life... \$\endgroup\$ – Caridorc Jan 6 '16 at 22:13
  • 1
    \$\begingroup\$ @Caridorc oh gosh that's awful.... \$\endgroup\$ – Adam Smith Jan 6 '16 at 22:53
  • \$\begingroup\$ Your comment "Doubles score nothing (unless a 1 or a 5) as above" is a bit unclear, it looks like you meant "Single or Double 1: 100 points each, Single or Double 5: 50 points each" \$\endgroup\$ – smci Jan 9 '17 at 5:50
8
\$\begingroup\$

"Pythonic" programming

Code is "pythonic" when it expresses its intention clearly, is easy to read or even looks like pseudo-code, and uses as little low-level garbage as possible. Let's take a look at a couple examples of low-level work in your code:

dice_array = [0] * sides
score = 0
for dice in my_roll:
    dice_array[dice-1] += 1
# as an aside, this isn't an array, it's a list.

This is wonky because it buids a fixed-size list, then plays with values-as-list-indexes to get a count. That might be fast, but it's certainly ugly. Let's not do that.

import collections

counts = collections.Counter(my_roll)

This is much better. It builds a dict-like object, a collections.Counter that has the counts for any dice present in the roll. counts[non_rolled_die] will throw a KeyError while counts.get(non_rolled_die, 0) will act like your current code does.

Your counting loop isn't a whole lot better. Let's look:

for i, count in enumerate(dice_array):
    dice = i + 1

We'll stop here for just long enough to mention that enumerate takes a keyword argument start that lets you tell it where to start counting from. This should be:

for die, count in enumerate(dice_array, start=1):

But since I switched to using a collections.Counter, we can just do:

for die, count in counts.items():
    if die == 1:
        if count == 6: score += 2200
        if count == 5: score += 2100
        if count == 4: score += 2000
        if count == 3: score += 1000
        if count in [1, 2]: score += (count * 100)
    else:
        if count >= 4: score += (dice * 200)
        if count >= 3: score += (dice * 100)
        if (die == 5 and count != 3): score += (count * 50)

First of all, avoid these repeated if statements. Try instead:

    if die == 1:
        if count == 3: score += 1000
        elif count == 4: score += 2000
        elif count > 4:
            score += 2000 + (count - 4) * 100

However there's a better approach. We already know that the triple value for each number is 100 * number, except for 1 which is 100 * 10 * number. Let's use that!

for die, count in counts.items():
    die_score = 0
    if count == 4:
        die_score += die * 2000 if die == 1 else die * 200
        count -= 4
    elif count == 3:
        die_score += die * 1000 if die == 1 else die * 100
        count -= 3

This should handle triplets and quadruplets of any value, and removes from count the number of dice consumed by the grouping. Now let's look at single dice values.

    if die in [1, 5]:
        single_value = 100 if die==1 else 50
        die_score += single_value * count

In fact we can factor some of that out to drop the if block.

    # somewhere earlier in the function
    single_values = {1: 100, 5:50}

    # then inside the loop we're looking at here....
    die_score += single_values.get(die, 0) * count

Finally at the end of our loop (just before exiting it), we add the single die_score to the roll-wide score.

    score += die_score

Testing

I'm a big fan of the unittest module for writing tests. In your case doctest may work better (since you've written examples already that only have to be tweaked slightly to use doctest), but if you start writing more intense functions, it's important to be able to grow your testing suite appropriately. Let's write some tests!

# ./test_farkle.py

import unittest
import farkle

class FarkleTests(unittest.TestCase):
    cases = [([1,1,1,5,5,5], 1500),
             ([1,1,1,1,6,6], 2000),
             ([5,3,6,5,3,3], 400),
             ([1,2,2,3,3,5], 150)]

    def testRolls(self):
        for got, want in self.cases:
            self.assertEqual(farkle.score(got), want)
            # fails test if `farkle.score(got) != want`

A change in data structure

It should occur to you that for every (die, count) pair, score increases by a given amount. This means we can hardcode that data in something like a dict.

# {die: {count: value, ... }, ... }
valuedict = {1: {1: 100,
                 2: 200,
                 3: 1000,
                 4: 2000,
                 5: 2100,
                 6: 2200},
             2: {3: 200,
                 4: 400},
             3: {3: 300,
                 4: 600},
             4: {3: 400,
                 4: 800},
             5: {1: 50,
                 2: 100,
                 3: 500,
                 4: 1000,
                 5: 1050,
                 6: 1100},
             6: {3: 600,
                 4: 1200}}

Now your function becomes pretty simple!

def score(my_roll, sides=6):
    valuedict = ...  # the whole deal above
    counts = collections.Counter(my_roll)
    score = 0
    for die, count in counts.items():
        score += valuedict[die][count]
    return score

Or even more simply:

def score(my_roll, sides=6):
    valuedict = ...
    counts = collections.Counter(my_roll)
    return sum(valuedict[die][count] for die,count in counts.items())
\$\endgroup\$
  • \$\begingroup\$ You are incredible! Thank you. Wish I could give you point++! This is exactly the type of breakdown I was looking for. I've just finished two intro python courses and this was my first self test. I knew there was a ton that needed work to make it look like python and you nailed it. I'd actually started the whole thing with collections so I could use the .count method but I kept getting an error that collections couldn't be found. Hence this route! \$\endgroup\$ – Matt O'Neill Jan 7 '16 at 7:07
  • \$\begingroup\$ collections is a stdlib module, just be sure to import it at top level. \$\endgroup\$ – Adam Smith Jan 7 '16 at 9:42
  • \$\begingroup\$ N.B. the "classic" Farkle rules (as I remember them) are here. When you start getting into more complicated combinations (e.g. four of one die, two of another), the hardcoded solution will be useless. \$\endgroup\$ – Adam Smith Jan 7 '16 at 20:25
1
\$\begingroup\$

Make the examples runnable

If You have made the effort of writing your examples, it is a waste to let them sit there doing nothing. Here is the format required to run them.

>>> score([1,1,1,5,5,5])
1250
...

Look into the doctest module.

\$\endgroup\$
  • \$\begingroup\$ I did have that but I thought I'd put them in the comments for people who didn't get my language. I thought providing the self-entry mechanism was more fun, too. \$\endgroup\$ – Matt O'Neill Jan 7 '16 at 7:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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