Dice roller in Python

Question

I'm trying to get better in Python and decided to rewrite my dice roller in Perl into Python.

#!/usr/bin/env python
"""roll D&D dice"""

from sys import argv
from random import randrange
import re

if len(argv) < 2:
    raise ValueError("args are dice in D&D format like 2d6")

for roll in argv[1:]:
    matched = re.match(r'^(\d*)d(\d+)$', roll)
    if matched:
        print roll + ':'
        roll_sum = 0
        count = 0
        if len( matched.group(1) ):
            count = int( matched.group(1) )
        else:
            count = 1
        sides = int( matched.group(2) )
        for z in range(count):
            rolled = randrange(1, sides+1)
            print "\troll " + str(z) + ': ' + str(rolled)
            roll_sum += rolled

        possible = str( sides * count )
        print "\ttotal: " + str(roll_sum) + "/" + possible
    else:
        print roll + ' is not a dice in 3d8 format'

I've fixed most of the complaints from Pylint. I'm looking for ways to make this more pythonic. If that means more succinct or less, so be it, but I am surprised that the Python version is longer than the Perl version. Is this the cleanest way to handle the implicit roll count as in d6 instead of 1d6?

Answer 1

Input validation

A 0-sided dice for example 3d0 will raise an exception with an ugly stack trace. Since technically it's in "3d8 format", perhaps an additional validation will be a good idea.

Printing error messages

It's a common practice to print error messages to stderr instead of stdout.

Following the pythonic way

A good start will be to follow PEP8, notably:

• the spaces in int( matched.group(1) ) should be removed
• if len( matched.group(1) ) can be simplified to if matched.group(1)

The only thing that limits this script to Python 2.x is the print statements. You can easily make this work in Python 3.x by changing the print statements to print(...) function.

String concatenation is a bit awkward, for example you have to manually convert integers to strings. Instead of this:

    print "\troll " + str(z) + ': ' + str(rolled)

The recommend way:

    print("\troll {}: {}".format(z, rolled))

(Nice pun with the "troll" btw :-)

User-friendliness

An example output looks like this:

2d6:
  roll 0: 6
  roll 1: 2
  total: 8/12

"roll 0" huh... D&D is a nerdy thing, but not really in that way... The roll indexes would be better 1-based instead of 0-based in the output.

Variable span

The span of a variable is the lines between two statements that use it. In this code, roll_sum has a large span. It's initialized early on, but not used until much later.

    print roll + ':'
    roll_sum = 0
    count = 0
    if len( matched.group(1) ):
        count = int( matched.group(1) )
    else:
        count = 1
    sides = int( matched.group(2) )
    for z in range(count):
        rolled = randrange(1, sides+1)
        print "\troll " + str(z) + ': ' + str(rolled)
        roll_sum += rolled

It would be better to move the initialization further down, right before the variable is actually used.

I would also move the print roll + ':' down closer to the statements that do the printing, so as to group similar operations together.

And some empty lines will be nice to create a visual grouping of closely related statements.

Misc

Instead of randrange, you can use randint for an inclusive range. That is, randrange(1, sides+1) can be written equivalently as randint(1, sides).

---

Although it's "intelligent" to combine the rolling, summing and printing steps, the code would be slightly cleaner if these were separated. Something like this:

rolls = [randint(1, sides) for _ in range(count)]

for i, rolled in enumerate(rolls):
    print("\troll {}: {}".format(i + 1, rolled))

possible = sides * count
print("\ttotal: {}/{}".format(sum(rolls), possible))

---

Inside the main for loop for the arguments, the if branch has a long body and the else branch just one line. By the time you read the else branch, the reader might have forgotten what the if was about. In situations like this it might more readable to flip the branches. Actually, once there, eliminating a branch becomes an option too, like this:

if not matched:
    print roll + ' is not a dice in 3d8 format'
    continue

# valid case, no more "else" branch, flatter code

Answer 2

Generic comments

When dealing with command-line arguments, it is often more efficient to reuse an existing framework rather than trying to reinvent the weel. I’ll use argparse as an example but there is also getopt in the standard library as well as many others available on PyPi. Using these framework will let you separate input validation and actual computation.

Next you should write function that performs small tasks and combine them together to achieve your goal. This way, it is simpler to test and debug if anything goes wrong.

You should also get familiar with the str.format syntax to avoid all those str() calls and strings concatenation.

Lastly, using if __name__ == '__main__' you can make your program works both as a command line tool or an importable module. Usefull for debug or reusability purposes.

Optimizations

• A regex used several times should be compiled to improve its efficiency.
• You can use sum and generator expressions to improve your computation
• You should use randint instead of randrange.

Proposed improvements

#!/usr/bin/env python
"""roll D&D dice"""

import re
import argparse
from random import randint


DICE_PATTERN = re.compile(r'^(\d*)d(\d+)$')


def roll(sides):
    value = randint(1, sides)
    print '\troll: {}'.format(value)
    return value


def rolls(die):
    for count, sides in die:
        print '{}d{}:'.format(count, sides)
        total = sum(roll(sides) for _ in xrange(count))
        print '\ttotal: {}/{}'.format(total, count*sides)


def dice(string):
    matched, = re.findall(DICE_PATTERN, string)
    try:
        return map(int, matched)
    except ValueError:
        # Catching the case of 'd6' which will put ('', '6') in matched
        return (1, int(matched[1]))


if __name__ == '__main__':
    parser = argparse.ArgumentParser('Dice roller', description=__doc__)
    parser.add_argument('dice', nargs='+', type=dice)

    args = parser.parse_args()
    rolls(args.dice)