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?

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.

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.