Music TUI app; what to improve?

code review
1

Hi there! I'm somewhat new to Rust, and I've created my first "real" (real as in not a <100 line script) project. It's only ~500 lines, but I'm hoping to get feedback on:

a) Does the code follow good idiomatic Rust guidelines?
b) Specifically relating to the multi-threading: did I use the Arc<Mutex<...>> correctly?
c) Is the code well written; are there parts that are unnecessarily complex, unnecessarily inefficient, or unnecessarily verbose?
d) Any other general feedback.

https://crates.io/crates/mm_music_tui

Thanks!

1 Like
2

Run cargo clippy.

When handling file paths, use &Path instead of &str, and PathBuf instead of String.

Never use &Vec<T>. It's a type that demands that the vector can grow in size, but also & forbids the vector from growing β€” this type makes no sense. Every &Vec<T> can be cast to &[T]. Owned Vec<T> is fine

Don't use .unwrap(), especially in functions returning Result. Use ? instead. Use maybe.map_err(|e| io::Error::new(…))? if the error types aren't compatible.

Use AtomicUsize instead of Mutex<usize>. If you use scoped threads, you won't need Arc.

7 Likes
3

Thank you for the reply!

I implemented these:

  • I ran clippy, and I implemented its suggestions. Thank you for the tip of using &[T] instead of &Vec<T>.
  • I'll have to look into AtomicUsize, but that sounds like a very neat solution instead of having to use a mutex. Thanks for that tip!

But I had some questions about these:

  • I'm already using &Path for many of the function signatures, but I kept the String in the Song struct instead of replacing it with PathBuf since I'm using Serde to serialize those structs. Does PathBuf work fine with Serde?
  • All of the places that I do use .unwrap() are places where I do want the program to panic and crash if there's an issue. Should I always avoid unwrapping? Doesn't that complicate error handling unnecessarily in some cases?

Overall, thanks again for taking the time to suggest these things!

4

Yep!

That sounds like an appropriate use of unwrap() to me.

Suggestions from my point of view:

  1. Add the GitHub link to your Cargo.toml so it will show up on the crates.io MetaData sidebar.
  2. Song can trivially derive Clone.
  3. Consider adding doc comments.
  4. All of this can be simplified to:
    let f = File::open(path).ok()?;
  5. This repeating pattern can use map_err:
    let data = fs::read_to_string(dir_path.join("library.json")).map_err(|e| {
    println!("WARN: Couldn't read library!; {e}");
    Err(())
})?;
  6. This one can use or_else:
    res.or_else(|_| {
    println!("Creating new library");
    Ok(Library::new())
})
    And because it's infallible, you can remove the Result from the function's return value.
    fn get_library_or_create_new_one(dir_path: &Path) -> Library {
    get_library(dir_path).unwrap_or_else(|_| {
        println!("Creating new library");
        Library::new()
    })
}
    Which also simplifies the call site since it does not need error handling.
5

I don't know that I would call the PathBuf support "fine". String is more upfront about what is actually supported.

Always avoid, no, but being unable to open a file (say) is a common situation that you may want to handle in a more user-friendly fashion.

Here's a longer article on the topic.

2 Likes
6

That's true. On the other hand, JSON strings are allowed to contain non-UTF-8 characters.

Examples... 
$ cargo run

Ok(Object {"foo": String("π„ž")})
Err(Error("lone leading surrogate in hex escape", line: 1, column: 14))
Err(Error("invalid unicode code point", line: 1, column: 10))

$ python3

>>> import json
>>> json.loads("{\"foo\":\"\\uD834\\uDD1E\"}")
{'foo': 'π„ž'}
>>> json.loads("{\"foo\":\"\\uDD1E\\uD834\"}")
{'foo': '\udd1e\ud834'}
>>> json.loads("{\"foo\":\"\xff\"}")
{'foo': 'ΓΏ'}

$ node

> JSON.parse("{\"foo\":\"\\uD834\\uDD1E\"}")
{ foo: 'π„ž' }
> JSON.parse("{\"foo\":\"\\uDD1E\\uD834\"}")
{ foo: '\udd1e\ud834' }
> JSON.parse("{\"foo\":\"\xff\"}")
{ foo: 'ΓΏ' }

serde itself handles PathBuf fine (assuming you are not serializing for cross-platform uses). I'm wrong. serde treats Path as a string. RIP. You can use OsString, which works the way I expect PathBuf to.

serde_json has some interoperability problems with JSON, though.

7

I would also suggest to provide a link to GitLab, GitHub, SourceForge and so on, for people who don't use Cargo.

8

mutex.lock().unwrap() is perfectly justified, since lock poisoning is just annoying. I use parking_lot just to get that out of the way.

I wouldn't panic on channel.send(). Just exit the thread quietly. The channels usually get disconnected during program's shutdown, and shutdown by crashing is inelegant.

Panicking when audio doesn't initialize also seems excessive to me. Although this is a fatal error for the program, it is a run-time condition that can happen in correctly-written program. I would use panics only for bugs that require fixing the program itself, not issues with its runtime environment that are outside of your control.

This is especially bad if you unwrap on Option, because user will just get error that None happened, not that their songs went missing or audio drivers are broken.

There are crates like anyhow and quick-error that help with the error handling boilerplate.

Rust has match, if let, let else, and dozens of helper functions on options and results to avoid using .unwrap() in normal code. With just .unwrap() it's hard to tell when that's just lazy coding, or an intentional panic. .expect(…) is clearer. For bug catching, assert! shows intention even better.

4 Likes
9

Awesome, I went back and changed lots of the error handling to use map_err, seemed to really cleanup the code. Thank you!

10

I went back and changed lots of the unwraps to expects, thank you for the feedback!

1 Like