New to Rust and looking for feedback writing cleaner and more idiomatic code

code review
1

I've started a todo list manager project to start learning Rust.

I'd be interested in advice on improving my code.

A few areas in particular I think I might need to pay attention to:

  • I use unwind a lot. At the very least, I think expect with a pretty error message would be an improvement.
  • The tada::action::* modules seem like they could be cleaned up a bit. In particular, I'd love if the tada::Action struct could include a closure, so that when an action is picked, I can just call action.closure(args) instead of the match on src/bin/tada.rs:27.
  • I don't like how repetitive and deeply nested determine_filename in src/action/mod.rs is.
  • Every time I create a new action, I need to add it to the end of src/action/mod.rs and also in the action and main functions in src/bin/tada.rs. It feels like it should be possible to reduce this to one central place to list available actions.

I'm sure there are other areas that can be improved on.

Things I don't need feedback on:

  • Tabs are the best form of indentation. You will not be able to convince me otherwise.
2

I took a quick look around and here are some things I noticed, mostly about Items:

  • In lib.rs you have:

    pub mod action;
pub mod item;
pub mod list;

pub use crate::action::*;
pub use crate::item::*;
pub use crate::list::*;

    That means that every item has 2 paths it can be accessed through: with action etc. or without. It would be better, IMO, to not make the modules public.

  • ItemFormatConfig could implement Clone.

  • ItemFormatConfig::new() checks the terminal size. It is surprising for a plain constructor to do that; it should highlight that dependency in its name (and documentation).

  • impl fmt::Display for Item does a lot of constructing temporary strings. It should instead try to do as much as possible with write!(f, ...) calls; this avoids memory allocations and text copying.

  • Item has lazy-initialized data, but that data is computed from pub fields, so an item could be mutated so that the lazy fields are out of sync. That might be an acceptable cost, but it should be documented. If that's undesired, use getter methods instead of public fields.

  • determine_filename's deeply nested matches might be better written by using Option::or_else() or by having each match just return the value if it succeeds.

5 Likes
3

Thanks for your feedback. Based on your suggestions, I've refactored impl fmt::Display for Item significantly, hidden all of Item's fields behind getters/setters (allowing the lazy initialized fields to be wiped when the data they're built from changes), and refactored determine_filename to be a lot tidier.

I'll look at implementing the other suggestions too.

4

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.