7
\$\begingroup\$

Am I making good use of the Rust language and APIs? What can be improved?

extern crate rand;

use std::old_io;

fn main() {
    println!("Guessing Game");
    // declaring the type in the left side of let helps random() return the right datatype
    // An alternative is: let answer = (rand::random::<u32>() % 100) + 1;
    let answer: u8 = (rand::random() % 100) + 1;
    let mut guess: String;
    let mut guess_cast: Result<u8, _>;

    loop {
        guess = guesser();
        guess_cast = guess.trim().parse();

        let guess_num = match guess_cast {
            Ok(num) => num,
            Err(_) => { println!("Guess was not a number, try again"); continue; }
        };

        if guess_num < answer { println!("Too low") }
        else if guess_num > answer { println!("Too high") }
        else { println!("You guessed it!"); return; }
    }
}

fn guesser() -> String {
    println!("Guess a number between 0 and 100: ");
    old_io::stdin().read_line().ok().expect("Error getting user input")
}
\$\endgroup\$
6
\$\begingroup\$

Here’s more or less what I would write:

extern crate rand;

use std::io::{self, Write};

fn main() {
    println!("Guessing Game");
    let answer: u8 = (rand::random() % 100) + 1;

    loop {
        print!("Guess a number between 1 and 100: ");
        let _ = io::stdout().flush();
        let mut input = String::new();
        io::stdin().read_line(&mut input).ok().expect("Error getting user input");

        let guess = match input.trim().parse() {
            Ok(num) => num,
            Err(_) => {
                println!("Guess was not a number, try again");
                continue;
            },
        };

        if guess < answer {
            println!("Too low");
        } else if guess > answer {
            println!("Too high");
        } else {
            println!("You guessed it!");
            return;
        }
    }
}

I might replace that whole if block at the end with this instead (plus a use std::cmp::Ordering; and a : u8 on the let guess):

        match guess.cmp(&answer) {
            Ordering::Less => println!("Too low"),
            Ordering::Greater => println!("Too high"),
            Ordering::Equal => {
                println!("You guessed it!");
                return;
            },
        }

There are a number of changes, mostly subtle; some I will comment on, others I may omit:

  • Make sure you get your bounds right (0 is not a possible number).
  • Don’t use std::old_io, it’s on the way out.
  • So long as you flush stdout explicitly, you can get the input on the same line as the query.
  • Prefer semicolons at the end of blocks when producing () (by that, I mean cases like in the if block).
  • With an if statement, either put the whole thing on one line or finish each line after the {.
  • Avoid putting multiple statements on the same line.
  • let mut a; loop { a = …; … } should only be preferred over loop { let a = …; … } if you’re going to use the value of a after the loop. Minimising the distance between definition and declaration and merging the two if possible is strongly preferable. As an aside, any place where you can drop a mut without convolution should also be taken.
  • There doesn’t seem much point in splitting the acquisition of user input into a separate function.
  • Using the same variable name with a variety of suffixes as you shift its format is OK, but if possible find nicer names. And when doing multiple steps, consider merging some lines together (guess_cast seems superfluous to me). I selected input for the user input and guess for the result, an arrangement that is both clearer and simpler in my opinion.

If you wanted to minimise allocations, you could shift the input definition out of the loop and replace its assignation inside the loop with input.clear(). That way it can keep using the same heap allocation time and time again. But that’s an optimisation that is not necessary for something like this. I might or might not do it, depending on what I had eaten, if anything, for morning tea.

Personally I think I’d go with break instead of return in this particular case, but that’s completely subjective.

\$\endgroup\$
  • \$\begingroup\$ You have been answering my question from the Rust IRC channel to StackOverflow to here for the past 2 days... You're becoming kinda of an unofficial mentor sir. \$\endgroup\$ – sargas Mar 25 '15 at 16:58
  • \$\begingroup\$ why would you use a match for guess, instead of a simple if? It sure looks more Rust-like. But is there a real benefit? \$\endgroup\$ – sargas Mar 27 '15 at 1:20
  • 1
    \$\begingroup\$ The two are equivalent and there’s no real benefit of one over the other. It allows you not to write the operands out multiple times (potentially useful when dealing with more complex expressions), it may take fewer lines, with multi-line blocks it winds up with two-deep indentation rather than one, &c.—there are benefits and disadvantages. It’s much of a muchness, really and for a case like this which one I would choose might depend on how many hours of sleep I got last night. \$\endgroup\$ – Chris Morgan Mar 27 '15 at 6:18
5
\$\begingroup\$

In addition to the existing answer, I'd encourage you to use rand's Range generator:

fn answer() -> u8 {
    let between = Range::new(0, 100);
    let mut rng = rand::thread_rng();
    between.ind_sample(&mut rng) + 1
}

I prefer the expressiveness of explicitly asking for a random number in a range, but there's a bigger benefit: this actually generates a uniform sample! In other words, it lets you avoid modulo bias.

\$\endgroup\$
  • 1
    \$\begingroup\$ You can also use rng.gen_range(low, high). The only difference is that you can cache the Range struct, which saves a little bit of computation. \$\endgroup\$ – moveaway00 Jun 3 '15 at 19:48

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.