Eval is evil: Dynamic method calls from named regex groups in Python 3

Question

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?

• Zach Gates: Are the values of proficiency_dice and boost_dice constant (and other dice types)?
• Mikey T.K.: Yes - they are hard coded and will never change.

Answer 1

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.

Answer 2

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.

• zondo: 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.
• Zach Gates: @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).

Answer 3

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.

• Zach Gates: 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().
• zondo: @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().
• Zach Gates: 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).
• zondo: @ZachGates: I quote, Without arguments, group1 defaults to zero (the whole match is returned).
• Zach Gates: Ah, you're right. That's what I get for skimming. +1