Die Roller for command line

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

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?

```
#

StackExchange Code Review Q#19305, answer score: 7