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

Ready for review #1

Merged
merged 1 commit into from
Apr 14, 2024
Merged

Ready for review #1

merged 1 commit into from
Apr 14, 2024

Conversation

khomiakmaxim
Copy link
Owner

@khomiakmaxim khomiakmaxim commented Apr 14, 2024

Telegram bot, which works similarly to Splitwise app. Create groups, add members, so that all of them can track their expenses and see overal debt state in the group.

  • Register bot with telegram BotFather,
  • Retrieve bot token
  • Place it while running cargo r -- --token "<YOUR TELEGRAM BOT TOKEN>"

Written with Rust, Teloxide and SeaORM.

image
image

Copy link

@andrewalbrecht05 andrewalbrecht05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This project looks good to me 👍
The code is well structured and easy to understand.
Also, i like the idea of this bot, seems quite useful


// How much many was spent overall
let sum: Decimal = user_spent.iter().map(|x| *x.1).sum();
assert!(!user_spent.is_empty());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line can cause a program to panic, it seems you wanted to do debug_assert

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewalbrecht05, this assert! won't panic, since I check for expenses_in_group to be non-empty above, and I use that collection to populate user_spent HashMap, so it shouldn't be empty. assert! here is used in order to show, why the line below, where I divide sum by the user_spent.len() won't panic as well. By the way, thank you very much for a review. It was a crucial part to get a feedback from the Rust forum, in order to pass this assignment, so you helped me a lot.

Copy link

@trepachkoDm trepachkoDm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "rust_splittea_bot" project shows a well-structured and comprehensive approach to building a bot with features for managing expenses in groups.

I liked its modular design, full CLI customization, and database integration.
Great job!

Copy link

@vsmysle vsmysle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on the code! I have a couple of comments:

  • It would be great to have tests covering the database interactions.
  • The logging could be improved, especially with adding tracing calls.
  • The commands appear to be lengthy and somewhat challenging to type (e.g /addmembertgroup or /listexpensesingroup). It might be beneficial to explore refactoring them with subcommands (if it is possible with the teloxide and Telegram).

@khomiakmaxim khomiakmaxim merged commit 6cd6483 into main Apr 14, 2024
@khomiakmaxim
Copy link
Owner Author

Nice job on the code! I have a couple of comments:

  • It would be great to have tests covering the database interactions.
  • The logging could be improved, especially with adding tracing calls.
  • The commands appear to be lengthy and somewhat challenging to type (e.g /addmembertgroup or /listexpensesingroup). It might be beneficial to explore refactoring them with subcommands (if it is possible with the teloxide and Telegram).

Thanks, that indeed would be a good thing to do.

@khomiakmaxim
Copy link
Owner Author

The "rust_splittea_bot" project shows a well-structured and comprehensive approach to building a bot with features for managing expenses in groups.

I liked its modular design, full CLI customization, and database integration. Great job!

Thanks, Dima!!

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

Successfully merging this pull request may close these issues.

5 participants