Some rocket.rs code from a newbie

Hi everyone, first time here and about my first time touching rust language. To give you a bit of background I have mostly worked with PHP and Java, did some JS code and a bit of C++. I was for a long time interested in rust but I never had the guts to start till now and thought that a good way to get into it is to make something I was familiar with, like an API so I chose to use Rocket.rs.

My code is hosted here
hello_rust

I would like some feedback since I have no experience with Rust and maybe I've done some stupid stuff or at least not "in Rust manner". Or maybe code organization is off. Dunno.. Any feedback is appreciated.

One example I don't fully like is how I export modules, I tried to avoid long namespaces like domain::models::user::User to be like domain::models::User but still being able to separate code in multiple files.

1 Like

I wrote a lot but I didn't see any big problems in your code, you could honestly ignore all of this and be totally fine from what I saw.

You can use assert_eq!(x, y) instead of assert!(x == y).

I write my unit tests in the same file where the thing being tested is, but if you want a separate module for them, you can just make the whole module #[cfg(test)]:

#[cfg(test)]
mod testing;

instead of having to put it in each test file.

You can nest imports:
Instead of

use crate::domain::model::SessionData;
use crate::domain::model::User;
use crate::domain::repository::ResultSession;
use crate::domain::repository::ResultUser;
use crate::domain::repository::SessionError;
use crate::domain::repository::SessionRepository;
use crate::domain::repository::UserRegistrationError;

you can write

use crate::domain::{
    model::{SessionData, User},
    repository::{
        ResultSession, ResultUser, SessionError, SessionRepository, UserRegistrationError,
    },
};
use chrono::NaiveDateTime;
use mysql::{error::Error::MySqlError, params, prelude::Queryable, Pool};
use mysql_common::Row;

or whatever other scheme you prefer. Of course, preferring the explicit one line per import style is fine as well. You can enforce your preferred import formatting with rustfmt configuration options, though you will need to use the nightly toolchain to format with them (cargo +nightly fmt, you can probably configure your editor to do this on save; I've done so on vscode). I use

imports_granularity = "Crate"
group_imports = "One"

in all my projects, but that's just my personal preference.

I prefer using a library like diesel or sqlx for database stuff. Both will enable compile time checks for your queries to make maintaining your code easier, though they employ very different strategies to do so. I personally prefer diesel, but both have their pros and cons and sqlx is certainly easier to get started with. SeaORM is another option but I haven't tried it.

You can use a library like thiserror or anyhow to make the error handling more convenient. thiserror enables you to have an enum with variants for each error that can occur, whereas anyhow lets you turn every error into the library's error type and attach some error message with context. The common advice is to use thiserror for libraries (to allow consumers of the library to match on the error type and choose what to do about each kind of error) and anyhow for binaries (because it's often not important to know the exact underlying error type in an application).

The modules are a little too nested for my taste. For example, src/domain/repository/mod.rs is just

mod sessionrepository;

pub use sessionrepository::ResultSession;
pub use sessionrepository::ResultUser;
pub use sessionrepository::SessionError;
pub use sessionrepository::SessionRepository;
pub use sessionrepository::UserRegistrationError;

Instead of this, I would just have src/domain/sessionrepository.rs.

I don't think the #[serde(crate = "rocket::serde")] attributes are necessary.

There's some ownership related stuff that could be handled more idiomatically, for example

impl UserResponse {
    pub fn new(user: &User) -> Self {
        Self {
            name: user.name.to_owned(),
            email: user.email.to_owned(),
            id: user.id,
            created: user.created,
        }
    }
}

could take ownership of the argument to avoid having to clone the name and email, since you never need the user after converting it into a response

impl UserResponse {
    pub fn new(user: User) -> Self {
        Self {
            name: user.name,
            email: user.email,
            id: user.id,
            created: user.created,
        }
    }
}

but it's also totally fine as is. Ownership is one of the hardest parts of Rust to learn, and it's a good idea to use .clone() to make things work especially when starting out, rather than get stuck trying to micro-optimize things that probably won't matter. I just figured I'd give one example.

5 Likes

Wow,

First of all, thank you very much for your time and the complete feedback.

On the test side didn't know that you can just specify #[cfg(test)] on module level, just changed, much better.

For the imports I usually nest them together, or to be more precise VSCode does it... sometimes :smiley: , but I do prefer them nested too.

I usually don't like ORM for dealing with the database, for this simple example I think it would be fine, but I usually find myself limited by their lack of flexibility. Yet I say this from my previous experience with ORMs in other languages like PHP and Java, and IMHO they are not very good. I looked at diesel a bit, but it was just a glance, I will review both the options you mention maybe they are better than my previous experience.

For the error handling I will checkout anyhow and thiserror, they look pretty cool and I thought I was doing it cool by implementing From trait (first implementation was with map_err :sweat_smile: ).

On the borrow/ownership thing, well, I'm still getting used to. I don't think is very complicated but it will take some time to get used to it. I kinda get it, from what I understand from your example you pass ownership of the User to UserResponse::new, which will further pass ownership of the strings to the instance that it's creating. Since this is the last step in our code and we really don't need to work with User once the UserResponse is created we just pass it and forget instead of borrowing it with &. Right?

About the serde(crate = "rocket::serde"), I'll be honest that I'm not sure whats exactly the purpose, i've seen it in rocket documentation, but I can't get exactly why. There is an explanation about serde exports, but I don't get it.

Looks fine to me. Personally, I would avoid creating so many submodules in separate files, since it makes reading code harder. Rust doesn't impose any module structure on you, so you can style code in a way which you find most convenient: in a single module, in an inline submodule in the same file, or in separate files. Of course, if you already have a plan to fill out all those submodules, go for it. But if you don't, it's fine to have everything in a few reasonably sized files, and it's very easy to refactor stuff into separate modules later, if you use an IDE.

1 Like

I think your assesment of ORMs is correct, but you can use both diesel and sqlx as just a way to type-check your queries that doesn't abstract things away too much. I haven't checked out SeaORM despite hearing a lot of good things about it because it does appear to be a proper ORM which I also haven't had good experiences with.

Yeah, if you need ownership of a parameter inside of a function it's best to just take it as owned. That lets the caller clone the argument if they need it later and just give it away if they don't, whereas borrow -> clone will always perform the clone even when not necessary.

sqlx is expressly not an ORM.

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.