Refactors for less nesting, slight performance and memory improvement for count command #46

Open
cordlesscoder wants to merge 1 commit from cordlesscoder/discord-bot:#46_Refactor into main
Contributor
No description provided.
cordlesscoder added 1 commit 2025-09-11 12:39:45 +00:00
Refactors for less nesting, slight performance and memory improvement for count command
All checks were successful
/ check_lfs (pull_request) Successful in 3s
/ lint_fmt (pull_request) Successful in 9s
/ lint_clippy (pull_request) Successful in 16s
/ build (pull_request) Successful in 1m19s
f3f08962a4
silver requested changes 2025-09-12 11:19:16 +00:00
silver left a comment
Owner

Overall pretty good, got some issues with some things but most looks good.

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(_) => {}
Owner

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.

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 {
Owner

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.

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);
Owner

This is good :3

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));
Owner

Also good!

Also good!
@ -46,3 +42,3 @@
// msg can be a max 2000 chars long
let mut limit = 2000 - 3;
let limit = 2000 - 3;
Owner

This can just be 2000 due to how ye are managing it.

This can just be 2000 due to how ye are managing it.
Author
Contributor

No it cannot?

No it cannot?
@ -60,2 +48,2 @@
limit -= length;
} else {
use std::fmt::Write;
let len_before = response.len();
Owner

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.

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();
Owner

Dont use the same var name for two different data types.
I know its possible but makes reading just the code hell.

Dont use the same var name for two different data types. I know its possible but makes reading just the code hell.
Author
Contributor

But this is the entire point of shadowing existing - it is absolutely idiomatic Rust.

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;
Owner

ye may as well import this far higher in the file

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();
Owner

Same thing about leaving comments here

Same thing about leaving comments here
@ -77,3 +70,1 @@
}
}
_ => None,
let x: i64 = row.try_get("discord").ok()?;
Owner

Pretty nice.
Though if ye are changing it ye may look into some slight changes such as:

  let x: u64 = row.try_get("discord").ok()?;
  if x == 0_u64 {
    // Default value, ID of 0 is not permitted
    return None;
  }
  Some(UserId::from(x))

Added the comment to make it clearer for folks in the future.

Pretty nice. Though if ye are changing it ye may look into some slight changes such as: ```rust let x: u64 = row.try_get("discord").ok()?; if x == 0_u64 { // Default value, ID of 0 is not permitted return None; } Some(UserId::from(x)) ``` Added the comment to make it clearer for folks in the future.
@ -78,4 +78,2 @@
.recv_json()
.await
{
Ok(res) => Some(res),
Owner

Another artifact left in from debugging, might be useful to leave as some services (this included) send a 200 for even a failure.

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(':') {
Owner

I strongly recommend doing anything with this file.
If ye want to change this make sure ye do a good job.

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<_>>()?;
Owner

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.

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(_) => {}
Owner

As I have outlined before, dont get rid of the match.
Consider it a hard learnt lesson in debugging.

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 {
Owner

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.

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();
Owner

Dont shadow var name.

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>(
Owner

Again with the match, there is a reason they are all on teh SQL sections.

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

What happened the ;?

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 {
Owner

In case ye were wondering the .get() was part of the migration from v0.11 to v0.12

https://github.com/serenity-rs/serenity/blob/current/CHANGELOG.md#ids

In case ye were wondering the ``.get()`` was part of the migration from v0.11 to v0.12 https://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 };
Owner

I get where ye are coming from, but this makes it less clear that its tied directly to teh role change.

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(
Owner

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.

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.
Author
Contributor

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

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
All checks were successful
/ check_lfs (pull_request) Successful in 3s
/ lint_fmt (pull_request) Successful in 9s
/ lint_clippy (pull_request) Successful in 16s
/ build (pull_request) Successful in 1m19s
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u #46_Refactor:cordlesscoder-#46_Refactor
git checkout cordlesscoder-#46_Refactor

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.

git checkout main
git merge --no-ff cordlesscoder-#46_Refactor
git checkout cordlesscoder-#46_Refactor
git rebase main
git checkout main
git merge --ff-only cordlesscoder-#46_Refactor
git checkout cordlesscoder-#46_Refactor
git rebase main
git checkout main
git merge --no-ff cordlesscoder-#46_Refactor
git checkout main
git merge --squash cordlesscoder-#46_Refactor
git checkout main
git merge --ff-only cordlesscoder-#46_Refactor
git checkout main
git merge cordlesscoder-#46_Refactor
git push origin main
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Skynet/discord-bot#46
No description provided.