Looking for feedback on my program design

Question

I'm aware that SO is for questions but overall the aim is to help people learn so I figured I'd try my hand at sharing some code and asking for feedback on it.

---

I'm looking to create a program that will rely on random numbers, specifically dice. These will be presented in the form of "2D6", "4D10+3", "2D2 + 3D3" and so on and so forth. I thus set out to create a dice roller module that would be able to accept input like in that form.

It works just fine for what's needed but has a bug for things that probably won't be needed (the docstring at the start of the file should explain). What I am interested in is what people think of my code and if anybody can see ways to improve it.

It is still WIP and I've not started on the unit tests yet.

Link to code

#!/usr/bin/env python3
"""
Created by Teifion Jordan
http://woarl.com

Notes: The roller does not correctly apply * and / signs:
A + B * C is worked out as (A + B) * C, not A + (B * C) as would be correct
"""

import random
import re
import math

class Roller_dict (object):
    """A 'dictionary' that stores rollers, if it's not got that roller it'll make a new one"""
    def __init__(self, generator=random.randint):
     super(Roller_dict, self).__init__()
     self.rollers = {}

     # Generator is used to supply a "rigged" random function for testing purposes
     self.generator = generator

    def __call__(self, constructor):
     constructor = constructor.replace(" ", "")
     if constructor not in self.rollers:
      self.rollers[constructor] = Roller(constructor, self.generator)

     return self.rollers[constructor]()

# Regular expressions used by the Roller class
# Compiled here to save time if we need to make lots of Roller objects
pattern_split    = re.compile(r"(\+|-|\*|/)")
pattern_constant    = re.compile(r"([0-9]*)")
pattern_die   = re.compile(r"([0-9]*)[Dd]([0-9]*)")
pattern_sign     = re.compile(r"^(\+|-|\*|/)")

class Roller (object):
    def __call__(self):
     return self.roll()

    def __init__(self, constructor, generator=random.randint):
     super(Roller, self).__init__()
     self.items = []
     self.rebuild(constructor)
     self.generator = generator

    def rebuild(self, constructor):
     """Builds the Roller from a new constructor string"""
     # First we need to split it up
     c = pattern_split.split(constructor.replace(" ", ""))

     # Check for exceptions
     if len(c) == 0:
      raise Exception('String "%s" did not produce any splits' % constructor)

     # Stitch signs back into their sections
     parts = []
     last_p = ""
     for p in c:
      if p in "+-*/":
       last_p = p
       continue

      if last_p != "":
       p = "%s%s" % (last_p, p)
       last_p = ""

      parts.append(p)

     # We have the parts, now we need to evaluate them into items
     for p in parts:
      # Look for a sign, default to positive
      sign = pattern_sign.search(p)
      if sign == None: sign = "+"
      else: sign = sign.groups()[0]

      # Strip out the sign, we're left with just the pure value
      body = p.replace(sign, "")

      # Now we find out what our main body is

      # Die
      value = pattern_die.search(body)
      if value != None:
       # Sign, Number, Sides
       self.items.append(("die", sign, int(value.groups()[0]), int(value.groups()[1])))
       continue

      # Constant
      value = pattern_constant.search(body)
      if value != None:
       self.items.append(("constant", sign, int(value.groups()[0])))
       continue

      # No matches
      raise Exception('The part string "%s" had no matches' % body)

    def roll(self):
     """Rolls the die/dice and returns the result"""
     result = 0

     for i in self.items:
      # Get value
      if i[0] == "die":   value = self._derive_die(i[2], i[3])
      elif i[0] == "constant": value = self._derive_constant(i[2])
      else: raise Exception('No handler for item type "%s"' % i[0])

      # Apply sign
      if i[1] == "+":  result += value
      elif i[1] == "-": result -= value
      elif i[1] == "*": result *= value
      elif i[1] == "/": result /= value

     return result

    def _derive_die(self, number, sides):
     result = 0
     for n in range(0, number):
      result += self.generator(0, sides)

     return result

    def _derive_constant(self, value):
     return value

# Useful for running the tests to make sure that it uses "random" numbers
false_numbers = (int(math.cos(x)*5)+5 for x in range(0,1000))
def false_numbers_func(*args):
    return false_numbers.next()

# If it's main, run unit tests?
if __name__ == '__main__':
    r = Roller_dict(false_numbers_func)

    print(r("2D6"))
    print(r("2D6"))
    print(r("2D6"))

Answer 1

Superficially, there's PEP08; in particular, the use of 4 spaces for an indent vs using tab characters.

It also seems like you have a lot of accidental complexity here, but I'd have to chew on it some more to understand. It seems like a simple enough idea that it shouldn't take as much effort to grok as it seems to be.

Answer 2

I think your approach makes for a lot of complexity: you're trying to solve the hard problem (parsing the input) at the same time you're solving the less-hard problem (doing the dice-rolling). It's easier if you separate the problems.

A class to roll dice is relatively easy to write. Two things I'm doing that you're not: the mapping of signs to operations (using a map means not having to write logic, plus it's reusable), and letting Roller objects be chained together in a simple linked list, so that calling roll on the head of the list rolls all of them and sums up the result.

import random
R = random.Random()

class Roller(object):
    # map signs to operations
    op = { "+" : lambda a,b: a+b,
           "-" : lambda a,b: a-b,
           "*" : lambda a,b: a*b,
           "/" : lambda a,b: a/b }

    def __init__(self, dice, sides, sign=None, modifier=0):
        self.dice = dice
        self.sides = sides
        self.sign = sign
        self.modifier = modifier
        self.next_sign = None
        self.next_roller = None

    def roll(self):
        self.dice_rolled = [R.randint(1, self.sides) for n in range(self.dice)]
        result = sum(dice_rolled)
        if self.sign:
            result = self.op[self.sign](result, self.modifier)
        if self.next_sign and self.next_roller:
            result = self.op[self.next_sign](result, self.next_roller.roll())
        return result

It's relatively easy to test that. Note that dice_rolled is saved as an attribute so that you can write unit tests more easily.

The next step is to figure out how to parse the input. This sort of works:

>>> p = """
(?P<next_sign>[-+*/])?
(?P<dice>[\d]+)
[\s]*D[\s]*
(?P<sides>[\d]+)
# trailing sign and modifier are optional, but if one is present both must be
([\s]*(?P<sign>[-+/*])[\s]*(?P<modifier>[\d]+))?"""
>>> r = re.compile(p, re.VERBOSE+re.IGNORECASE)
>>> m=r.match('2 d 20 +1')
>>> m.group('dice'), m.group('sides'), m.group('sign'), m.group('modifier')
('2', '20', '+', '1')
>>> r.findall('3D6*2-1D4+1*2D6-1')
[('', '3', '6', '*2', '*', '2'), ('-', '1', '4', '+1', '+', '1'), ('*', '2', '6', '-1', '-', '1')]

There's a lexical ambiguity that the syntax allows - 2D6+1D4 gets parsed as 2D6+1 followed by the unmatched D4, and it's not obvious to me how to fix that in the regular expression. Maybe that can be fixed with a negative lookahead assertion.

At any rate, once the regular expression gets fixed, the only thing left to do is process the results of r.findall to create a chain of Roller objects. And make that a class method if you really dig encapsulation.

Answer 3

The pyparsing examples page includes a similar dice expression parser and roller, including these test cases:

D5+2d6*3-5.5+4d6
D5+2d6*3-5.5+4d6.takeHighest(3)
2d6*3-5.5+4d6.minRoll(2).takeHighest(3)

The first 30 lines or so of the script contain the parser, the rest contains an evaluator, including debugging code showing the rolls being rolled.

I realize this is more a "silver platter" answer rather than feedback to your posted code - one thing in common with Robert Rossney's answer is the clear separation of parsing vs. rolling. Perhaps between this and Robert's sample you can glean some tidbits for your own dice roller.