3
\$\begingroup\$

I started learning Rust a few hours ago with the Rust Book (2018 edition) and as part of one of the exercises decided to make a temperature converter. I wanted to make sure it handled all inputs properly in an idiomatic fashion. The code is as follows:

use std::io;
use std::io::Write;
use std::str::FromStr;

fn main() {
    println!("Welcome to the temperature converter!");

    println!("Pick a conversion:");
    println!("[1] Fahrenheit to Celsius");
    println!("[2] Celsius to Fahrenheit");

    let choice: u32 = loop {
        let value = read_value_from_input("> ", "Please enter a valid integer!");

        if value == 1 || value == 2 {
            break value;
        }
        println!("Please enter a valid choice (0 or 1)!");
    };

    if choice == 1 {
        let temperature: f64 = read_value_from_input("Enter the temperature to convert: ",
            "Please enter a valid floating point variable!");

        println!("{:.2} °F is {:.2} °C.", temperature, (temperature - 32f64) * 5f64 / 9f64);

    } else if choice == 2 {
        let temperature: f64 = read_value_from_input("Enter the temperature to convert: ",
            "Please enter a valid floating point variable!");

        println!("{:.2} °C is {:.2} °F.", temperature, temperature * 9f64 / 5f64 + 32f64);

    } else {
        println!("{} was not a valid choice!", choice);
    }
}

fn read_value_from_input<T: FromStr>(prompt: &str, error_message: &str) -> T {
    let result: T = loop {
        print!("{}", prompt);
        io::stdout().flush().expect("Unable to flush STDOUT!");

        let mut input_value = String::new();

        io::stdin().read_line(&mut input_value)
            .expect(error_message);

        match input_value.trim().parse() {
            Ok(value) => break value,
            Err(_) => {
                println!("{}", error_message);
                continue;
            }
        }
    };
    result
}

Here are the things I want to know from this code review:

  • Is my code idiomatic Rust? Does it follow its coding conventions, style, etc.?
  • Does my code handle all erroneous inputs properly?
  • Can the code be shortened or optimized in any way?
\$\endgroup\$
2
\$\begingroup\$

Is my code idiomatic Rust? Does it follow its coding conventions, style, etc.?

It's pretty close. There are a couple of things I would recommend changing:

  1. Use match whenever multiple branches in your flow are based upon a single value.

    This means that you should replace the if ... else if ... else with a match statement:

    match choice {
        1 => { ... },
        2 => { ... },
        _ => { ... },
    }
    

    I'd also use match instead of if value == 1 || value == 2 but in that case you aren't gaining as much:

    match value {
        1 | 2 => break value,
        _ => (),
    }
    
  2. read_value_from_input should probably return a Result<T, Error> or Option<T>. This provides you with a couple of benefits.

    First, by returning a Result you can use the new "try" operator (?) in the body (which is essentially just a way of automatically return an error in the case that they fail):

    print!("{}", prompt);
    io::stdout().flush()?;
    
    let mut input_value = String::new();
    io::stdin().read_line(&mut input_value)?;
    
    return input_value.trim().parse()?;
    

    Second, this way you can handle how to control failures higher up in the program flow.

    One problem with this approach is that all Errors may not have the same type (parse errors and io Errors are different) but you should be able to use Result<T, impl Error> as your return type which should work with all of them. Or, maybe you want the program to immediately exit on those conditions, in which case expect is fitting.

  3. Reduce the number of loops you have.

    Right now you have two loops. One in read_value_from_input which gets any value from stdin, and one in your main function which validates that integer or tries again. I'd try to just handle all of that logic in one or the other, but since you only want the value 1 or 2 that could be difficult.

  4. Use return in the read value loop.

    If you didn't remove that loop, you should at least be returning directly instead of breaking out of the loop and then finishing with that value. Also, the continue in there isn't doing anything as far as I can tell.

    fn read_value_from_input<T: FromStr>(prompt: &str, error_message: &str) -> T {
        loop {
            print!("{}", prompt);
            io::stdout().flush().expect("Unable to flush STDOUT!");
    
            let mut input_value = String::new();
    
            io::stdin().read_line(&mut input_value)
                .expect(error_message);
    
            match input_value.trim().parse() {
                Ok(value) => return value,
                Err(_) => {
                    println!("{}", error_message);
                }
            }
        }
    }
    

Does my code handle all erroneous inputs properly?

Your code currently panics when it can't flush or read from stdin. This may be what you want. Beyond that, your code does seem to handle inputs correctly.

Can the code be shortened or optimized in any way?

I've given a few ways it can be shortened and optimized. One more thing I would suggest is passing around the string or character instead of an integer. This would save that parsing step.

You could even create an enum with the possible modes of your program and impl FromStr for it, then pass that around (which is what I did in my working example).

Here's my complete example. It has essentially the same behavior as yours.

use std::io;
use std::io::Write;
use std::str::FromStr;

struct ParseModeError {}

enum Mode {
    F2C,
    C2F,
}
impl FromStr for Mode {
    type Err = ParseModeError;

    fn from_str(s: &str) -> Result<Mode, ParseModeError> {
        match s {
            "1" => Ok(Mode::F2C),
            "2" => Ok(Mode::C2F),
            _ => Err(ParseModeError {}),
        }
    }
}

fn main() {
    println!("Welcome to the temperature converter!");

    println!("Pick a conversion:");
    println!("[1] Fahrenheit to Celsius");
    println!("[2] Celsius to Fahrenheit");

    let choice: Mode = read_value_from_input(
        "> ",
        "Please enter a valid choice (0 or 1)!"
    );

    match choice {
        Mode::F2C => {
            let temperature: f64 = read_value_from_input(
                "Enter the temperature to convert: ",
                "Please enter a valid floating point variable!"
            );

            println!(
                "{:.2} °F is {:.2} °C.",
                temperature,
                (temperature - 32f64) * 5f64 / 9f64
            );
        },
        Mode::C2F => {
            let temperature: f64 = read_value_from_input(
                "Enter the temperature to convert: ",
                "Please enter a valid floating point variable!"
            );

            println!(
                "{:.2} °C is {:.2} °F.",
                temperature,
                temperature * 9f64 / 5f64 + 32f64
            );
        },
    }
}

fn read_value_from_input<T: FromStr>(prompt: &str, error_message: &str) -> T {
    loop {
        print!("{}", prompt);
        io::stdout().flush()
            .expect("Unable to flush STDOUT!");

        let mut input_value = String::new();
        io::stdin().read_line(&mut input_value)
            .expect("Unable to read STDIN!");

        match input_value.trim().parse() {
            Ok(value) => return value,
            Err(_) => println!("{}", error_message),
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wow, this is a very thorough answer! One more question though: Is there a way to keep constants like 32f64 more readable while still being of type f64? \$\endgroup\$ – Arnav Borborah Oct 22 '18 at 23:59
  • 1
    \$\begingroup\$ @ArnavBorborah you can use static constants. Here's a demonstration: play.rust-lang.org/… \$\endgroup\$ – PitaJ Oct 23 '18 at 4:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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