Review my first Rust project (Spacepix)

code review
1

Hello all! I've been having a blast learning Rust, it's actually a very intuitive language once you get past the learning curve. I'm coming from higher level languages like JavaScript and Python, Rust is the first Systems Programming language I have learned (Except Arduino C++).

I'm looking for some code review, some suggestions on how I can improve and get more Rusty. I'm sure I'm bringing some bad habits, but I can't catch them. The program is at the moment only a desktop application using egui and reqwest. It gets it's data from the NASA API, it's meant to be educational software.

I'm looking for feedback like:

  • is my project following the "Rust way"
  • how is the speed/performance
  • how can I improve?
  • Anything else you think I should know in my Rust journey?

** NOTE **: You don't have to make an account at the NASA API, you can go to settings->set api key and type in DEMO_KEY and it will work with the demo key, just limited amount of requests and slower speeds.

Here is a link to my project, many thanks!

2

Here comes a massive wall of text for you:

Run cargo clippy to get a sense of whether the code is idiomatic. The lints sometimes have false positives, and some people don't like the defaults. But it's definitely a good starting point to help clean up things that look weird.

It seems ok to me. Nothing stands out as egregiously power hungry.

These are not related to the code, but there are a few things that are jarring while using the app:

In the code specifically:

  • Three of the unit tests are todo!()

  • Tests in the urls module have a race condition in the file system:

    • test_build_url_neows() depends on secret.json, but it does not create it.
    • test_get_secret_sauce() creates secret.json, but it runs in parallel with test_build_url_neows().
    • test_build_url_neows() fails on the first cargo test invocation and succeeds on subsequent invocations. Remove secret.json to see the initial failure condition.

  • Writing JSON as text is not very robust (and you already have json as a dependency. Should probably use it).

    • In fact, this whole test has way too much code...
    • match foo { Err(e) => panic!("{e}") } is also spelled foo.unwrap():
    #[test]
fn test_get_secret_sauce() {
    // Create a test secret.json
    let mut f = File::create("secret.json").unwrap();
    f.write_all(json::object!{"key": "TEST_KEY"}.dump().as_bytes())
        .unwrap();
    assert_eq!(Urls::get_secret_sauce().unwrap(), "TEST_KEY");
}

  • Using thiserror can clean up a lot of the boilerplate on your error types.

    let naive_start_date = NaiveDate::parse_from_str(&dates.0, "%Y-%m-%d")?;

  • Urls::make_secret_sauce() does a lot of unnecessary work by constructing and destructing its own type. It calls the constructor Urls::urls() (which is usually named new) and immediately replaces the heap-allocated strings that it contains. I would:

    1. Move the strings to module-level constants.
    2. Remove the Urls::urls() constructor and refer to the string constants where appropraite.
    3. After these changes, Urls::make_secret_sauce() becomes:
    pub fn make_secret_sauce(saucy_key: &str) -> Result<Self, SecretSauceFileNotFoundError> {
    Ok(Self {
        apod: format!("{APOD_URL}{saucy_key}"),
        neows: format!("{NEOWS_URL}{saucy_key}"),
        donki: format!("{DONKI_URL}{saucy_key}"),
    })
}
    1. The Result here is weird. It looks like it was planned to read a file at some point. This will never return an error. Consider removing it.

  • SpacePixUi has two separate caches for "APOD", one is unused.

    • Apod should probably have an inner struct type instead of the tuples. Naming fields will make the code easier to follow (what's a data.4? I have no idea; I have to look it up in some unrelated code! Oh, it's a title. 4 is a weird name for a title.)

  • If the wrong API key is entered after the app first starts, it will show the error placeholder until the app is restarted, no matter how many times you try to set the correct API key. It's as easy as adding self.apod.cache = None; to App::set_api_key().

  • These six lines could be:

    if open::that("https://api.nasa.gov/").is_err() {
    ui.label("Failed to open web browser.");
}

    But clippy will suggest collapsing this with its parent if. Which is sort of ugly:

    if ui.link("Don't have a NASA API Key?").clicked()
    && open::that("https://api.nasa.gov/").is_err()
{
    ui.label("Failed to open web browser.");
}

  • This is egui-specific, but egui_extras::install_image_loaders() is ideally called once in your App constructor, not in the update cycle (which runs every frame -- it's safe to do this, but wasteful.)

5 Likes
3

This is all beautiful feedback, thank you so much! :smiley:

There are so many things that I was wondering about, like why the menu wasn't closing when you click the item for example.

4

thanks for cargo clippy - never used before

can I make rust-analyzer run it by default, as wonder how many fails I did in my code :smiley:

5

Yes, you can configure RA to use clippy: User Manual [1]

  1. I hope you are well-versed in configuring LSP servers. It's annoyingly IDE-specific. The RA user manual has some hints that might help, though. ↩︎

2 Likes
6

Can you please clarify this?

7

I found a good explanation.