Code review for small-ish CLI application

code review
1

Hey friends!

I've been using Rust for about 6 months now. I feel like I've got a good grasp of the basics but would love to get some feedback on my latest project: ReadStor.

Any feedback is welcome but I'll try to focus it a bit.

  1. Application structure is always difficult for me. I looked at a few other repos to see how they did it and tried to mimic some aspects of them. But still not sure how much I should put into main or should I make an App struct that handles args or should I just make a bunch of separate functions.

  2. I come from writing Python so there's a lot of OOP stuff I've had to drop but I'm not quite sure I'm there. Is there anything that screams OOP?

  3. Handling of public/private is a new concept for me as Python doesn't really have this besides the use of _ or __.

  4. Application configuration is the biggest mystery of them all. I've tried finding resources on the best way to do this but I'm not sure if I'm looking in the right places. A lot of what I've read has to do with the object itself (global mutable, global immutable, concrete, Traits) but not how to use them in an application properly. I don't have a background in CS and if I did I'd still be wondering how things "should" be done in Rust.

Anyways, any feedback would be amazing!

Thanks so much! :slight_smile:

1 Like
2

You say that you're relatively new to Rust but from what I can see you're quite well along. It's nice and readable, and I find it well organized.

One thing I did notice is a number of functions returning Result which have no error variant, ie fn foo() -> LibResult<()> and then the last statement returning the unit type, ie Ok(()). Generally speaking you only need Result of the function may return an error. Otherwise you can omit the return value entirely and do away with the Ok(()).

As for configuration, I never saw anything approaching a language level blessed way of doing things. I usually have a Config struct, and if the application is something long lived I might use serde to write it to disk in toml format. There's a few crates which will give you a turnkey configuration system, but I think they're overkill.

Oo wise, you can freely bring your own choosing style into Rust, nobody ever said that Rust can't incorporate Oo concepts. The compiler will tell you when you're fighting against it. After a while, the language itself will change your approach kind of organically.

3

Thanks so much for taking the time to look @jeang3nie! :smiley:

You say that you're relatively new to Rust but from what I can see you're quite well along. It's nice and readable, and I find it well organized.

Thanks! :smiley: That's a relief. When I read articles on rust half the time they go over my head and I wonder "Am I doing this right??"

One thing I did notice is a number of functions returning Result which have no error variant, ie fn foo() -> LibResult<()> and then the last statement returning the unit type, ie Ok(()) . Generally speaking you only need Result of the function may return an error. Otherwise you can omit the return value entirely and do away with the Ok(()).

I'll look into this! I might have missed these during a refactor!

As for configuration, I never saw anything approaching a language level blessed way of doing things. I usually have a Config struct, and if the application is something long lived I might use serde to write it to disk in toml format. There's a few crates which will give you a turnkey configuration system, but I think they're overkill.

I think the thing that I'm most unsure about is if it's okay to litter every function signature with &Config. It makes testing way easier cause I can test different configurations really quickly but it feels like the function signature could be simpler. I've never worked on a very large application so I don't have enough experience to have have intuition about this.

Oo wise, you can freely bring your own choosing style into Rust, nobody ever said that Rust can't incorporate Oo concepts. The compiler will tell you when you're fighting against it. After a while, the language itself will change your approach kind of organically.

This makes sense. Think I've read a few posts/articles on OOP in rust so I was concerned that I might have brought over too many OOP stuff from Python.

4

For a long lived application, you might try storing your config using a combination of lazy_static and a mutex, so that it's always available. I've done that on a couple of programs. Something like this.

#[macro_use]
extern crate lazy_static;

lazy_static! {
    static ref CONFIG: Mutex<config::Config> = Mutex::new(
        config::Config::from_file()
        .unwrap_or_default()
    );
}

The whenever you need to read your Config struct you just need to aquire the mutex lock.

let config = CONFIG.lock().unwrap()

There may very well be a newer way since once_cell landed, but this is how I'm doing it in two of my Gtk programs. I get what you're saying about constantly passing it as a function parameter, and it may eventually hit a point where that really does become cumbersome and you just need some global state.

5

If you set up your config and never modify it after that, you're probably better off with a RwLock. (Or even just leak it.)

1 Like
6

Oh sweet! Yea I've used this before but never for configuration. This is good to know. I've also never tried using a Mutex, felt like it was beyond my level. But I should give it a try!

I'm not quite sure what "leaking" means in this context. I've heard it used before...

7

Mutex isn't hard once you get over the fear.

In the case where it's never going to be modified after the initial setup, you could dispense with Mutex or RwLock altogether and just allow free access to that data. I believe that's what he's getting at. I only use Mutext because my applications can change the configuration dynamically at runtime, so it avoids nasty collisions.

8

It's simple: you allocate a memory on the heap, but never delete it (OS will clean it up for you when your process terminates).

let config:&'static Config = Box::leak(Box::new(Config{}));
// Now you have a static reference which you can pass around without any issues Borrow checker
9

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.