Skip to content

Commit

Permalink
dice: eliminate near-duplicate regex matching of dice expressions
Browse files Browse the repository at this point in the history
Matching on the dice expression pattern only once is more efficient, and
it simplifies the code.

Passing a dice expression `re.Match` to `_roll_dice()` eliminates any
need for the pesky `InvalidDiceExpressionError` case that was impossible
to cover using only the example decorator (and the `if result is None`
check was a recent addition to help the type-checker, rather than an
actual runtime requirement).

I only wish that there was an equivalent to `re.findall()` that returns
a *list* of `re.Match` objects instead of an iterator, but the list
comprehension required to use `re.finditer()` isn't *that* bad.
  • Loading branch information
dgw committed Oct 28, 2023
1 parent f59886b commit 315cc7a
Showing 1 changed file with 17 additions and 35 deletions.
52 changes: 17 additions & 35 deletions sopel/modules/dice.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,6 @@ def __init__(self, *args):
super().__init__(*args)


class InvalidDiceExpressionError(DiceError):
"""Custom exception type for invalid dice expressions."""
def __init__(self, expression: str):
super().__init__(expression)

@property
def expression(self) -> str:
return self.args[0]


class InvalidDiceFacesError(DiceError):
"""Custom exception type for invalid number of die faces."""
def __init__(self, faces: int):
Expand Down Expand Up @@ -182,8 +172,6 @@ def total(self) -> int:


def _get_error_message(exc: DiceError) -> str:
if isinstance(exc, InvalidDiceExpressionError):
return "Invalid dice expression: {}".format(exc.expression)
if isinstance(exc, InvalidDiceFacesError):
return "I don't have any dice with {} sides.".format(exc.faces)
if isinstance(exc, NegativeDiceCountError):
Expand All @@ -197,22 +185,9 @@ def _get_error_message(exc: DiceError) -> str:
return "Unknown error rolling dice: %r" % exc


def _roll_dice(dice_expression: str) -> DicePouch:
result = re.search(
r"""
(?P<dice_num>-?\d*)
d
(?P<dice_type>-?\d+)
(v(?P<drop_lowest>-?\d+))?
$""",
dice_expression,
re.IGNORECASE | re.VERBOSE)

if result is None:
raise InvalidDiceExpressionError(dice_expression)

dice_num = int(result.group('dice_num') or 1)
dice_type = int(result.group('dice_type'))
def _roll_dice(dice_match: re.Match[str]) -> DicePouch:
dice_num = int(dice_match.group('dice_num') or 1)
dice_type = int(dice_match.group('dice_type'))

# Dice can't have zero or a negative number of sides.
if dice_type <= 0:
Expand All @@ -230,8 +205,8 @@ def _roll_dice(dice_expression: str) -> DicePouch:

dice = DicePouch(dice_num, dice_type)

if result.group('drop_lowest'):
drop = int(result.group('drop_lowest'))
if dice_match.group('drop_lowest'):
drop = int(dice_match.group('drop_lowest'))
if drop >= 0:
dice.drop_lowest(drop)
else:
Expand Down Expand Up @@ -279,9 +254,12 @@ def roll(bot: SopelWrapper, trigger: Trigger):
number of lowest dice to be dropped from the result. N is the constant to
be applied to the end result. Comment is for easily noting the purpose.
"""
# This regexp is only allowed to have one capture group, because having
# more would alter the output of re.findall.
dice_regexp = r"-?\d*[dD]-?\d+(?:[vV]-?\d+)?"
dice_regexp = r"""
(?P<dice_num>-?\d*)
d
(?P<dice_type>-?\d+)
(v(?P<drop_lowest>-?\d+))?
"""

# Get a list of all dice expressions, evaluate them and then replace the
# expressions in the original string with the results. Replacing is done
Expand All @@ -291,9 +269,13 @@ def roll(bot: SopelWrapper, trigger: Trigger):
return

arg_str_raw = trigger.group(2).split("#", 1)[0].strip()
dice_expressions = re.findall(dice_regexp, arg_str_raw)
arg_str = arg_str_raw.replace("%", "%%")
arg_str = re.sub(dice_regexp, "%s", arg_str)
arg_str = re.sub(dice_regexp, "%s", arg_str, 0, re.IGNORECASE | re.VERBOSE)

dice_expressions = [
match for match in
re.finditer(dice_regexp, arg_str_raw, re.IGNORECASE | re.VERBOSE)
]

if not dice_expressions:
bot.reply("I couldn't find any valid dice expressions.")
Expand Down

0 comments on commit 315cc7a

Please sign in to comment.