3
\$\begingroup\$

I have created a simple bot using Discord.py which outputs the contents of text files which contain scraped sports fixtures and channel data (scraped data is received from other small scripts I created but haven't included here).

discordbot.py

import discord
import os
from dotenv import load_dotenv


client = discord.Client()

load_dotenv()
TOKEN = os.getenv('DISCORD_TOKEN')
GUILD = os.getenv('DISCORD_GUILD')

client = discord.Client()

@client.event
async def on_ready():
    for guild in client.guilds:
        if guild.name == GUILD:
            break

    print(
        f'{client.user} is connected to the following guild:\n'
        f'{guild.name}(id: {guild.id})'
    )

                
@client.event
async def on_message(message):
    id = client.get_guild(73194****229982)
    

    if str(message.channel) == "boxingmma":
        if message.content.find("!boxing") != -1:

            with open('/home/brendan/Desktop/boxingtest.txt', 'r') as file:
                msg = file.read(2000).strip()
                while len(msg) > 0:
                    await message.author.send(msg)
                    msg = file.read(2000).strip()
                    
    if str(message.channel) == "football":
        if message.content.find("!english") != -1:

            with open('/home/brendan/Desktop/splittest.txt', 'r') as file:
                msg = file.read(2000).strip()
                while len(msg) > 0:
                    await message.author.send(msg)
                    msg = file.read(2000).strip()
                    
    if str(message.channel) == "football":
        if message.content.find("!scottish") != -1:

            with open('/home/brendan/Desktop/testing2.txt', 'r') as file:
                msg = file.read(2000).strip()
                while len(msg) > 0:
                    await message.author.send(msg)
                    msg = file.read(2000).strip()
            


client.run("NzMxOTQ4Mzk5NzY2NjY3NDg2.XwuPsw.iN****-e2yDnRS_uWqff43Thvqw")

The bot is running fine and when commands are run on the relevant discord channel a DM is sent to the user who sent the request to the bot. The biggest issue I have with this code is the repetitive code in the on_message function. The only difference in the three different double if statements is the channel name e.g. Football or Boxing, the command e.g. !scottish, !english, !boxing and the filepath to the scraped data.

I am curious if anyone knows of anyways this could be improved or a way for the repetitive code to be reused? I tried to pass these as positional arguments from another function, for example:

def callall():
    on_message(channel="football", command="!english", filepath="/home/brendan/Desktop/test.txt")

but it appears that after researching this it is not possible to pass arguments to on_message in discord.py

Any suggestions on how the repetitive code can be resued, or ways in which the general program flow can be improved would be much appreciated.

If this would be better over at Stackoverflow instead please let me know as well.

Thanks all.

\$\endgroup\$
2
\$\begingroup\$
@client.event
async def on_ready():
    for guild in client.guilds:
        if guild.name == GUILD:
            break

    print(
        f'{client.user} is connected to the following guild:\n'
        f'{guild.name}(id: {guild.id})'
    )

There is a bug if GUILD is not in the client guilds, as it will print out 'client.user' is connected to the last guild in the list. This is especially problematic if client.guilds is empty, as you'll end up with an exception due to never initialising 'guild'.

I would suggest a little more "defensive" code to check for this kind of problem, as good logging here could save you a headache later. It should be fine to do since this code will rarely be run.

@client.event
async def on_ready():
    def get_guild_by_name(guild_name):
        for guild in client.guilds:
            if guild.name == guild_name:
                return guild
    
    guild = get_guild_by_name()
    if not guild:
        // Code to handle this case. Log, print, throw, whatever you want it to do

        return

    print(
        f'{client.user} is connected to the following guild:\n'
        f'{guild.name}(id: {guild.id})'
    )

id = client.get_guild(73194****229982)

This is an example of a hardcoded value that should really be either in a config file, or assigned to a constant at the top of the file. If you were ever to change the guild for some reason, you'd have to track down all these occurrences and update them. Other places to look at are the hardcoded paths, and the token(!) at the bottom.


if str(message.channel) == "boxingmma":
    if message.content.find("!boxing") != -1:

        with open('/home/brendan/Desktop/boxingtest.txt', 'r') as file:
            msg = file.read(2000).strip()
            while len(msg) > 0:
                await message.author.send(msg)
                msg = file.read(2000).strip()

This block of code repeats a few times, with some minor differences. Since you may add more stats or need to make information available across multiple channels, I would think about how to make adding new commands to a channel as easy as possible. Ideally adding a new command is at most a line long, and obviously doesn't break any other commands.

I think splitting the above code into two functions, one that checks if the command is recognised (the matching) and one that does the work (retrieving the stats). The reasoning behind this separation is that the part of the above code that does the work doesn't change much, but the command matching does.

Some small things of note

  • x in y is a little more direct than y.find(x) != -1
  • I don't know why it reads the file in 2000 byte chunks, so I've left it alone. Is this really needed?
  • while len(msg) > 0: can be reduced to while msg: as containers like strings are truthy when they are not empty.
  • If you are on a recent enough version of Python, the walrus operator can be used here to remove some repeated code.
  • I like Path from pathlib, it has prevent a couple of errors I would have otherwise made.

The below code is for demonstration purposes, I haven't run it so I don't know if there are any issues. I also have defined the functions inline, but they would probably be better off outside on_message definition.

@client.event
async def on_message(message):
    id = client.get_guild(73194****229982)

    def matches_channel_command(message, expected_channel, expected_command):
        channel_name = str(message.channel)
        if channel_name != expected_channel:
            return False

        return expected_command in message.content

    async def retrieve_stats(path_):
        with open(path_, 'r') as file:
            msg = file.read(2000).strip()
            while msg:
                await message.author.send(msg)
                msg = file.read(2000).strip()

    if matches_channel_command(message, "boxingmma", "!boxing"):
        await retrieve_stats("/home/brendan/Desktop/boxingtest.txt")
    ...
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the fantastic review of the code. The reason for the 2000 limit is due to discord having a 2000 character limit per message. I am currently looking at a better way for instance. check the contents of text file between two separators so that a new message is outputted for each game rather than splitting strictly every 2000 characters. \$\endgroup\$ Jul 15 '20 at 23:30
  • 1
    \$\begingroup\$ @BrendanRodgers Thanks. That's interesting, TIL. \$\endgroup\$
    – spyr03
    Jul 16 '20 at 14:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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