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.
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
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!
Here's my attempt: https://github.com/DazorPlasma/temp2
I'm new to Rust so any criticism is much appreciated
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.