The code is bad

[pikaninja]

Creating a table for every guild is a horrible horrible idea, on top of the fact that you only should have one aiosqlite connection at a time.

[pikaninja]

that's just one issue, why tf is there a dm all command, on top of being API abusive, ANYONE CAN USE THIS

[pikaninja]

you should not be doing so much code in on_ready, as it may get called multiple times in a bot's runtime, should create an async task instead

[kaylynnnn]

I just found this repository and have found numerous rookie mistakes. You're creating a new table every single time the client joins a new guild which if you learn anything about Database's you know this is just a bad idea in general. There's general bad things going on in the code itself as well such as checking the equality of a argument to None which just isn't it chief. Also formatted strings could lead to SQL injections within SQL queries. e.g.

SELECT * FROM {member.display_name};

That is a very niché case but it's an example of a terrible statement with a format string in Python.
In general this code is just plain bad and you should probably consider re-writing it.

[alphascriptyt]

Creating a table for every guild is a horrible horrible idea, on top of the fact that you only should have one aiosqlite connection at a time.

Thank you for letting me know this, I didn't realise about the tables, I'd seen that the number of tables didn't matter. I will add the guild_id as another key and have every entry in one table, would that be better? For only having one connection at a time, that makes a lot of sense now, I'd thought that because I was closing the database it was fine, however, I completely overlooked the fact that if two people use the command at the same time when updating, it could cause problems. So, should I simply connect at the start of the program, and then close it when the program finishes? How would I make sure it closed as the code would not finish if I simply stopped it from running? Thanks.

[alphascriptyt]

that's just one issue, why tf is there a dm all command, on top of being API abusive, ANYONE CAN USE THIS

It was made for a youtube tutorial, why would I have a bunch of random commands together in one bot like this? This bot is not a real bot, its just a collection of code from my series.

[alphascriptyt]

I just found this repository and have found numerous rookie mistakes. You're creating a new table every single time the client joins a new guild which if you learn anything about Database's you know this is just a bad idea in general. There's general bad things going on in the code itself as well such as checking the equality of a argument to None which just isn't it chief. Also formatted strings could lead to SQL injections within SQL queries. e.g.

SELECT * FROM {member.display_name};

That is a very niché case but it's an example of a terrible statement with a format string in Python.
In general this code is just plain bad and you should probably consider re-writing it.

First of all, thanks for the criticism, not sure you wrote it in the nicest or most constructive way but I will take it. The tables issue has already been brought up and to be honest, it just made sense to me to have the database split into a table for each guild as it removed the need for another field for the guild id, please could you tell me why it is a bad thing, or is it the dynamic creation of them?
I don't understand what you're saying that checking if an argument is None is a bad thing? None is returned when the convertor fails to find the given type and in multiple cases I tell the user this? I've seen multiple people compare values to None, so please let me know what is wrong with this.
Finally, in the video I made on this I stated that the reason for using the string formatting was simply because what I was passing through cannot change, I know what SQL injections are and the risks of them, however, like I said, the input cannot change to something malicious. This issue has brought to my attention that the reason you can't pass through variables for table names is probably because you're not supposed to dynamically create them, so thanks for that.

[alphascriptyt]

you should not be doing so much code in on_ready, as it may get called multiple times in a bot's runtime, should create an async task instead

Thank you, I've managed to forget how the bot will disconnect and reconnect overtime. Should this be a background task? Or how would I go about this as I only want to do it when the bot starts but if I do it before the on_ready event fires, it won't work as bot.guilds won't be ready etc?

[alphascriptyt]

Once I have received your feedback I will rewrite parts.
I saw you guys also joined my discord and then left, I would like to ask, kal-byte, why was the code in #code-share like that, it seemed so overcomplicated, but maybe that was the point or I'm missing something.

[pikaninja]

Creating a table for every guild is a horrible horrible idea, on top of the fact that you only should have one aiosqlite connection at a time.

Thank you for letting me know this, I didn't realise about the tables, I'd seen that the number of tables didn't matter. I will add the guild_id as another key and have every entry in one table, would that be better? For only having one connection at a time, that makes a lot of sense now, I'd thought that because I was closing the database it was fine, however, I completely overlooked the fact that if two people use the command at the same time when updating, it could cause problems. So, should I simply connect at the start of the program, and then close it when the program finishes? How would I make sure it closed as the code would not finish if I simply stopped it from running? Thanks.

yes, most of these solutions are good, regarding how to close it when the program is done, you can subclass the commands.Bot, and override async def close to first close the db before using super().close

[pikaninja]

you should not be doing so much code in on_ready, as it may get called multiple times in a bot's runtime, should create an async task instead

Thank you, I've managed to forget how the bot will disconnect and reconnect overtime. Should this be a background task? Or how would I go about this as I only want to do it when the bot starts but if I do it before the on_ready event fires, it won't work as bot.guilds won't be ready etc?

something such as a bot.loop.create_task, and you can use wait_until_ready at the top

[kaylynnnn]

Yeah pretty much everything Pika said, you should have a single table for guilds in your db e.g. named called guilds and you could have it like:

Columns:
id - prefix
Rows:
guild id here - prefix

Unsure if you're aware of constraints etc
so guild would also be a primary key as there should only be one entry for it in the table

[pikaninja]

Just a general comment involving table schemas, while you have a limited number of columns and tables, you do however have a pretty much unlimited amount of rows, which you should take advantage of.

[pikaninja]

I just found this repository and have found numerous rookie mistakes. You're creating a new table every single time the client joins a new guild which if you learn anything about Database's you know this is just a bad idea in general. There's general bad things going on in the code itself as well such as checking the equality of a argument to None which just isn't it chief. Also formatted strings could lead to SQL injections within SQL queries. e.g.

SELECT * FROM {member.display_name};

That is a very niché case but it's an example of a terrible statement with a format string in Python.
In general this code is just plain bad and you should probably consider re-writing it.

First of all, thanks for the criticism, not sure you wrote it in the nicest or most constructive way but I will take it. The tables issue has already been brought up and to be honest, it just made sense to me to have the database split into a table for each guild as it removed the need for another field for the guild id, please could you tell me why it is a bad thing, or is it the dynamic creation of them?
I don't understand what you're saying that checking if an argument is None is a bad thing? None is returned when the convertor fails to find the given type and in multiple cases I tell the user this? I've seen multiple people compare values to None, so please let me know what is wrong with this.
Finally, in the video I made on this I stated that the reason for using the string formatting was simply because what I was passing through cannot change, I know what SQL injections are and the risks of them, however, like I said, the input cannot change to something malicious. This issue has brought to my attention that the reason you can't pass through variables for table names is probably because you're not supposed to dynamically create them, so thanks for that.

Regarding injections, even if its not possible to become malicious, it's still better practice to use the library sanitization. It should also be noted that as this is a tutorial, people will read this and use it for other projects, which may have problems with injections.
Regarding checking against None, while it works, it's not a good practice. For things like bools and None, using is is a better practice as == relies on the classes __eq__ which may not check for None and such.
As for why it is bad for having many tables, this is not how SQL was built, having many tables is not good, while having many rows is fine.

[pikaninja]

Slight another comment, you have

# THIS IS FOR PEOPLE WHO USE CLIENT INSTEAD OF client

inside the client version, shouldn't this be instead of bot? unless I'm misunderstanding

[alphascriptyt]

Creating a table for every guild is a horrible horrible idea, on top of the fact that you only should have one aiosqlite connection at a time.

Thank you for letting me know this, I didn't realise about the tables, I'd seen that the number of tables didn't matter. I will add the guild_id as another key and have every entry in one table, would that be better? For only having one connection at a time, that makes a lot of sense now, I'd thought that because I was closing the database it was fine, however, I completely overlooked the fact that if two people use the command at the same time when updating, it could cause problems. So, should I simply connect at the start of the program, and then close it when the program finishes? How would I make sure it closed as the code would not finish if I simply stopped it from running? Thanks.

yes, most of these solutions are good, regarding how to close it when the program is done, you can subclass the commands.Bot, and override async def close to first close the db before using super().close

Okay, thank you, and that will work if the program somehow crashed?

[alphascriptyt]

you should not be doing so much code in on_ready, as it may get called multiple times in a bot's runtime, should create an async task instead

Thank you, I've managed to forget how the bot will disconnect and reconnect overtime. Should this be a background task? Or how would I go about this as I only want to do it when the bot starts but if I do it before the on_ready event fires, it won't work as bot.guilds won't be ready etc?

something such as a bot.loop.create_task, and you can use wait_until_ready at the top

Thought so, I've used the event loop before in other programs, thanks again.

[alphascriptyt]

Yeah pretty much everything Pika said, you should have a single table for guilds in your db e.g. named called guilds and you could have it like:

Columns:
id - prefix
Rows:
guild id here - prefix

Unsure if you're aware of constraints etc
so guild would also be a primary key as there should only be one entry for it in the table

Thank you, I am aware, in my case, it would be something like this for the rows though right as I would need to be searching for both guild and user id as they would appear multiple times in the table?
GUILD_ID | USER_ID | EXP

Or, would I concatenate them and use that as the primary key, like this, so it would be a string that I would search for?
GUILD_ID+USER_ID | EXP

I believe the later option would work better as then I'm not searching for two ids?

[alphascriptyt]

I just found this repository and have found numerous rookie mistakes. You're creating a new table every single time the client joins a new guild which if you learn anything about Database's you know this is just a bad idea in general. There's general bad things going on in the code itself as well such as checking the equality of a argument to None which just isn't it chief. Also formatted strings could lead to SQL injections within SQL queries. e.g.

SELECT * FROM {member.display_name};

That is a very niché case but it's an example of a terrible statement with a format string in Python.
In general this code is just plain bad and you should probably consider re-writing it.

First of all, thanks for the criticism, not sure you wrote it in the nicest or most constructive way but I will take it. The tables issue has already been brought up and to be honest, it just made sense to me to have the database split into a table for each guild as it removed the need for another field for the guild id, please could you tell me why it is a bad thing, or is it the dynamic creation of them?
I don't understand what you're saying that checking if an argument is None is a bad thing? None is returned when the convertor fails to find the given type and in multiple cases I tell the user this? I've seen multiple people compare values to None, so please let me know what is wrong with this.
Finally, in the video I made on this I stated that the reason for using the string formatting was simply because what I was passing through cannot change, I know what SQL injections are and the risks of them, however, like I said, the input cannot change to something malicious. This issue has brought to my attention that the reason you can't pass through variables for table names is probably because you're not supposed to dynamically create them, so thanks for that.

Regarding injections, even if its not possible to become malicious, it's still better practice to use the library sanitization. It should also be noted that as this is a tutorial, people will read this and use it for other projects, which may have problems with injections.
Regarding checking against None, while it works, it's not a good practice. For things like bools and None, using is is a better practice as == relies on the classes __eq__ which may not check for None and such.
As for why it is bad for having many tables, this is not how SQL was built, having many tables is not good, while having many rows is fine.

Yes, I completely agree, thanks, I made sure to mention that but will most likely be remaking the video and updating it in response to this issue that you created.

In regards to the checking against None, I've just noticed that I used '==' to compare against it once, which is my mistake and I've used 'is' to compare them, for a long time, for your exact reason as I've researched this is the past. I was confused at what kal-byte mentioned because I know that using '==' is wrong but I presumed that he was referencing me using 'is', I will correct that, thank you.

[alphascriptyt]

Slight another comment, you have

# THIS IS FOR PEOPLE WHO USE CLIENT INSTEAD OF client

inside the client version, shouldn't this be instead of bot? unless I'm misunderstanding

Yes that's an oversight on my part, I used the replace function to replace every instance of 'bot' with 'client' and clearly missed that comment. I will update that, thanks.

[kaylynnnn]

Well for separate users you may want to create a separate table for this maybe the primary key just being a SERIAL? and then the guild id can reference the guild in the guilds table with 2 other columns them being user_id and their current xp

[pikaninja]

Yeah pretty much everything Pika said, you should have a single table for guilds in your db e.g. named called guilds and you could have it like:
Columns:
id - prefix
Rows:
guild id here - prefix
Unsure if you're aware of constraints etc
so guild would also be a primary key as there should only be one entry for it in the table

Thank you, I am aware, in my case, it would be something like this for the rows though right as I would need to be searching for both guild and user id as they would appear multiple times in the table?
GUILD_ID | USER_ID | EXP

Or, would I concatenate them and use that as the primary key, like this, so it would be a string that I would search for?
GUILD_ID+USER_ID | EXP

I believe the later option would work better as then I'm not searching for two ids?

its possible that guild_id+user_id would work, however this is less clean.

[pikaninja]

Creating a table for every guild is a horrible horrible idea, on top of the fact that you only should have one aiosqlite connection at a time.

Thank you for letting me know this, I didn't realise about the tables, I'd seen that the number of tables didn't matter. I will add the guild_id as another key and have every entry in one table, would that be better? For only having one connection at a time, that makes a lot of sense now, I'd thought that because I was closing the database it was fine, however, I completely overlooked the fact that if two people use the command at the same time when updating, it could cause problems. So, should I simply connect at the start of the program, and then close it when the program finishes? How would I make sure it closed as the code would not finish if I simply stopped it from running? Thanks.

yes, most of these solutions are good, regarding how to close it when the program is done, you can subclass the commands.Bot, and override async def close to first close the db before using super().close

Okay, thank you, and that will work if the program somehow crashed?

if something such as the host crashing happens, it will not work, but not closing the connection shouldn't be a big problem anyways.

[alphascriptyt]

Well for separate users you may want to create a separate table for this maybe the primary key just being a SERIAL? and then the guild id can reference the guild in the guilds table with 2 other columns them being user_id and their current xp

I'm not sure, what you mean by SERIAL, please could you clarify?
Okay, so you would say that having searching for the two keys separately would work best? Thanks.

[alphascriptyt]

Yeah pretty much everything Pika said, you should have a single table for guilds in your db e.g. named called guilds and you could have it like:
Columns:
id - prefix
Rows:
guild id here - prefix
Unsure if you're aware of constraints etc
so guild would also be a primary key as there should only be one entry for it in the table

Thank you, I am aware, in my case, it would be something like this for the rows though right as I would need to be searching for both guild and user id as they would appear multiple times in the table?
GUILD_ID | USER_ID | EXP
Or, would I concatenate them and use that as the primary key, like this, so it would be a string that I would search for?
GUILD_ID+USER_ID | EXP
I believe the later option would work better as then I'm not searching for two ids?

its possible that guild_id+user_id would work, however this is less clean.

Okay, thanks, so I should opt for having the two keys, guild_id and user_id?

[alphascriptyt]

Creating a table for every guild is a horrible horrible idea, on top of the fact that you only should have one aiosqlite connection at a time.

Thank you for letting me know this, I didn't realise about the tables, I'd seen that the number of tables didn't matter. I will add the guild_id as another key and have every entry in one table, would that be better? For only having one connection at a time, that makes a lot of sense now, I'd thought that because I was closing the database it was fine, however, I completely overlooked the fact that if two people use the command at the same time when updating, it could cause problems. So, should I simply connect at the start of the program, and then close it when the program finishes? How would I make sure it closed as the code would not finish if I simply stopped it from running? Thanks.

yes, most of these solutions are good, regarding how to close it when the program is done, you can subclass the commands.Bot, and override async def close to first close the db before using super().close

Okay, thank you, and that will work if the program somehow crashed?

if something such as the host crashing happens, it will not work, but not closing the connection shouldn't be a big problem anyways.

Yeah, that makes sense, thanks again.

[pikaninja]

Yeah pretty much everything Pika said, you should have a single table for guilds in your db e.g. named called guilds and you could have it like:
Columns:
id - prefix
Rows:
guild id here - prefix
Unsure if you're aware of constraints etc
so guild would also be a primary key as there should only be one entry for it in the table

Thank you, I am aware, in my case, it would be something like this for the rows though right as I would need to be searching for both guild and user id as they would appear multiple times in the table?
GUILD_ID | USER_ID | EXP
Or, would I concatenate them and use that as the primary key, like this, so it would be a string that I would search for?
GUILD_ID+USER_ID | EXP
I believe the later option would work better as then I'm not searching for two ids?

its possible that guild_id+user_id would work, however this is less clean.

Okay, thanks, so I should opt for having the two keys, guild_id and user_id?

yeah I would think this is more clear, especially since this is for a tutorial

[kaylynnnn]

SERIAL is basically auto-increment so once you insert a row it'll go up by 1
so e.g. you may have like:

1 - one field here - second field here
2 - one field here - second field here
3 - one field here - second field here
4 - one field here - second field here

So say you were to delete the 3rd one this will be the end result:

1 - one field here - second field here
2 - one field here - second field here
4 - one field here - second field here

Note how 4 doesn't change, if you entered another row it'll be 5 rather than 3 or another number.

[alphascriptyt]

SERIAL is basically auto-increment so once you insert a row it'll go up by 1
so e.g. you may have like:

1 - one field here - second field here
2 - one field here - second field here
3 - one field here - second field here
4 - one field here - second field here

So say you were to delete the 3rd one this will be the end result:

1 - one field here - second field here
2 - one field here - second field here
4 - one field here - second field here

Note how 4 doesn't change, if you entered another row it'll be 5 rather than 3 or another number.

Got it, thanks.

[alphascriptyt]

Yeah pretty much everything Pika said, you should have a single table for guilds in your db e.g. named called guilds and you could have it like:
Columns:
id - prefix
Rows:
guild id here - prefix
Unsure if you're aware of constraints etc
so guild would also be a primary key as there should only be one entry for it in the table

Thank you, I am aware, in my case, it would be something like this for the rows though right as I would need to be searching for both guild and user id as they would appear multiple times in the table?
GUILD_ID | USER_ID | EXP
Or, would I concatenate them and use that as the primary key, like this, so it would be a string that I would search for?
GUILD_ID+USER_ID | EXP
I believe the later option would work better as then I'm not searching for two ids?

its possible that guild_id+user_id would work, however this is less clean.

Okay, thanks, so I should opt for having the two keys, guild_id and user_id?

yeah I would think this is more clear, especially since this is for a tutorial

Sounds good, I'll redo it all, thank you for your constructive criticism.