Request for review and suggestion

code review
1

Hi everyone, I’m new and trying to learn Rust. I’m currently building a collaborative sketch app in Rust, and for now my goal is to store everything in memory. I just wanted to get started with Rust and learn by doing.

Here’s my repo: GitHub - abhishek-shivale/sketch · GitHub

I’d really appreciate any feedback—what I might be doing wrong and what I could improve.

Thanks for your help!

2

Hi,

Welcome, hope you're enjoying learning rust. I found it one of the most enjoyable languages to learn and work with and disagree whole-heartedly that it's hard to learn - the documentation and community are both amazing!

That's a huge codebase but seeing as no-one else has handed any feedback yet I'll provide a few high-level pointers. Most of this is 80% opinion and 20% hard-rules (even if it sounds like a 20%-80% split at times :wink:) ...

  • First off, I'd be really interested (genuinely, I spend a lot of time thinking about how different people code) as to how you managed to build something so big without any tests.
  • For apps that go beyond "trivial" (for your personal definition of trivial), it's usually considered a good idea to put most of the logic in a library (for simpler cases centered around lib.rs in the same crate) and let main.rs handle: startup, call functions from the lib, shutdown. For example:
    • in your tokio loop in main: either the whole creation of work could be moved to a dedicated type, or the logic inside the filter_map could be moved out to a separate function. Depending on whether loop: let work = h_lock.iter().filter_map(cleanup_dead_events).collect(); or loop: let Work = Work::new(h_lock) better convey the meaning of your code. Doing this may also give you insights into other ways to flatten the nesting and make main easier to read
  • both rooms.rs & utils.rs look like they could benefit from being consolidated around structured types with impls. Utils looks like a collection of "things" (enums & structs) while rooms is a load of functions but no "Room" to hold it all together.
  • The sort of shape that is idiomatic for rust uses common traits (such as (Try)From, ? to propogate errors) to encapsulate chunks of logic and help reduce nesting and complexity in your main functions. Having something a bit like this (manipulated by hand from room.rs)
    impl TryFrom<&str> for MessageEvent {
    type Error = ...;
    fn try_from(msg: &str) -> Self {
        {
            let parsed = serde_json::from_str::<Data>(&data)?;
            match &parsed.key {
                GlobalEvents::Disconnected => {
                    clean_up(key, &state).await;
                 }
                 GlobalEvents::Message => {
                      if let Some(data) = parsed.value.clone() {
                          data.events
                      } else { ... }
                 }
...
    would let you lose a load of nested logic which is just parsing message data to get the event and make interact() read something like:
    match msg {
    Ok(message::Text(data)) => {
        let event = MessageEvent::try_from(&data)?;
        match event {
...

Do those idea help as pointers for things to look at?

Implementing them will probably force you to consider how you are holding and mutating global state (rethinking that would be a good thing in my view) and how you handle errors (judicious use of ?, functions that return a Result and then either recover/retry at a suitable point or handle them in or main() is a good pattern)

1 Like
3

thanks, i will improve on this