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.
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
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
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.
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.
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.
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.
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))
