Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A couple more code reviews #6

Open
pikaninja opened this issue Apr 30, 2021 · 3 comments
Open

A couple more code reviews #6

pikaninja opened this issue Apr 30, 2021 · 3 comments

Comments

@pikaninja
Copy link

pikaninja commented Apr 30, 2021

Hey, don't know if you remember me from the last time I criticized your code, but I have a few more comments.

This is all for the invite tracking code.

First off, I would like to say that this was a large improvement from last time, but there are a few issues

The way you compare datetimes is bad.

strftiming then splitting is just- not needed, datetimes can be subtracted
for example

time = member.created_at
now = datetime.datetime.utcnow()
diff = now - time # this creates a timedelta object
seconds = diff.total_seconds() # the seconds in difference

if seconds < 604800: # the number of seconds in a week
    # new member

pep-8

The line lengths here are a bit long, especially the sql statements, you can split it like:

query = """
SELECT (a, b)
FROM TABLE
WHERE  a = ?
AND b = ?
"""
objects = (1, 2)
await bot.db.execute(query, objects)

Logic Issues

Discord does not dispatch invite_delete for expired invites, which will be an issue for this. You might want a task.loop to continuously poll the invites

Other comments

  1. in line 42, you might as well commit in the update_totals itself
  2. In the event that a bot using this gets large, executing into the database so often may create issues. You might want to just cache all the invites, as in this case you're querying all of them on startup anyways
  3. It may be good to implement docstrings in your functions, to explain what they are doing
@pikaninja
Copy link
Author

Additionally, episode 13's paginator should be done with a d.py extension menus: here this is an official extension for creating paginators

@alphascriptyt
Copy link
Owner

Hi, I do remember you. Thank you.
For the datetimes, that is very true and I do realise that I overcomplicated it rather than using its operators.

In terms of pep-8 I agree they are a bit long and it would've been clearer to split them up, it was just never that important to me. I'll start to do this though, thanks.

For the invite_delete issue, that's a good point, I never knew that, thank you.

Other Comments:

  1. My mistake, I must've overlooked this.
  2. That is very true, I hope that someone doesn't use this in a large bot, its more to show the logic, but yes that would be much more efficient.
  3. Again with pep-8, I agree, however, I think the function names explain themselves pretty well but I will start doing this out of good practise.

@alphascriptyt
Copy link
Owner

Finally for the paginators, I do know of that but I wanted to show some logic behind it. I do agree that it would be better though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants