Interactive Discord bot for tabletop RPG

Question

I'm new to Python, but have experience in a variety of other languages. I've learnt that a code review early on would make sure I don't start with bad habits. Which are more difficult to get out the longer you keep them.

This is a Discord bot for one of my hobby projects.

import os
import random
import discord
from discord.ext import commands
from dotenv import load_dotenv
load_dotenv()
token = os.getenv('DISCORD_TOKEN')
bot = commands.Bot(command_prefix='!')
@bot.command(name='ability', aliases=['a', 'ab'], help='Roll an ability check. Parameters: ability [no. of heroic dice].')
async def ability(ctx, ability:int, heroic:int=0):
 if ability < -5 or ability > 5:
 await ctx.send('Invalid ability value "{}". Must be between -5 and +5'.format(ability))
 return
 if heroic < 0 or heroic > 5:
 await ctx.send('Invalid heroic dice amount "{}". Must be between 0 and 5 (3 + 2 ritual dice)'.format(heroic))
 return
 dice = 2 + abs(ability) + heroic
 keep_dice = 2 + heroic
 if (ability < 0):
 keep = 'lowest'
 else:
 keep = 'highest'
 roll = []
 for x in range(0, dice):
 roll.append(roll_die())
 result = sorted(roll)
 if keep == 'highest':
 result.reverse()
 outcome = result[:keep_dice]
 # check for critical success (a pair) or failure (all 1s)
 last = -1
 critical = False
 fail = True
 for d in outcome:
 if d != 1:
 fail = False
 if d == last and d != 1:
 critical = True
 last = d
 if critical:
 # critical success - add the next highest/lowest die as well
 if keep_dice == dice:
 # roll an additional die
 outcome.append(roll_die())
 outcome.sort()
 else:
 outcome = result[:keep_dice+1]
 elif fail:
 outcome = []
 for x in range(0, keep_dice):
 outcome.append(0)
 # now sum up all our dice for the total
 sum = 0
 for d in outcome:
 sum += d
 embed = discord.Embed(title='**Dragon Eye** Ability Test for {}'.format(ctx.author.name),
 description='Rolling {}d10, keep {} {}'.format(dice, keep, keep_dice),
 color=0x22a7f0)
 embed.add_field(name='Roll', value=format(roll), inline=False)
 embed.add_field(name='Outcome', value=format(outcome), inline=True)
 embed.add_field(name='Total', value=format(sum), inline=True)
 if critical:
 embed.add_field(name='Additional Info', value='**Critical Success**', inline=True)
 elif fail:
 embed.add_field(name='Additional Info', value='**Critical Fail**', inline=True)
 await ctx.send(embed=embed)
def roll_die(sides=10):
 die = random.randint(1, sides)
 return die
bot.run(token)

Answer 1

List Comprehension

Instead of

roll = []
for x in range(0, dice):
 roll.append(roll_die())

do this

roll = [roll_die() for _ in range(0, dice)]

The result is the same, it requires less code, and more pythonic. This also utilizes the _. The underscore is meant to show that the variable in the loop isn't used. This applies to other places in your code as well.

Ternary Operator

Instead of

if (ability < 0):
 keep = 'lowest'
else:
 keep = 'highest'

do this

keep = 'lowest' if ability < 0 else 'highest'

Using a ternary operator can keep you from writing more code, and looks a lot nicer. In this case, it converts four lines into one, which is a big plus.

Direct Returns

You don't have to assign a value inside a function before returning it. In this case, you don't need a dice variable inside roll_die(). You can simply return the call to random.randint. Take a look:

def roll_die(sides=10):
 return random.randint(1, sides)

Using sum

Instead of

sum = 0
for d in outcome:
 sum += d

use pythons built-in sum function

dice_sum = sum(outcome)

The sum functions returns the sum of all the numbers in an iterable. So, you can pass the list used in the loop to that function. It does the same thing you do, but it's already written.

• Tom: Thanks, all of these are great comments.
• BlueRaja - Danny Pflughoeft: Isn't d for d in outcome the same as outcome?
• Ben A: @BlueRaja-DannyPflughoeft That's an excellent point. I missed that while I was reviewing. The answer has been edited.

Answer 2

Compact Comparisons

Instead of

if ability < -5 or ability > 5:

try

if not -5 <= ability <= 5:

I prefer these kinds of comparisons when defining acceptable bounds of inputs because you get a firm definition of the upper and lower bound without having to jump around to see where the variable is relative to the number and where the sign is pointing.

Cool Counter

last = -1
critical = False
fail = True
for d in outcome:
 if d != 1:
 fail = False
 if d == last and d != 1:
 critical = True
 last = d

There's a lot of code here, and it's not particularly readable. In the built in collections module, there's a class called Counter, you can use it to replace this code like this:

from collections import Counter
# ... 
counts = Counter(outcome)
# check for critical success (a pair) or failure (all 1s)
fail = counts[1] == keep_dice
critical = not fail and any([ count >= 2 for _, count in counts.items() ])

If you use this Counter cleverly, you can avoid having to keep making sure that outcomes is sorted and reversed properly.

Cromulent Class

I'm not so overzealous as to apply OOP where it isn't needed, but the fact that you forgot to reverse outcome here makes me wonder if there's a legitimate need for a class that wraps your dice rolls.

 if critical:
 # critical success - add the next highest/lowest die as well
 if keep_dice == dice:
 # roll an additional die
 outcome.append(roll_die())
 outcome.sort()

from random import randint
from collections import Counter
def roll_die(sides=10):
 return randint(1, sides)
class DiceRoll:
 """Keeps track of which numbers have been rolled and which will be kept"""
 def __init__(self, total: int):
 self.dice = [ roll_die() for i in range(total) ]
 self.keep(0)
 """Change the number of dice that are being kept in this roll
 count: how many dice to keep
 highs: whether or not to keep the highest dice."""
 def keep(self, count: int, highs: bool = True):
 # roll any extra die if necessary
 self.dice += [ roll_die() for i in range(count - len(self.dice)) ]
 self.kept = Counter(sorted(self.dice, reverse = highs)[:count])
 """Returns a Boolean indicating if all rolls kept herein are of the same digit.
 The digit can be supplied as the parameter, otherwise True is returned if
 the kept rolls consist of only one digit regardless of what that digit is.
 If no dice have been rolled or kept, False is returned.
 """
 def keeps_only(self, d: int = None):
 least_one = [ k for k, count in self.kept.items() if count != 0 ]
 return len(least_one) == 1 and (least_one[0] is d or d is None)
 """Returns a Boolean indicating if at least two rolls of the same digit are kept herein.
 The digit can be supplied as the parameter, otherwise the Boolean returned indicates if
 *any* digit is repeated at least once.
 """
 def keeps_pair(self, d: int = None):
 return any(count >= 2 and (n is d or d is None) for n, count in self.kept.items())
 """Returns the number of dice that aren't being discarded in this DiceRoll"""
 def kept_count(self):
 return len(self.kept_list())
 """Returns a list of all of the dice as currently kept in the DiceRoll"""
 def kept_list(self):
 return list(self.kept.elements())
 """Sets the results of all of the rolls (kept or not) to the provided digit."""
 def set_all(self, digit: int):
 self.dice = [digit] * len(self.dice)
 self.keep(self.kept_count())
# input
ability, heroic = [ int(input(a + ': ')) for a in ['ability', 'heroic'] ]
if not -5 <= ability <= 5:
 print(f'Invalid ability value "{ability}". Must be between -5 and +5')
if not 0 <= heroic <= 5:
 print(f'Invalid heroic dice amount "{heroic}". Must be between 0 and 5 (3 + 2 ritual dice)')
# basic roll config
total = 2 + abs(ability) + heroic
keep_count = 2 + heroic
keep_highs = ability >= 0
# make the roll
roll = DiceRoll(total)
roll.keep(keep_count, keep_highs)
# info now for printing later
raw = roll.dice.copy();
info = ''
# special effects
if roll.keeps_only(1):
 roll.set_all(0)
 info = "Failure"
elif roll.keeps_pair():
 roll.keep(keep_count + 1, keep_highs)
 info = "Success"
print(f"""
Rolling {total}d10, keep {'highest' if keep_highs else 'lowest'} {keep_count}
Roll: {format(raw)}
Outcome: {format(roll.kept_list())}
Total: {sum(roll.kept_list())}
{f'Additional Info: **Critical {info}**' if info else ''}
""")

Answer 3

@Linny already told you about list comprehensions. His suggestion would currently lead to the following code:

roll = [roll_die() for _ in range(0, dice)]
result = sorted(roll)
if keep == 'highest':
 result.reverse()

Looking at the documentation of sorted(...), reveals that is also accepts an argument called reversed. Using that extra parameter allows the code piece above to be rewritten as

result = sorted(roll, reverse=(keep == 'highest'))

---

Another neat list-trick is that you can use outcome = [0, ] * keep_dice to generate a list of a given size with the same elements at all spots. However, be warned of the caveats.

---

Depending on which Python versions the code should run on, maybe also have a look at the new f-string formatting. Using them allows code like

embed = discord.Embed(title='**Dragon Eye** Ability Test for {}'.format(ctx.author.name),
 description='Rolling {}d10, keep {} {}'.format(dice, keep, keep_dice),
 color=0x22a7f0)

to be rewritten as

embed = discord.Embed(title=f'**Dragon Eye** Ability Test for {ctx.author.name}',
 description=f'Rolling {dice}d10, keep {keep} {keep_dice}',
 color=0x22a7f0)

f-strings are very powerful and also allow more complex expressions to be used:

embed.add_field(
 name='Additional Info',
 value=f'**Critical {"Success" if critical else "Fail"}**',
 inline=True
)

---

The string flags 'highest'/'lowest' could be replaced with an Enum. IDEs with autocompletion can help with them and therefore it's easier to avoid typos. However, this will slightly increase the effort needed to print them nicely for the user. In this special case a simple bool variable would also work, since you only have two possible values. The enum as well as the bool version could then use a ternary expression for printing if you want to follow this route.

• Tom: Thank you. Those are good hints as well. I saw the f-strings, but didn't understand what they are for. Now I do. :-)