Refactors for less nesting, slight performance and memory improvement for count command #46
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "cordlesscoder/discord-bot:#46_Refactor"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Overall pretty good, got some issues with some things but most looks good.
@ -73,4 +56,2 @@
};
match add_server(&db, ctx, &server_data).await {
Ok(_) => {}
This is more of a personal experience thing but I used to use more
if let Err(
/if let Ok(
but often found myself having to change it to a match to be able to add a capture for the other option to be able to debug why something was succeeding, but not correctly.It is useful to keep around SQL queries I find.
@ -12,3 +12,2 @@
pub async fn run(command: &CommandInteraction, ctx: &Context) -> String {
let sub_options = if let Some(CommandDataOption {
value: CommandDataOptionValue::SubCommand(options),
let Some(CommandDataOption {
I am saying no to this style.
While it is technically possible, readability falls off a cliff.
I had to check out teh file locally to be able to get some understanding, and even then it was only because my IDE was hilighting stuff that it was possible to see the flow.
Cool that it can be done that way, but it means that it has a higher chance to be either abandoned or ripped out in the future.
@ -27,2 +24,2 @@
false
};
})
.unwrap_or(false);
This is good :3
@ -43,3 +40,2 @@
cs.sort_by_key(|(count, _)| *count);
cs.reverse();
cs.sort_by_key(|(count, _)| std::cmp::Reverse(*count));
Also good!
@ -46,3 +42,3 @@
// msg can be a max 2000 chars long
let mut limit = 2000 - 3;
let limit = 2000 - 3;
This can just be 2000 due to how ye are managing it.
No it cannot?
@ -60,2 +48,2 @@
limit -= length;
} else {
use std::fmt::Write;
let len_before = response.len();
Whenever ye are doing something "clever" ye need to add a fuckton of comments.
So please add them to explain it.
This is nice though, far better than what I had before.
@ -106,2 +93,2 @@
}
}
let committees = get_committees(&db).await.into_iter().flatten().map(|committee| (committee.id, committee));
let committees: HashMap<_, _> = committees.collect();
Dont use the same var name for two different data types.
I know its possible but makes reading just the code hell.
But this is the entire point of shadowing existing - it is absolutely idiomatic Rust.
@ -155,3 +121,1 @@
response.push(line);
limit -= length;
} else {
use std::fmt::Write;
ye may as well import this far higher in the file
@ -156,2 +121,2 @@
limit -= length;
} else {
use std::fmt::Write;
let len_before = response.len();
Same thing about leaving comments here
@ -77,3 +70,1 @@
}
}
_ => None,
let x: i64 = row.try_get("discord").ok()?;
Pretty nice.
Though if ye are changing it ye may look into some slight changes such as:
Added the comment to make it clearer for folks in the future.
@ -78,4 +78,2 @@
.recv_json()
.await
{
Ok(res) => Some(res),
Another artifact left in from debugging, might be useful to leave as some services (this included) send a 200 for even a failure.
@ -61,3 +57,1 @@
.split(',')
.map(|s| {
let s = s.split(':').collect::<Vec<&str>>();
this.colors = if args.colors.contains(':') {
I strongly recommend doing anything with this file.
If ye want to change this make sure ye do a good job.
@ -78,3 +74,1 @@
colors.push(c);
}
.collect::<std::io::Result<_>>()?;
Just a personal thing (wont reject over it) but not too much of a fan of
<_>
, makes it harder to read the code and follow through.Its great if ye have an IDE which will populate it, not so great if ye dont have teh IDE open.
@ -393,4 +372,2 @@
.fetch_optional(db)
.await
{
Ok(_) => {}
As I have outlined before, dont get rid of the match.
Consider it a hard learnt lesson in debugging.
@ -254,3 +245,1 @@
continue;
}
Some(x) => x.to_owned(),
let Some(name) = path_local2.file_name().map(ToOwned::to_owned) else {
This is grand for using
let Some()
.The ones up in
add_servers
were far more complex and so not suited to using this format.@ -319,2 +304,2 @@
}
}
let name_lowercase = logo.name.to_ascii_lowercase();
let name_lowercase = name_lowercase.to_str().unwrap_or_default();
Dont shadow var name.
@ -24,7 +24,7 @@ struct WolvesResultUserMin {
}
async fn add_users_wolves(db: &Pool<Sqlite>, user: &WolvesResultUserMin) {
// expiry
match sqlx::query_as::<_, Wolves>(
Again with the match, there is a reason they are all on teh SQL sections.
@ -122,2 +120,2 @@
if let Ok(x) = x.trim().parse::<u64>() {
config.compsoc_server = GuildId::new(x);
if let Ok(x) = x.trim().parse() {
config.compsoc_server = GuildId::new(x)
What happened the
;
?@ -55,3 +55,3 @@
// committee server takes priority
let committee_server = config_global.committee_server;
if new_member.guild_id.get() == committee_server.get() {
if new_member.guild_id == committee_server {
In case ye were wondering the
.get()
was part of the migration from v0.11 to v0.12https://github.com/serenity-rs/serenity/blob/current/CHANGELOG.md#ids
@ -116,3 +115,1 @@
if let Some(x) = new_data {
on_role_change(&db, &ctx, x).await;
}
let Some(x) = new_data else { return };
I get where ye are coming from, but this makes it less clear that its tied directly to teh role change.
@ -129,3 +127,3 @@
let config = config_lock.read().await;
match Command::set_global_commands(
if let Err(e) = Command::set_global_commands(
ye really dont like matching.
In this case would be good to keep it since sometimes the error is in the Ok branch, when stuff goes wrong, but not badly.
Could you selectively merge whatever changes you're fine with?
I'd like to address the ones I didn't get right in separate, smaller PRs
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.