HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Dice roller in Python

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
rollerdicepython

Problem

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+)

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?, 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?

Solution

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

Code Snippets

print "\troll " + str(z) + ': ' + str(rolled)
print("\troll {}: {}".format(z, rolled))
2d6:
  roll 0: 6
  roll 1: 2
  total: 8/12
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
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))

Context

StackExchange Code Review Q#129784, answer score: 7

Revisions (0)

No revisions yet.