Die Roller for command line

Question

I've written a die roller which can be run from the command line. It currently exists as three files, but I'll say later why I'd like to make it four. The files are as follows:

• die.py: Defines the Side and Die classes and extends Die with the NSided and Fudge classes.
• summer.py: Defines the default Summer class and extends it with Highest, a class that will sum the highest values in a set of rolls.
• parser.py: Implements main functionality of the application by creating and calling instances of die and summer classes, as well as predefined values. Takes arguments from the user and returns the sum of all values.

I feel that die.py is well implemented but am willing to hear how it can be improved. The other files are less concise and will likely get more of my attention.

from random import choice

class Side(dict):
    """A side contains all info needed by a die and roller.

    >>> d = Side(4, 'death')
    >>> print d
    death
    >>> print d.keys()
    ['char', 'num']
    >>> print d.values()
    ['death', 4]
    >>>

    """

    def __init__(self, num, char):
        """
        Args:
            num (int) - Used to calculate value of roll.
            char (str) - Used to display face of roll.

        """

        self['num'] = num
        self['char'] = char


    def __repr__(self):
        return str(self['char'])
    __str__ = __repr__



class Die(list):
    """A die is a list of sides, all of equal probability."""

    def __init__(self, sides):
        for side in sides: self.append(side)

    def roll(self):
        return choice(self)



class NSided(Die):
    """Returns an n-sided die.

    Args:
        numsides (int) - The highest value on the die.

    >>> d = NSided(6)
    >>> print d
    [1, 2, 3, 4, 5, 6]
    >>>

    """

    def __init__(self, numsides):
        sides = []
        for i in range(numsides):
            sides.append(Side(i+1, str(i+1)))
        Die.__init__(self, sides)



class Fudge(Die):
    """Returns a Fudge die.

    >>> d = Fudge()
    >>> print d
    [-,  , +]
    >>>

    """

    def __init__(self):
        sides = []
        sides.append(Side(-1, '-'))
        sides.append(Side(0, ' '))
        sides.append(Side(1, '+'))
        Die.__init__(self, sides)



if __name__ == '__main__':
    import doctest
    doctest.testmod()

summer.py was difficult to write because I was indecisive about the extent of its role. It works, and for now that's good enough, but I'd like to revisit this file and consider how to improve upon the idea. This was the first time I found a use for generators and using one threw new problems at me.

Summers are used in a single die declaration to determine the final value given when throwing a set of dice. In DnD, an ability roll is a set of four six-sided dice with the lowest removed; the rest are summed. This is implemented in the Highest class which could simulate this behavior as Highest(NSided(6), '3,4').sum(). For regular old arithmetic type summing, the Summer class does just fine.

class Summer(object):

    def __init__(self, die, summer_args):
        """Sums a series of die rolls."""

        # (Die) object
        self.die = die
        # (int) dice of the same type, (dict)
        self.number_of_dice, self.args = self.parse_args(summer_args)
        self.roller = self.get_roller()


    def get_roller(self):
        """Returns a generator which yields die sides."""
        # print "self.number_of_dice" + ", " + str(type(self.number_of_dice)) + ", " + str(self.number_of_dice)
        return (self.die.roll() for i in range(self.number_of_dice))


    def parse_args(self, arg):
        try:
            num_dice = int(arg)
        except ValueError:
            num_dice = 1
        return num_dice, {}


    def sum(self):
        rolls = []
        while True:
            try:
                rolls.append(self.roller.next()['num'])
            except StopIteration:
                break
        return sum(rolls)



class Highest(Summer):

    def parse_args(self, arg):
        number_of_dice = int(arg[arg.find(',')+1:])
        number_to_count = int(arg[:arg.find(',')])
        return number_of_dice, {'number_to_count': number_to_count}


    def sum(self):
        rolls = []
        while True:
            try:
                rolls.append(self.roller.next()['num'])
                if len(rolls) > self.args['number_to_count']: rolls.remove(min(rolls))
            except StopIteration:
                break
        return sum(rolls)

This is my first, successful attempt at writing a parser. parser.py will accept a series of arguments and return the sum of their values.

$ python parser.py 1d20 7 combat_advantage
15

It can accept die declarations (1d20), integers (7) and predefined values (combat_advantage).

Integers can be positive or negative and are directly added to the total.

Die declarations are handled simply. A summer and die are declared, separated by a 'd'. If either the summer or die is an integer, the default is called. The default summer is Summer and the default die is NSided. The following are all valid die declarations: 1d20 2d4 4dF H3,4d6. When indicating a summer or die, a single-character flag can be used to specify a non-default type. Summers are passed the slice between the flag and 'd'. Dice take no arguments, with the exception of NSided, which needs a number of sides.

Predefined values are the part that became most interesting to me. They can be as simple as giving a string an integer value ('difficult_terrain': -2,) but can also contain references to other values (VALUES['CA'] = VALUES['combat_advantage']) or to nested declarations ('fudge': '4dF',). This nesting can be as deep or complex as the user wants. A user might define a common attack with its die declaration and modifier, then call that attack by its name. They could then write my_sword_attack combat_advantage or even use synonyms, SAtt CA.

Overall, this file does its job. I'm happy with that, but I'm not convinced it does it well. The other files were built to be extended. I can't say the same for the parser. Of the three, it is most at risk of being entirely re-written.

#!/usr/bin/python

import die as _die
import summer as _summer

# Die and summer flags must be a single letter.  Lowercase 'd' is not valid.
DICE = {
    'default': _die.NSided,
    'F': _die.Fudge,
}
SUMMERS = {
    'default': _summer.Summer,
    'H': _summer.Highest,
}

# Values must begin with a letter or underscore
VALUES = {
    'ability': 'H3,4d6',
    'combat_advantage': 2,
    'difficult_terrain': -2,
    'fudge': '4dF',
}
# Synonyms...
VALUES['CA'] = VALUES['combat_advantage']
VALUES['DT'] = VALUES['difficult_terrain']


def is_int(arg):
    try:
        int(arg)
        return True
    except ValueError:
        return False


def main(args):
    values = []
    for arg in args:
        # print "arg: %s" % arg
        values.append(parse(arg))
    # return values
    return sum(values)

def parse(arg):
    # If it is a number, return it, else continue.
    try:
        # print "value: %i" % int(arg)
        return int(arg)
    except ValueError:
        pass

    # If it is a predefined value, evoke recursion, else continue.
    try:
        # print "value: %s" % VALUES[arg]
        return main(str.split(str(VALUES[arg])))
    except KeyError:
        pass

    left = arg[:arg.find('d')]
    right = arg[arg.find('d')+1:]

    # Get die type.
    if is_int(right):
        die = DICE['default'](int(right))
    else:
        die = DICE[right]()

    # Get summer type.
    if is_int(left):
        summer, summer_args = SUMMERS['default'], left
    else:
        summer, summer_args = SUMMERS[left[0]], left[1:]

    # print str(summer) + ", " + str(die) + ", " + summer_args
    return summer(die, summer_args).sum()



if __name__ == '__main__':
    from sys import argv
    # print argv[1:]
    print main(argv[1:])

These are the three problems I see with my code:

• I practiced writing more professional documentation on die.py but did not spend the same effort on the other files. A little time would greatly improve them as they are not very explicit in their functionality. I'll accept any tips if my documentation style needs improvement.

• In previous attempts to write this application, I spent time writing and rewriting error handlers, and only made decent progress once I abandoned them altogether. They don't seem necessary, but I could use the exercise adding them where ever they might be most beneficial.

• The VALUES codeblock should be in a separate file. A user should be able to edit predefined values without looking at code. The easiest way would be to write another python file with the values defined there, then import it into the VALUES global.

In my own usage, I made a bash alias called dice that runs parser.py:

$ dice fudge 3
-1

Answer 1

from random import choice

class Side(dict):

You should almost never inherit from the builtin python collections. This is certainly not a case where you should. A side is not a kind of dictionary, it shouldn't try and inherit from dict

    """A side contains all info needed by a die and roller.

    >>> d = Side(4, 'death')
    >>> print d
    death

Is that really a useful way to print a Side?

    >>> print d.keys()
    ['char', 'num']

What possible use do you have for Side supporting a keys method?

    >>> print d.values()
    ['death', 4]
    >>>

    """

    def __init__(self, num, char):
        """
        Args:
            num (int) - Used to calculate value of roll.
            char (str) - Used to display face of roll.

        """

        self['num'] = num
        self['char'] = char

Use attributes, there is no reason to by using a dictionary here.

    def __repr__(self):
        return str(self['char'])

Repr should return something like Side(4, "death")

    __str__ = __repr__



class Die(list):

Again don't do that. A Die is not a list, don't pretend it is.

    """A die is a list of sides, all of equal probability."""

    def __init__(self, sides):
        for side in sides: self.append(side)

Use self.extend(sides), or better yet store the list as an attribute.

    def roll(self):
        return choice(self)

This class is pretty marginally useful, it raises the question of whether you actually need it.

class NSided(Die):
    """Returns an n-sided die.

    Args:
        numsides (int) - The highest value on the die.

    >>> d = NSided(6)
    >>> print d
    [1, 2, 3, 4, 5, 6]
    >>>

    """

    def __init__(self, numsides):
        sides = []
        for i in range(numsides):
            sides.append(Side(i+1, str(i+1)))
        Die.__init__(self, sides)



class Fudge(Die):
    """Returns a Fudge die.

    >>> d = Fudge()
    >>> print d
    [-,  , +]
    >>>

    """

    def __init__(self):
        sides = []
        sides.append(Side(-1, '-'))
        sides.append(Side(0, ' '))
        sides.append(Side(1, '+'))

Use a list literal, rather then several appends.

        Die.__init__(self, sides)

I'd make these functions which return Die rather then Die subclasses.

class Summer(object):

    def __init__(self, die, summer_args):
        """Sums a series of die rolls."""

        # (Die) object
        self.die = die
        # (int) dice of the same type, (dict)
        self.number_of_dice, self.args = self.parse_args(summer_args)

Don't use a self.args, just store arguments as attributes on the objects.

        self.roller = self.get_roller()


    def get_roller(self):
        """Returns a generator which yields die sides."""
        # print "self.number_of_dice" + ", " + str(type(self.number_of_dice)) + ", " + str(self.number_of_dice)
        return (self.die.roll() for i in range(self.number_of_dice))

I suggest

 for i in range(self.number_of_dice):
      yield self.die.roll()

I think it easier to follow

    def parse_args(self, arg):
        try:
            num_dice = int(arg)

You should really depend on the user of the class to do this, and not try to manipulate input here

        except ValueError:
            num_dice = 1

Don't do this. If somebody passes your class invalid data you should never never never try to replace it with something valid and carry on. You should probably just throw an exception here. return num_dice, {}

This would be way simpler to just do in the constructor.

    def sum(self):
        rolls = []
        while True:
            try:
                rolls.append(self.roller.next()['num'])
            except StopIteration:
                break

Use:

for roll in self.roller:
    rolls.append(roll['num'])

You should almost never need to deal with StopIteration directly.

Actually, you can even do:

rolls = [roll['num'] for roll in self.roller]

        return sum(rolls)



class Highest(Summer):

    def parse_args(self, arg):
        number_of_dice = int(arg[arg.find(',')+1:])
        number_to_count = int(arg[:arg.find(',')])

Again, parsing the input really doesn't fit here. That should happen in your parsing code.

        return number_of_dice, {'number_to_count': number_to_count}

This whole thing would still make more sense in a constructor.

    def sum(self):
        rolls = []
        while True:
            try:
                rolls.append(self.roller.next()['num'])
                if len(rolls) > self.args['number_to_count']: rolls.remove(min(rolls))

that's going to be inefficient

            except StopIteration:
                break
        return sum(rolls)

Do something like

rolls = [roll['num'] for roll in self.roller] rolls.sort(reverse = True) return sum(rolls[:self.number_to_count])

#!/usr/bin/python

import die as _die
import summer as _summer

Why are you importing them like that?

# Die and summer flags must be a single letter.  Lowercase 'd' is not valid.
DICE = {
    'default': _die.NSided,

Suspiciously useful, given 'default' isn't special to the dict class.

    'F': _die.Fudge,
}
SUMMERS = {
    'default': _summer.Summer,
    'H': _summer.Highest,
}

# Values must begin with a letter or underscore
VALUES = {

Bad name, call it PREDEFINED or something

    'ability': 'H3,4d6',
    'combat_advantage': 2,
    'difficult_terrain': -2,
    'fudge': '4dF',
}
# Synonyms...
VALUES['CA'] = VALUES['combat_advantage']
VALUES['DT'] = VALUES['difficult_terrain']


def is_int(arg):
    try:
        int(arg)
        return True

Put the last line in an else block

    except ValueError:
        return False


def main(args):
    values = []
    for arg in args:
        # print "arg: %s" % arg
        values.append(parse(arg))

Use values = map(parse, args)

    # return values
    return sum(values)

def parse(arg):
    # If it is a number, return it, else continue.
    try:
        # print "value: %i" % int(arg)
        return int(arg)
    except ValueError:
        pass

Duplicate of is_int...

    # If it is a predefined value, evoke recursion, else continue.
    try:
        # print "value: %s" % VALUES[arg]
        return main(str.split(str(VALUES[arg])))
    except KeyError:
        pass

    left = arg[:arg.find('d')]
    right = arg[arg.find('d')+1:]

    # Get die type.
    if is_int(right):
        die = DICE['default'](int(right))
    else:
        die = DICE[right]()

    # Get summer type.
    if is_int(left):
        summer, summer_args = SUMMERS['default'], left
    else:
        summer, summer_args = SUMMERS[left[0]], left[1:]

    # print str(summer) + ", " + str(die) + ", " + summer_args
    return summer(die, summer_args).sum()

I don't really like your parser. Sadly, I don't know the format well enough to suggest improvements. Haphazard string manipulation is difficult to follow and error prone. I'd look into regular expressions.

if __name__ == '__main__':
    from sys import argv
    # print argv[1:]
    print main(argv[1:])

Overall, your code is much more complicated then it needs to be, here is my approach:

import re
import random
import sys

# load the PREDEFINED from an external file
with open('predefined.txt') as predefined_file:
    PREDEFINED = dict(line.strip().split(' ', 1) for line in predefined_file)

# regular expression matches the various versions of rolls
ROLL_EXP = re.compile(r'(?:H(\d+),)?(\d+)d(\d+|F)')
#                        H(keep),(dice)d(sides)
def handle_roll(match):
    """
    Handle the rolls matched by ROLL_EXP
    """
    keep, dice, number = match.groups()

    # massage the data
    dice = int(dice)

    # if keep is None means that There is not (H3,) section
    if keep is None:
        keep = dice
    else:
        keep = int(keep)

    # F indicates a fudge die, 
    if number == 'F':
        roll = [random.randint(-1, 1) for x in xrange(dice)]
    else:
        roll = [random.randint(1, int(number)) for x in xrange(dice)]

    # sum up the remaining die
    roll.sort(reverse = True)
    return sum(roll[:keep])

# making the constant
CONSTANT = re.compile(r'-?\d+')
def handle_constant(match):
    # just use python's builtin int to understand the number
    return int(match.group(0))

# store a list of possible methods
METHODS = [
    (ROLL_EXP, handle_roll),
    (CONSTANT, handle_constant),
]

class BadRoll(Exception):
    """
    This exception is thrown to indicate a failure to understand what the user 
    specified
    """

def parse_roll(argument):
    """
    Given a basic roll, either return the summation or raise a BadRoll
    """
    for method_re, method_handler in METHODS:
        # try to match it
        match = method_re.match(argument)
        if match is not None:
            return method_handler(match) 
    else:
        raise BadRoll(argument)

def handle_argument(argument):
    """
    Return the summation of described rolls
    """
    # continually lookup up the argument in PREDEFINED
    # until we find nothing, then use parse_roll
    while True:
        try:
            argument = PREDEFINED[argument]
        except KeyError:
            return parse_roll(argument)




def main(args):
    try:
        # sum up the description of the arguments
        print sum(map(handle_argument, args[1:]))
    except BadRoll as error:
        # print what we didn't understand
        print >> sys.stderr, "Did not understand:", str(error)

if __name__ == '__main__':
    main(sys.argv)