First project using Rust

code review
1

Hello everyone,
This is my first project using Rust. It’s entirely command-line based. I’d appreciate any feedback so I can improve both the project and my own skills.cli en rust, I made a few more improvements based on the discussion

2

You forgot to add Cargo.toml to the repo...

3

Thank you very much, ready add cargo.

4

clap is perfect for this kind of tool - instead of "manually" processing the command line.

Also - even if you don't - you could use enums instead of strings, e.g.

enum Status {
    Todo,
    InProgress,
    Done,
}

Note that the program currently panics if no arguments are passed ...

2 Likes
5

Welcome, congratulations on taking the plunge to learn rust & respect for reaching out for feedback.

As Jesper has already suggested an enums for your status. I would add a pro-tip in that regard: think about how, and why, you could impl FromStr for Circle To and from Strings - Rust By Example.

I'll also suggest the following example of how you could get your Struct ToDo working nicely with your enum Status:

#[derive(Serialize, Deserialize, Debug)]
struct Todo {
    id: u64,
    description: String,
    status: Status,
    create_at: DateTime<Utc>,
    update_at: DateTime<Utc>,
}

#[derive(Serialize, Deserialize, Debug)]
enum Status {
    Todo,
    InProgress,
    Done,
}

impl Todo {
    fn update_status(&mut self, status: Status) {
        self.status = status;
        self.update_at = Utc::now();
    }
}

As a hint for where to go with this - think about replacing your free functions with a struct ToDoList with functions like load_from_file and get_task (or search the docs for the Index trait if you want to be really clever)

6

Do you really may have so many tasks? usize may work better on low grade machines having 16- or 32- bit bus.

7

Hello,

Thank you very much for your feedback — it was really helpful. I applied your suggestion, and the code is now much cleaner and easier to read.

I really appreciate you taking the time to help!

8

Hello,

Thank you very much for your feedback — it was really helpful. You brought up several points that I found especially interesting, and I’m excited to apply them and keep learning.

I appreciate you taking the time to review my code and help me improve it.

1 Like
9

Hello,

Thank you very much for the recommendation — I’ve already applied it to the code, and I found it really useful.

I appreciate you taking the time to review it!

10

This is not helpful advice.

First, you should choose the integer type that best represents your data. Whether that's u64, u32, or usize. Usually you only use usize if you're indexing or operating on memory.

Second, usize is not actually the "fast" integer: it's the integer that's sized the same as a pointer. On many platforms that's the same as the bus width, but not all.

Third, being that "special size" is sometimes not the fastest choice. It's platform dependent. It's not even clear what the "fastest" width on a given platform is.

Fourth, what's far more likely to impact performance is cache lines and space efficiency, which can only be measured on a case by case basis with profiling. Sometimes it's actually more performant to waste space. Other times it hurts cache locality. It all depends.

Fifth, almost any system they're running on is 64-bit, so they shouldn't be worried to begin with.

Sixth, any performance improvement (or degradation) resulting from their choice of integer is almost guaranteed to be negligible. This is a prime example of premature optimization. So all of this is pointless anyways.

For these reasons (and probably more), this advice is not only unhelpful, but actually confusing. The better advice would be "choose what integer fits your data". If a u32 will do, maybe use that instead. But please don't tell people (wrongly) to use usize because it's "faster".

(this was recently discussed, at length, at Best type alias for uXX type corresponding to usize)

7 Likes
11

Thank you for the detailed explanation — this was actually very helpful.

I now have a much clearer understanding of why choosing an integer type should be based on the data rather than assumptions about performance. Your points about platform dependency and cache effects especially helped clarify things for me.

I really appreciate you taking the time to break this down.

13

I'm not some super genius, nor do I believe I am - I know everything I do now because other people took the (painful, I'm sure) time to teach and correct me. And to be quite honest, I still don't know much about truly low-level hardware details. Half of that discussion I linked goes right over my head.

In any case, my advice (to evaluate your needs and act accordingly, rather than worry about performance before it's a problem) is applicable to developers of any skill or learning level. I'm sorry if I came across as overly aggressive or haughty.

2 Likes
14

Well, let us all stay calm, anyway this is a code review post, not a place for us to have a fight on HPC standards.

Using uszie or u32 all have its pros and cons.

If using usize, pros:

  1. now we mainly live in a world of 64-bit cpu, so processing u32 often requires zero-extension using instructions like movzx to ensue the high bit of the registers are 0 and will not effect normal computing of the 32 bit value. (though through instruction reording and superscalar execution of the cpu, this is typically not such a thing)
  2. because the rust array length uses type usize, sometime using usize could help the LLVM to better eliminate the bound checks when converting u32 to usize type. (though the LLVM is usually smart enough)
  3. ...

cons:

  1. usize behaves somehow very like u64 on 64-bit targets, so it requires more space, wasting the precious cpu L1/L2/L3 cache space and sometimes make the prefetcher fail to predict. (although the effect is minimal on typical concurrency load, not to say we are talking about a sync program, and also the prefetcher is typically smart enough)
  2. ...

If using u32, the pros of usize is the main cons of it, and the cons of usize just get to its pros.

But anyway, first and foremost, @MOCKBA @Kyllingene , this is a rust newcomer's seeking for code review post, and it is about a todo-list app, not a cloud based engine. I really understands you guys want to get the best out of rust programming to our newcomers, but, maybe just maybe, talking about the almost minimal perf effect of choosing usize or u32 is a bit of over engineering and may get the newcomers confused.

And @proRust , just remember two things:

  1. the best design for a project do not have a standard answer, just feel free to try. And there are typically no siver-bullet answer.
  2. as for the performance issue, always remember bench will give you the answer.

For benching you would refer to Criterion.rs - Criterion.rs Documentation for more details. I am not such an expert on rust and high performance computing, so my advice should just be taken for a reference, but certainly not an answer.

15

bringing the discussion back to something that is hopefully useful for the OP, writing their first project

It's always worth exploring crates.io / docs.rs - the quality of libraries and documentation out there is phenomenal. For example: uuid - Rust could be a good alternative to a simple number. Using a few crates in your first project is a good way to get a feel for how rust APIs are usually put together and documented.

16

Fully agreed...

:wink:

Back to code review:

  • At the start of your file, you have some beautifully ordered imports. That honestly looks better than most. However, I noticed a few things:

    • You import std::vec::Vec, when that's automatically imported into every Rust project (unless you tell Rust not to).
    • You import std::fs, which is good form. You just forgot to use fs:: instead of std::fs:: on line 213.

  • You use the builder API for clap, which is fine. However, it's more typical to use the derive API. There's nothing wrong with using the builder API, especially if it's for learning purposes, but the derive API will get you some nice things (you have to type less code, you get type safety, etc.).

    • If you choose not to use the derive API, you should disable it in your Cargo.toml (remove features = ["derive"]).

  • This would be a good first project to learn how to make multiple modules. For example, you could split your file handling logic into a separate module, or your task manipulation. It's not necessary, but it's something to keep in mind when you want to learn a little more about Rust.

  • Don't worry about performance too much right now, just worry about making it work. However, your format_error_command function isn't really optimal. You're going through all the trouble of building and formatting a String, but all you ever do with it is give it to eprintln! without any changes. You should probably just make it print directly; this will also reduce boilerplate.

    Additionally, you have three arguments to format_error_command, all of which are Strings. This is a place a lot of beginners get tripped up. Rust actually has two string types: str and String. String is for when you need to pass ownership around (if you need help understanding that, people around here have some better links than I). In this case, format! (and eprintln!) don't need ownership, so you should instead take &str. It'd look like fn format_error_command(message: &str, erro: &str, help: &str). The bonus is that now you don't have to use String::from when calling the function.

    The general rule of thumb:

    • When you need to store a string somewhere, use String
    • When you just need to let a function look at a string, use &str.

  • When you perform actions on a task that doesn't exist (like todo-cli-rust update 123123123 ...), you don't print an error. This is a bigger issue because you also don't print anything when the task does exist, leaving the user guessing as to what exactly their commands just did. You should print a quick status update after each command, like "task <id> created" or "task <id> doesn't exist".

  • When run without any arguments, it may be more helpful to print the help message than to just say "command not found".

Overall, this is a pretty well-organized, well-thought-out project. Don't get stressed out over any of the details above, especially if you don't understand the str/String part. There are loads of good resources out there, and if you ever need help, feel free to ask for it. It's not a stupid question if you don't know the answer.

If you're looking for ways to expand it (these are just my opinion):

  • Consider adding status reports after each command.
  • Consider allowing multiple tasks to get marked at once, e.g. mark-done 1 2 3 4.
  • Consider adding a pretty-print option, rather than only displaying JSON.
  • Consider adding support for this kind of invocation:
    todo-line-cli add Hello, World! # currently fails because it's technically two arguments
todo-line-cli update 0 Goodbye, World! # fails for the same reason
  • Consider dividing your project into multiple files (not really necessarily at this scale, but it's not bad either).
  • Consider using a crate like dirs to store todo.json in a central location, rather than the current directory.
    • You could also add a flag to allow the user to specify the path to the JSON file, e.g. -f ../../todo.json.
3 Likes
17

Thank you very much for your feedback — it has been really helpful and gave me a lot of ideas to improve my code and learn new concepts.
I truly appreciate the time you took to review my project and provide such detailed suggestions.

I’ll start applying several of your recommendations, especially regarding structuring the project into modules and improving the CLI feedback.
Thanks again for your help!

18

None of your tasks returns a 'Result'. One of the hallmarks of Rust is its strict commitment to error checking. Even if you're dead-dog-certain your task will succeed, it's a good habit to always be prepared to handle things going wrong. Go so far as to add a subcommand designed to fail to test your error handling. At the end of the day, 'unwrap()' is a crutch. To avoid having more matches than a pyromaniac's closet, learn to leverage the syntactical sugary '?' operator.

As for git. Make use of branches and merging, even if you're the sole contributor. It's good practice. Any professional environment you enter that uses git will be using branching, merging, rebasing. Get used to the cycle of:

  • Think of a task
  • git checkout -b the-task
  • work the task
  • test the task
  • git difftool main
  • git rebase main # (possibly with -i to squash commits)
  • git checkout main
  • git merge the-task
  • Rinse, Lather, Repeat (always repeat)

Add a doc comment for each one of your fn's. Tedious but good practice.

19

Thank you for feedback, me helped much for learning.