Temperature Converter

code review
1

Hello everyone!
Thank you for your review of the temperature converter code.
I'm new to Rust development, and I'm really enjoying learning so far.

repo
main.rs

2

The README specifies

Be sure to have Rust and Cargo installed. Then, run cargo build && cargo run.

In case you weren't aware: cargo run also includes the building step, so the cargo build && part of this command is reduntant.

The temperature converter function has a string argument that encodes one of two choices, but this string is not any user input but instead provided by the calling code directly. In such a case, it would generally be preferred to define an enum with two variants and use that one as an argument.

I haven't run the code but I looks to me as if when converting from Fahrenheit to Celsius, the prompt will still ask for "the temperature in Celsius" as an input, even though it's expecting the temperature in Fahrenheit.

It's generally good practice to avoid &String function argument types and use &str instead as that's the more generally usable type (I'm looking at the type signature of choice_handler). Rust will implicitly convert &String into &str for you, so you could simply change the signature. This is something that cargo clippy will presumably also point out. Clippy can be quite useful to learn a few small things about more idiomatic Rust or avoiding a few pitfalls, so you might want to try using it yourself.

In fact, trying clippy myself gives two suggestions, one more beyond what I wrote above, to quote it's output:

warning: writing `&String` instead of `&str` involves a new object where a slice will do
  --> src/main.rs:19:27
   |
19 | fn choice_handler(choice: &String) {
   |                           ^^^^^^^ help: change this to: `&str`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg
   = note: `#[warn(clippy::ptr_arg)]` on by default

warning: this pattern reimplements `Result::unwrap_or`
  --> src/main.rs:20:23
   |
20 |       let choice: u32 = match choice.trim().parse() {
   |  _______________________^
21 | |         Ok(num) => num,
22 | |         Err(_) => 0,
23 | |     };
   | |_____^ help: replace with: `choice.trim().parse().unwrap_or(0)`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or
   = note: `#[warn(clippy::manual_unwrap_or)]` on by default
2 Likes
4

I suspected about the redundancy of cargo build && cargo run but wasn't sure and didn't knew about cargo clippy .
Thanks for the feedback!

5

Here's my attempt: https://github.com/DazorPlasma/temp2

I'm new to Rust so any criticism is much appreciated :sparkling_heart:

6

You could consider implementing the conversion from u8 to Choice through From.

You could consider removing the Nothing variant and using a Option<Choice> or Result<Choice, u8> (and implement TryFrom) instead of Choice.

It would be better to create a newtype around Temperature values that stores the value in a unit known at compile time, but is able to receive or produce a value in a unit of choice at run-time. This is what the uom crate does.

Other than that, add some unit tests.

1 Like
7

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.