3
\$\begingroup\$

I've just started learning myself Rust. I am practicing with the exercises from the book available on the official website. Here is my attempt at the temperature converter exercise at the end of chapter 3.

Could you tell me if my approach is correct? Is there something I could have done more efficiently? Is there something I shouldn't do because it's not how Rust is supposed to work?

Thank you!

use std::io;

fn main() {
    println!("Enter a temperature.");

    let mut temperature = String::new();

    io::stdin()
        .read_line(&mut temperature)
        .expect("Failed to read temperature");

    let (value, unit) = temperature.split_at(temperature.len() - 2); 

    let value: f32 = match value.trim().parse() {
        Ok(num) => num,
        Err(_) => panic!("{} is not a number", value),
    };  
    let unit = unit.trim().to_lowercase();

    print!("The input temperature is: {}", temperature);

    if unit == "c" {
        c_to_f(value);
    } else if unit == "f" {
        f_to_c(value);
    } else {
        println!("Unsupported unit");
    }   
}

fn c_to_f(c: f32) {
    output(c * 1.8 + 32.0, "F".to_string());
}

fn f_to_c(f: f32) {
    output((f - 32.0) / 1.8, "C".to_string());
}

fn output(t: f32, u: String) {
    println!("The output temperature is: {}{}", t, u); 
}
```
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I hope some late help will still be helpful! I've erred on the side of providing a bit too much feedback, but this may give you a sense of some of the styles and conventions that Rust uses even if they are overkill for this simple program.

  • Nicely organized code overall, very understandable and clear use of functions.

  • In function arguments, it's generally cleaner/more efficient/more idiomatic to accept &str instead of String. This is usually confusing for beginners, but &str is read-only string data and String is an owned, dynamically growable string, so basically if you only need read-only, you should use &str. As a rule of thumb, it's a good starting point to just make all function arguments &str and the return value String. Here's output fixed; noticed that this means you don't have to to .to_string() in your other functions:

    fn c_to_f(c: f32) {
        output(c * 1.8 + 32.0, "F");
    }
    
    fn f_to_c(f: f32) {
        output((f - 32.0) / 1.8, "C");
    }
    
    fn output(t: f32, u: &str) {
        println!("The output temperature is: {}{}", t, u); 
    }
    
  • Although it doesn't make a huge difference for a small project like this, another good rule of thumb is to avoid functions with printing behavior; instead functions should return Strings, and only the top-level program (i.e. main function) prints. So let's change fn output again. All we need to do is use format! instead of println! (and print in main):

    fn c_to_f(c: f32) -> String {
        output(c * 1.8 + 32.0, "F")
    }
    
    fn f_to_c(f: f32) -> String {
        output((f - 32.0) / 1.8, "C")
    }
    
    fn output(t: f32, u: &str) -> String {
        format!("The output temperature is: {}{}", t, u)
    }
    
  • The let value: f32 logic is a bit verbose, the usual function for this is unwrap_or_else:

    let value: f32 = value.trim().parse().unwrap_or_else(|_| {
        panic!("{} is not a number", value)
    });
    
  • In the line let (value, unit) = temperature.split_at(temperature.len() - 2), this use of split_at is considered fragile and mostly discouraged. Remember that in Rust strings are not just sequences of bytes, but valid UTF-8 data where the characters may not correspond to bytes. So this code introduces a silent failure (i.e. one that is not explicitly exposed and dealt with by the application): I tried running your program on input 4 😍 and I got

    thread 'main' panicked at 'byte index 5 is not a char boundary; it is inside '😍'
    

    In fact, there is actually a second similar issue in the same line: when run with an empty input line in release mode, you have integer underflow:

    thread 'main' panicked at 'byte index 18446744073709551615 is out of bounds of `
    `'
    

    These edge cases do not introduce a bigger problem for your case, but in production code such failures could cause an unexpected crash, or worse, a security vulnerability. So the preference in Rust is always to be explicit about handling errors. You can browse the functions available in the documentation https://doc.rust-lang.org/std/primitive.str.html for some ideas of how to parse strings in general; here the easiest is probably to explicitly check for both errors:

    let mid = temperature.len().checked_sub(2).expect("Input string was empty");
    if !temperature.is_char_boundary(mid) {
        panic!("Input should end in a single-byte char (the unit)");
    }
    let (value, unit) = temperature.split_at(mid);
    
  • If you only need mutability for a temporary amount of time (as with temperature), it's good practice to declare the variable immutable after that: let temperature = temperature

  • The last part of main can be reduced a bit by using match instead of if: match unit.as_str() {

  • Finally, for extensibility it would be great to abstract temperature into an appropriate type; Rust programmers like to leverage static types to make sure they keep for example Celsius and Fahrenheit values separate, without any actual overhead at runtime. Although I won't change your code for this, you would define types like so:

    struct Fahrenheit(f64);
    struct Celsius(f64);
    

    You could alternatively have a Temperature type if you want a lot of functionality related to temperatures in general:

    enum Temperature {
        Fahrenheit(f64),
        Celsius(f64),
    }
    

    Then, you would define parsing functions (parse from &str), display functions (convert to String), and so on on those types, rather than including all of this in main.


Here's the final code incorporating all the suggestions except the last one:

use std::io;

fn main() {
    println!("Enter a temperature.");

    let mut temperature = String::new();
    io::stdin()
        .read_line(&mut temperature)
        .expect("Failed to read temperature");
    let temperature = temperature;

    let mid = temperature.len().checked_sub(2).expect("Input string was empty");
    if !temperature.is_char_boundary(mid) {
        panic!("Input should end in a single-byte char (the unit)");
    }
    let (value, unit) = temperature.split_at(mid);

    let value: f32 = value.trim().parse().unwrap_or_else(|_| {
        panic!("{} is not a number", value)
    });
    let unit = unit.trim().to_lowercase();

    println!("The input temperature is: {}", temperature);

    match unit.as_str() {
        "c" => println!("{}", c_to_f(value)),
        "f" => println!("{}", f_to_c(value)),
        _ => panic!("Unsupported unit"),
    }
}

fn c_to_f(c: f32) -> String {
    output(c * 1.8 + 32.0, "F")
}

fn f_to_c(f: f32) -> String {
    output((f - 32.0) / 1.8, "C")
}

fn output(t: f32, u: &str) -> String {
    format!("The output temperature is: {}{}", t, u)
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Wow, that's an in depth review, it's greatly appreciated! Thank you so much. I need to digest that now. \$\endgroup\$
    – A.D.
    Commented Feb 2, 2021 at 18:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.