patternpythonMinor
Dice roller in Python
Viewed 0 times
rollerdicepython
Problem
I'm trying to get better in Python and decided to rewrite my dice roller in Perl into Python.
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
#!/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
Printing error messages
It's a common practice to print error messages to
Following the pythonic way
A good start will be to follow PEP8, notably:
The only thing that limits this script to Python 2.x is the
String concatenation is a bit awkward, for example you have to manually convert integers to strings. Instead of this:
The recommend way:
(Nice pun with the "troll" btw :-)
User-friendliness
An example output looks like this:
"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,
It would be better to move the initialization further down, right before the variable is actually used.
I would also move the
And some empty lines will be nice to create a visual grouping of closely related statements.
Misc
Instead of
That is,
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:
Inside the main
the
By the time you read the
the reader might have forgotten what the
In situations like this it might more readable to flip the branches.
Actually, once there, eliminating a branch becomes an option too, like this:
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 toif 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 += rolledIt 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 codeCode Snippets
print "\troll " + str(z) + ': ' + str(rolled)print("\troll {}: {}".format(z, rolled))2d6:
roll 0: 6
roll 1: 2
total: 8/12print 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 += rolledrolls = [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.