First project in Rust, please review

code review
1

This is my first complete project in Rust. It's a command line tool to randomly generate player characters in the table top role-playing game called Traveller. Please review and comment on any questions or criticisms of the code. Thanks

2

This generally looks really good for a first project! Everything I could come up with really is just nitpicking, and if it works, it works. Make it work first, then clean it up after if there's benefit to doing so. Rust's strictness both makes the first working version usually good enough and makes it easier to refactor confidently if is isn't.

  • Commiting IDE configuration files (e.g. .vscode) is a matter of style, but in an OSS environment where many developers may open the project using many different IDEs, it's typically considered good practice to exclude IDE files from the repo using .git/info/exclude.
  • You don't specify a license in Cargo.toml. It's fine to choose to use GPL-3.0-or-later and not the Rust community's default MIT OR Apache-2.0, but you should add the metadata to reflect that.
  • You don't need to use extern crate declarations anymore; that's only necessary for legacy edition 2015 code.
  • You don't need to add # Panics or # Errors sections to every function's documentation. Whether a function can return an error is part of the signature (i.e. whether it returns Result or not), and the usual assumption if you don't document panics is that the function is intended not to panic (if used in a manner consistent with the documentation).
  • There's no requirement to pre-call rand::rng(); it's marginally faster to do so, but all you're saving is a thread-local access and Rc::clone. The actual rng is almost certainly going to dominate that cost.
  • By implementing Distribution<Character> for StandardUniform, you can ask an RNG source directly for a Character instead of mutating.
  • There's no need for the extension traits like Skills; just impl directly on the type. This is allowed since the type your implementing on is from the same crate.
  • I'm not familiar with the Traveller system, but D&D brain wants to name your throw_die as roll_d6 instead, to clarify what die is being rolled.
1 Like
3

Partial review:

  • Since you're not using the 2015 edition, there's no need for the extern crate ... lines in main.rs.

  • Your UPP::random_upp() method is non-idiomatic in several ways:

    • The purpose of the method is just to create a new UPP; modifying an existing one doesn't make any sense. The method should be an associated function instead: keep it in the impl UPP block, get rid of the &mut self receiver, and return a new UPP.
    • There is zero reason to limit the rng argument to just a ThreadRng. It would be better to instead make the method generic in R: Rng and do rng: R.

  • Character::random_gender() has the same problems as random_upp(), and it would also make more sense for it to be associated with Gender instead of Character. Give Character a with_gender(&mut self, g: Gender) method that the random gender is passed to.

  • In Character::nobility_title(), you can change each {return Some("Foo".to_string())} block to just Some("Foo".to_string()).

  • Character::describe_rank() would be better if it returned Option<String>, with None instead of an empty string, akin to nobility_title().

4

Not sure I understand how to implement this one, perhaps I need to go back and read the rand book.

5

Some observations:

  • UPP::social could be an enum
  • Character::printout() could be an implementation of std::fmt::Display
  • Career could be split into separate traits to make each implementation more concise
  • Overall some functions are very long and some code blocks seem repetitive. Consider generalising them further.