Beginner project: a grocery list maker using `serde` to parse JSON

code review
1

I wrote a typical first project type application called grusterylist, which I've been using to put grocery lists together. It updates a library of groceries we regularly buy, a library of recipes we often make, saves a most recent grocery list in case I just want to update a list I made earlier, and prints out a grocery list. I've been using Element to copy and paste my list and send to my phone before we go shopping.

I would really like feedback about what needs to happen to make this a better demonstration of me knowing what I'm doing. I'm especially interested in how I should be structuring a program like this and how I should be writing tests and handling errors.

I also hope this will be useful for any beginner programmer Rust learners like me.

Update: I've committed a lot of changes to the repository in response to the help I received here so far, so if people are seeing this now or seeing it again it would be great to get feedback on those improvements.

2

Others may have more comments. But looking it over, here are some possible improvements that jumped out at me:

  • Better organization would help. For example Groceries contains write_to_file, read_from_file, and get_sections. These aren't really specific to the impl for a given instance of Groceries. It feels like maybe these should be made into a module rather than an impl block.
  • A lot of the read / write code is duplicated. Perhaps this could be done with a generic function.
  • It might be good to show off crates more. Like maybe tracing for logging or perhaps structopt
  • The loop condition on lib.rs:242 with no_need_to_add_to_recipes could be made into the loop conditional (avoiding the local variable).
  • Some of the huge methods like run could be broken up into private functions, where each of these commented sections could become a rustdoc for a private method. Then it is easy to get an overview of the run loop because it calls 3-4 clearly named functions in sequence which show what is happening.
  • There are more type annotation than are required by the compiler.
  • Show code being run through clippy this is a great tool for learning good and bad patterns.
  • Multi-line string literals could avoid some aquard line wraps
  • A lot of things are just strings. These could made type safe by creating types for these things and using shrinkwraprs to retain all the expected methods from string.
1 Like
3

Thank you for all your helpful suggestions!

Some bits I wasn't clear on:

I couldn't understand what you were suggesting. Here's a snippet of the code:

    let mut no_need_to_add_to_recipes = false;
    while !no_need_to_add_to_recipes {
        let add_recipes_prompt =
            "Do we want to add more recipes to our recipes library?\n(y for yes, any other key for no)";
        eprintln!("{}", add_recipes_prompt);
        if let "y" = input().trim() {
            // ... do stuff
        } else {
            no_need_to_add_to_recipes = true;
        }
    }

Then,

I use cargo clippy to check my code if that's what you mean. Or do you mean some other use for clippy that I don't know?

Also,

Could you explain bit about what you mean about this? I'm at that stage where I'm not following. Totally fine if you just link to concepts elsewhere so I can understand.

Thanks again for taking a look and giving your feedback!

4

You have a pervasive pattern of returning a Result<T, &'static str> which always return an Ok variant, and panics when there are errors instead of returning the Err variant. This is an excellent starting point to learn to use the ? operator and proper error handling.

2 Likes
6

I was suggesting factoring out:

fn promp_for_add_recipe() -> bool {
        let add_recipes_prompt =
            "Do we want to add more recipes to our recipes library?\n(y for yes, any other key for no)";
        eprintln!("{}", add_recipes_prompt);
        "y" == input().trim()
}

Then the loop is:

    while promp_for_add_recipe() {
        // ... do stuff
    }

Yes. I was just suggesting mentioning that in like a Readme or other document walking through the code.

Sure. For example you have:

struct GroceriesSection {
    section: String,
    items: Vec<String>,
}
struct Recipe {
    recipe: String,
    ingredients: Vec<String>,
}
struct ShoppingList {
    recipes_msg: String,
    recipes: Vec<String>,
    checklist_msg: String,
    checklist: Vec<String>,
    list_msg: String,
    list: Vec<String>,
}

Here the types do not make clear which items are used in which places. For example GroceriesSection::String is totally unrelated to the items in Recipe::ingredients. However GroceriesSection::items is the same. So rather than all of these being just String you could have types like:

#[derive(Shrinkwrap)]
struct RecipeName(String);
#[derive(Shrinkwrap)]
struct Ingredient{ name: String }

etc. (using shrinkwraprs)
This would make things like ShoppingList much easier to understand by looking at the struct.

1 Like
7

Thanks so much for suggesting this. It's taken me a while to get here, but I took your suggestion to heart and I'd love to get feedback about how I'm handling errors in grusterylist now.

8

Thank you for generously answering my follow-up questions! I've done a lot of work inspired by your suggestions. Thanks again for taking the time. If you'd be interested to see how I've changed things here's the link again.

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.