6
\$\begingroup\$

I've made a Discord bot, and (to my knowledge) the code is pretty good. However, I've mostly taught myself Python and discord.py (the discord library for Python), so I was wondering if there's any improvements to be made.

The only thing I can discern right now is that I could probably do with a few more comments, but other than that I'm not really sure.

https://github.com/Sinel-Servers-Limited/JoyBot

The 'on topic' rules state that I can post external links, but the most important part of the code has to be on the site. I would consider this my most important part:

# JoyBot/cogs/bump.py

    @commands.Cog.listener()
    async def on_message(self, message):
        try:
            if Ban(message.guild.id).is_banned():
                return
        except AttributeError:
            return

        if Counting(message.guild.id).channel_get_id() == message.channel.id:
            return

        firstbump = False
        if message.author.id == config['DISBOARDID']:
            for embed in message.embeds:
                if ":thumbsup:" in embed.description:
                    bumpID = await get_id().member(embed.description)
                    bump = Bmp(message.guild.id, bumpID)
                    oldtop = bump.get_top(raw=True)

                    if oldtop is None:
                        firstbump = True
                        oldtop = 0

                    bump.add_total()
                    newtop = bump.get_top(raw=True)

                    send_msg = f"<@{bumpID}>, your bump total has been increased by one!\nType `.bumptotal` to view your current bump total!"
                    if not firstbump:
                        if oldtop != newtop:
                            send_msg += "\nYou also managed to get the top spot! Nice!"
                            send_msg += f"\n\n<@{oldtop}>, you've lost your top spot!"

                    else:
                        send_msg += "\n\nYou were also the first to bump the server. Congrats!"

                    await message.channel.send(send_msg)

This code tracks the amount of times that a person who 'bumps' the server on Disboard. I consider this important because it's the main feature of my bot, and the one I've spent the most time on.

\$\endgroup\$
6
\$\begingroup\$

Decorators

The problem to solve

The top of your code should be moved into another function as you have duplicated the code elsewhere. To DRY your code we can build a decorator. To explain decorators I'll start from two made up function and work up.

In our world negative numbers are not allowed, if we get a negative number we return. We have two functions one to double numbers and another to add 1 to a number.

def double(number):
    if number < 0:
        return
    return 2 * number

def add_one(number):
    if number < 0:
        return
    return 1 + number

Now having the same code in two functions is a code smell. If the spec changes, you need to modify your code at least twice. If you forget even one location you've introduced a bug into your code! For example not handling None may be an oversight, where we actually need to.

We can introduce a function to check if the number is valid. But this isn't exactly helpful here, because all we've done is hide number < 0 behind a function. And we still have the same if ...: return boilerplate.

def is_positive(number):
    return 0 <= number

def double(number):
    if not is_positive(number):
        return
    return 2 * number

Functions as first class citizens

Instead we can pass a function to is_positive to be called if the number is positive. There is the downside that if we don't call our functions in a convoluted way we don't actually validate the input.

def is_positive(fn, number):
    if number < 0:
        return
    return fn(number)

def double(number):
    return 2 * number

def add_one(number):
    return 1 + number

print(is_positive(double, 2))
print(is_positive(double, -1))
print(double(-1))
print(is_positive(add_one, 2))
4
None
-2  # Oops, should be None
3

Closures to the rescue

To always call is_positive before calling; double and add_one, we can use a closure. The closure will take in a function (double or add_one) and return a new function (what is_positive currently is).

If we re-assign double to the newly made function the issue is solved.

def is_positive(fn):
    def inner(number):
        if number < 0:
            return
        return fn(number)
    return inner

def double(number):
    return 2 * number
    
print(is_positive(double)(2))
print(is_positive(double)(-1))
print(double(-1))  # Still has the issue

double = is_positive(double)  # Fix the issue

print(double(2))
print(double(-1))
4
None
-2  # Look the issue
4
None  # Now the issue is fixed

Decorator syntactic sugar

I find double = is_positive(double) to be quite ugly. However Python has @ syntax to decorate functions. @ is the same as using double = is_positive(double).

def is_positive(fn):
    def inner(number):
        if number < 0:
            return
        return fn(number)
    return inner

@is_positive
def double(number):
    return 2 * number

print(double(2))
print(double(-1))
4
None

Common wrapper patterns

Before selecting how to apply decorators to your code, we need to look at ways we can provide arguments to the decorator.

  1. No wrapper

    Pro: No extra parentheses and really simple implementation.
    Con: No option for positional or keyword arguments.

    import functools
    
    def is_positive(fn):
        @functools.wraps(fn)
        def inner(*args, **kwargs):
            # ...
        return inner
    
    @is_positive
    def double(number):
        return 2 * number
    
    @is_positive
    def add_one(number):
        return 1 * number
    
  2. Closure wrapper

    Pro: Allow positional and keyword arguments.
    Con: Have to always use parentheses even when not providing arguments.

    import functools
    
    def is_positive(include_zero=True):
        def wrapper(fn):
            @functools.wraps(fn)
            def inner(*args, **kwargs):
                # ...
            return inner
        return wrapper
    
    @is_positive(False)
    def double(number):
        return 2 * number
    
    @is_positive()
    def add_one(number):
        return 1 * number
    
  3. Partial wrapper

    Pro: Don't have to provide parentheses when not providing arguments.
    Con: Only allows keyword arguments.

    import functools
    
    def is_positive(fn=None, *, include_zero=True):
        if fn is None:
            return functools.partial(include_zero=include_zero)
    
        @functools.wraps(fn)
        def inner(*args, **kwargs):
            # ...
        return inner
    
    @is_positive(include_zero=False)
    def double(number):
        return 2 * number
    
    @is_positive
    def add_one(number):
        return 1 * number
    

Which you should pick can be a bit complicated. The basics are;

  • If you don't need to provide arguments; use 1.
  • If you are upgrading from 1, and you need to maintain backwards compatibility; use 3.
  • Otherwise pick whichever you prefer between 2 and 3.

For the most part picking between 2 and 3 is like picking between Coke and Pepsi. Pick the one that tickles your fancy more. I prefer 3. I normally don't need positional arguments. I think forcing arguments to be keywords is a benefit. And I don't like having to provide unneeded parentheses. However 2 is simpler and I think the more common approach. For example we can guess commands.Cog.listener is using 2.

Your code

How should you decorate

Note: Code from here is untested

To decorate your function we can go one of four ways.

  1. Your method should be a static method. You never use self in the code, so we can decorate with @staticmethod first.

    Problem: You won't be able to decorate a non-static method.

  2. The user should specify the argument's index where message is.

    Problem: The user of the function may forget to specify the correct index. The code now can be kind of noisy, if we haven't selected a good default.

  3. You could make a method-aware decorator, possibly making a descriptor, and assume message is the first argument.

    Problem: We'd need to convert to a class decorator and would force message to be the first argument. Probably would be annoying to get correctly working with static and class methods.

  4. You can inspect the function's signature to get the index of the argument named "message".

    Similar to how dependency injection injects the correct stuff. However we wouldn't be injecting dependencies and we'd be going off names not types.

    Problem: You must name the argument "message".

The best option is really your choice. I quite like 4, but DI isn't really a thing in Python. Never mind untyped half-baked DIesque patterns. Regardless I'll show you how to implement 2. 2 is probably the best general way to solve the issue. Not too clever and works in most situations.

Since we are going by index we can, to simplify the code, only take args.

def validate_message(index=0):
    def wrapper(fn):
        @functools.wraps(fn)
        async def inner(*args):
            message = args[index]
            try:
                if Ban(message.guild.id).is_banned():
                    return
            except AttributeError:
                return

            if Counting(message.guild.id).channel_get_id() == message.channel.id:
                return
            return await fn(*args)
        return inner
    return wrapper
@commands.Cog.listener()
@validate_message(index=1)
async def on_message(self, message):
    firstbump = False
    # ...

Allowing keyword arguments and default values

You may need to allow keyword arguments or default values. To do so we should inspect the wrapped function and bind the arguments. Your code will then be one step away from implementing 4.

import inspect


def validate_message(index=0):
    def wrapper(fn):
        @functools.wraps(fn)
        async def inner(*args, **kwargs):
            _args = signature.bind(*args, **kwargs)
            message = _args.arguments.get(parameter.name, parameter.default)
            if message is inspect.Parameter.empty:
                raise ValueError("a message must be provided")
            # ...
            return await fn(*_args.args, **_args.kwargs)

        signature = inspect.signature(fn)
        parameter = list(signature.parameters.values())[index]
        return inner
    return wrapper

Review

  • Your code follows the arrow anti-pattern. You can use guard statements to stop your code from moving more and more rightwards.

    if message.author.id != config["DISBOARDID"]:
        return
    # ...
    
  • Your style isn't consistent, some places you use ' other places you use ". Same with the case of variables firstbump, bumpID and send_msg.

    Have you taken and modified parts of your code from code on the internet? Have you followed the licence(s) if you've taken and modified other people's IP?

  • Strings are not guaranteed to have fast concatenation. With your code I can't really see the need though. However you can build a list and "".join to guarantee fast concatenation of strings in all spec compliant environments.

    To my eye the code is now less pretty here.

  • If you have more than one embed in one message, where one is the first bump; your code will say all embeds are the first bump.

  • The rest of the code looks like boilerplate code around get_id and Bmp. My answer is long enough, so I'll leave me review here.

@commands.Cog.listener()
@validate_message(index=1)
async def on_message(self, message):
    if message.author.id != config['DISBOARDID']:
        return
    for embed in message.embeds:
        if ":thumbsup:" not in embed.description:
            continue

        bump_id = await get_id().member(embed.description)
        bump = Bmp(message.guild.id, bump_id)
        old_top = bump.get_top(raw=True)
        _old_top = old_top or 0
        bump.add_total()
        new_top = bump.get_top(raw=True)

        msg = [f"<@{bump_id}>, your bump total has been increased by one!\nType `.bumptotal` to view your current bump total!"]
        if old_top is None:
            msg.append("\n\nYou were also the first to bump the server. Congrats!")
        elif _old_top != _new_top:
            msg.append(
                "\nYou also managed to get the top spot! Nice!"
                f"\n\n<@{_old_top}>, you've lost your top spot!"
            )

        await message.channel.send("".join(msg))
\$\endgroup\$

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.