6
\$\begingroup\$

I don't yet see a code review for a number-guessing game in Rust. Since I am just learning the language, and since the number-guessing game is part of the official Rust tutorial, I thought it would be beneficial both for me and for this site to open a code review request.

I've started with the official tutorial and then built upon it using the basic language constructs (functions, conditionals, loops) to add some error checking and break common logic into reusable functions.

My background is mostly in C and Java development. This is the final example I've come up with after spending about 5 hours reading the book. What I'm interested to learn is whether I'm using the language's unique features in the expected ways and if I'm re-inventing the wheel in any parts.

extern crate rand;

use std::io;
use std::cmp::Ordering;
use rand::Rng;

fn read_i32() -> i32 {
    loop {
        let mut input = String::new();
        io::stdin().read_line(&mut input)
            .expect("Failed to read line.");
        let input: i32 = match input.trim().parse() {
            Ok(n) => n,
            Err(_) => {
                println!("Please input a number.");
                continue;
            },
        };
        return input;
    }
}

fn read_guess(min: i32, max: i32) -> i32 {
    let mut guess: i32;
    loop {
        guess = read_i32();
        if guess < min {
            println!("Please input a number that is at least {}.", min);
        } else if guess > max {
            println!("Please input a number that is no larger than {}.", max);
        } else {
            break;
        }
    }
    guess
}

fn main() {
    println!("Guess the number!");
    let (min, max) = (1, 10);
    let secret_number = rand::thread_rng().gen_range(min, max+1);
    loop {
        println!("Please input your guess, between {} and {}.", min, max);
        let guess = read_guess(min, max);
        match guess.cmp(&secret_number) {
            Ordering::Less    => println!("Too small!"),
            Ordering::Greater => println!("Too big!"),
            Ordering::Equal   => {
                println!("You win!");
                break;
            },
        };
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$ – Phrancis Mar 21 '16 at 4:21
2
\$\begingroup\$

As you probably expect, there's not much that's crazy about your code. It's pretty straight-forward. There's just a little bit that belies your C and Java background. ^_^

  1. There's no need for a mutable guess in read_guess.
  2. There's no need for an input variable at all. This also removes the need for an explicit type as it can be inferred from the return type.
  3. For a very performance oriented approach, move the string allocation outside of the loop. In the off chance that someone keeps entering invalid numbers, there's no reason to allocate a new string each time.
  4. Don't end the expect string with punctuation as a colon is automatically added.
  5. Add spaces around binary operators
  6. Generally, I'd indicate that min and max are CONSTANTs, but doing so may be overkill here, as it would be verbose (const MIN: i32 = 1).
extern crate rand;

use std::io;
use std::io::prelude::*;
use std::cmp::Ordering;
use rand::Rng;

fn read_i32() -> i32 {
    let mut input = String::new();

    loop {
        input.clear();
        io::stdin().read_line(&mut input).expect("Failed to read line");
        match input.trim().parse() {
            Ok(n) => return n,
            Err(_) => println!("Please input a number."),
        };
    }
}

fn read_guess(min: i32, max: i32) -> i32 {
    loop {
        let guess = read_i32();
        if guess < min {
            println!("Please input a number that is at least {}.", min);
        } else if guess > max {
            println!("Please input a number that is no larger than {}.", max);
        } else {
            return guess;
        }
    }
}

fn main() {
    println!("Guess the number!");
    let (min, max) = (1, 10);
    let secret_number = rand::thread_rng().gen_range(min, max + 1);
    loop {
        println!("Please input your guess, between {} and {}.", min, max);
        let guess = read_guess(min, max);
        match guess.cmp(&secret_number) {
            Ordering::Less    => println!("Too small!"),
            Ordering::Greater => println!("Too big!"),
            Ordering::Equal   => {
                println!("You win!");
                break;
            },
        };
    }
}

For fun, here's an alternate read_i32 that didn't end up looking much better:

fn read_i32() -> i32 {
    let stdin = io::stdin();
    for line in stdin.lock().lines() {
        let line = line.expect("Failed to read line");
        match line.trim().parse() {
            Ok(n) => return n,
            Err(_) => println!("Please input a number."),
        }
    }
    unreachable!("Ran out of standard input");
}

Since everything is immutable unless specified otherwise, what is the purpose of a constant?

A constant can exist outside of a function, while let statements must occur inside of one. Additionally, a const is likely to be inlined at compile time, while a static can have its address taken. More info is in the book.

Can you briefly explain when and why I should use the unreachable macro?

You should use unreachable when the code is unreachable, but the compiler cannot statically verify that. In my example, the compiler cannot know that standard input will provide data forever or return an Err which will cause a panic.

Actually, thinking about it further, your original code will go into an infinite loop when /dev/null is connected to standard in: cargo run </dev/null. The version I have will panic and print a marginally helpful error message. Maybe my version is useful after all...

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thank you! your comment #6 took me by surprise (i haven't yet read about constants). since everything is immutable unless specified otherwise, what is the purpose of a constant? \$\endgroup\$ – Woodrow Barlow Mar 21 '16 at 15:46
  • \$\begingroup\$ also, can you briefly explain when and why i should use the unreachable macro? \$\endgroup\$ – Woodrow Barlow Mar 21 '16 at 15:46
  • \$\begingroup\$ @WoodrowBarlow added some comments \$\endgroup\$ – Shepmaster Mar 23 '16 at 12:53

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.