5
\$\begingroup\$

I'm working on a simple dice roller for a Python IRC bot. The particular game this roller is for uses six different kinds of dice, which I've defined using a simple base class, and created six instances of it, passing a string array with the possible values of each dice.

class DiceBag(object):
    def __init__(self, dice: List[str]):
        self.bag = dice

    def draw(self, count: int) -> str:
        return [random.choice(self.bag) for _ in range(0, count)]

An example of the instances:

proficiency_dice = DiceBag(['', 'S', 'S', 'SS', 'SS', 'A', 'AS', 'AS', 'AS', 'AA', 'AA', '!'])
boost_dice = DiceBag(['', '', 'AA', 'A', 'SA', 'S'])

Now for a user to actually access this function, they must write a textual dice expression in the IRC channel. An example usage:

<user> ?roll 2p1b
<bot>  Result:  P(A, AA)  B(S)

So I need a way to quickly convert from the dice expression provided by the user, to call the draw method on the appropriate class.

I'm using a regular expression to evaluate the dice expression, so named capture groups seem like the most straightforward way to handle this - at the possible cost of my immortal soul, since I'm eval-ing the group name to the proper instance of DiceBag.

#User's input comes in as a string on the dice var
rex = (r'(?P<setback>\ds)?'
       r'(?P<ability>\da)?'
       r'(?P<difficulty>\dd)?'
       r'(?P<proficiency>\dp)?')

to_roll = re.match(rex, dice).groups()
    for group in to_roll:
        if group:  # We don't care about the Nones
            dicetype = group.split('')[1]
            dicecnt = group.split('')[0] # handle the extra rolls later
            eval(dicetype + "_dice" + ".draw(" + dicecnt + ")")

Is there a better, or saner, or more pythonic, or perhaps less evil way I could be handling this use case?

\$\endgroup\$
2
  • \$\begingroup\$ Are the values of proficiency_dice and boost_dice constant (and other dice types)? \$\endgroup\$
    – Zach Gates
    Apr 3, 2016 at 20:39
  • \$\begingroup\$ Yes - they are hard coded and will never change. \$\endgroup\$
    – Mikey T.K.
    Apr 3, 2016 at 20:40

3 Answers 3

6
\$\begingroup\$

Since you're converting user input into your own inner representation before evaluating, this is lesser evil. And sometimes this is the right tool for the job (inspect and pdb make good use of it).

But here, I'd go with simple dictionaries. It allows for automatically building the regexp and you don't really need the name of the capturing group since the dictionary already maps that information:

# Maps allowed characters to actual dice
DIE = {
    's': setback_dice,
    'd': difficulty_dice,
    'a': ability_dice,
    'p': proficiency_dice,
}

# Build the rules to parse user input
REX = ''.join(r'(\d{})?'.format(t) for t in DIE)

to_roll = re.match(REX, dice).groups()
for group in filter(None, to_roll):
    count, type_ = group
    DIE[type_].draw(int(count))

Note the use of filter to "not care about the Nones" and the uppercase variable names for constants.

Also:

  • you might want to wrap your code into functions to ease reusability and testing;
  • you might want to use re.compile to improve efficiency;
  • you might want to come up with a better regexp as this one is very dependant on the order it is written.
\$\endgroup\$
2
\$\begingroup\$
  1. Subclass built-in types; namely list. This eliminates the need for self.bag. By using *args, you can pass each roll rather than a list (see #2).

    class DiceBag(list):
    
        def __init__(self, *args):
            list.__init__(self, args)
    
        ...
    
  2. Instead of passing dice values on instance creation, hardcode your defaults to a class variable:

    class DiceBag(list):
        ...
    
    DiceBag.defaults = {
        "p": DiceBag('', 'S', 'S', 'SS', 'SS', 'A', 'AS', 'AS', 'AS', 'AA', 'AA', '!')
        "b": DiceBag('', '', 'AA', 'A', 'SA', 'S')
        # Other defaults
    }
    
  3. When using your regex, check to see if the message is a proper command:

    rex = "(\d[abdps])+?"
    

    This regex will check to see if the command is properly formatted, regardless of the order.

  4. Iterate across matches:

    if re.fullmatch(rex, dice):
        for (cnt, typ) in re.findall(rex, dice):
            DiceBag.defaults[typ].draw(int(cnt))
    

    If you know that every message will use the command properly, you can eliminate the if statement.

\$\endgroup\$
2
  • \$\begingroup\$ It would be slightly more efficient to use re.finditer() rather than re.findall(). Since you are just iterating anyway, the list created by re.findall() is unnecessary. \$\endgroup\$
    – zondo
    Apr 3, 2016 at 22:49
  • \$\begingroup\$ @zondo: finditer generates match objects. You would need to use the groups method to do anything with them; thereby creating a list (for each match, rather than just one). \$\endgroup\$
    – Zach Gates
    Apr 3, 2016 at 22:52
2
\$\begingroup\$

If ever you are attempted to use eval() to get a variable, you should be using a dictionary:

names = {
    's': setback_dice,
    'a': ability_dice,
    'd': difficulty_dice,
    'p': proficiency_dice,
}

for result in re.finditer(rex, dice):
    match = result.group()
    if match:
        number, dice_type = match
        name[dice_type].draw(number)

I changed re.match() to re.finditer() because re.match() finds only the first match.

\$\endgroup\$
5
  • \$\begingroup\$ Rather than if not match: continue, why not use if match: <code to execute>? It looks a lot cleaner. Also, you have the method group of result; it should be groups: result.groups(). \$\endgroup\$
    – Zach Gates
    Apr 3, 2016 at 23:15
  • \$\begingroup\$ @ZachGates: I'll edit for the if match:, but I used .group() because each match has just one group. It is unnecessary to use .groups(). \$\endgroup\$
    – zondo
    Apr 3, 2016 at 23:19
  • \$\begingroup\$ Then you need to specify which group you want. According to the documentation for group, m.group(1) will return the first group of the MatchObject named m. Note: group(0) will return the entire match (see examples in the docs). \$\endgroup\$
    – Zach Gates
    Apr 3, 2016 at 23:22
  • \$\begingroup\$ @ZachGates: I quote, Without arguments, group1 defaults to zero (the whole match is returned). \$\endgroup\$
    – zondo
    Apr 3, 2016 at 23:24
  • \$\begingroup\$ Ah, you're right. That's what I get for skimming. +1 \$\endgroup\$
    – Zach Gates
    Apr 3, 2016 at 23:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.