Rust cows and bulls

Question

I just wrote a cows and bulls implementation in Rust. It seems a little large. Are there any changes I could make while still keeping it easily readable?

extern crate rand;
use rand::Rng;
use std::{io,process,str};

fn input_check(s: &str, size: u32) -> bool{
    //Check if number is positive valid number
    let mut res = true;
    let guess_num: u64 = match s.trim().parse() {
        Ok(num) => num,
        Err(_)  => {
            println!("Guess is not a valid ");
            res = false;
            return res;
        }
    };
    //Check if the number contains unique numbers and it has the right number of characters
    let unique_check: Vec<char> = s.chars().collect();
    let mut t = unique_check.clone();
    t.dedup();
    if t.len() != unique_check.len() {
        println!("Guess must have unique characters ");
        res = false;
        return res;
    }
    if t.len() != size as usize {
        println!("Guess is not of correct size ");
        res = false;
        return res;
    }
    res
}
fn main() {
    //Main game loop
    loop{
        let mut secret_num_seed: Vec<_>= (48..58).collect();
        let mut rng = rand::thread_rng();
        rng.shuffle(&mut secret_num_seed);
        let mut in_size = String::new();
        println!("Enter the number digit for the Secret number");
        io::stdin().read_line(&mut in_size).expect("Failed to read");
        let size: u32 = match in_size.trim().parse() {
            Ok(num) => num,
            Err(_)  => {
                println!("Enter a number please");
                continue;
            }
        };
        if size < 1 || size > 10 {
            println!("Enter a number between 1 and 10");
            continue;
        }
        let mut secret_num = str::from_utf8(&mut secret_num_seed[0..size as usize]).unwrap();
        println!("{:?}", secret_num);
        println!("Secret number generated, let the games begin!");
        let mut guess = String::new();
        loop{
            let (mut bulls, mut cows) = (0, 0);
            println!("Enter your guess");
            guess = String::new();
            io::stdin().read_line(&mut guess).expect("Failed to read");
            match input_check(&mut guess.trim(), size){
                true  => (),
                false => continue
            }
            for (i, j)  in guess.chars().zip( secret_num.chars() ){
                if i == j{
                    bulls += 1;
                }
                else if secret_num.contains(i){
                    cows += 1;
                }
            }
            if bulls == size{
                println!("Congrats, you've won!, The number is: {} \nWould you like to play again Y?", secret_num);
                let mut op_in = String::new();
                io::stdin().read_line(&mut op_in).expect("Failed to read");
                if op_in.trim() == "Y"{
                    break;
                }
                else{
                    println!("See you next time");
                    process::exit(0);
                }
            }
            println!("Bulls: {}, Cows: {}", bulls, cows);
        }
    }
}

Answer 1

1. Fix the compiler warnings; that's free advice you get just for compiling the code!

   1. Unused variables should be removed or prefixed with a _ to indicate that they are unused on purpose.
   2. Values assigned but never read are a waste of code and time; remove them.
   3. Extra mutability is asking to shoot yourself in the foot later.

2. Use rustfmt. Notable issues in this code:

   1. Use spaces after commas.
   2. Use spaces before {
   3. Use spaces after //
   4. Use spaces around operators like =
   5. Don't use spaces inside ().
   6. else / else if on same line as }

3. res is a useless variable. Just return literals immediately from the function. This also allows you to remove more mutability.

4. Use if let for match statements where only one branch matters.

5. Prefer to let the compiler infer as much type information as it can. Collect into a Vec<_>, for example, not a Vec<char>.

6. Instead of commenting sections of a method, extract those sections to new methods, using the comment as the start of a name.

7. Vec::dedup is flat-out wrong. [1,2,1].dedupe() is [1,2,1]! Instead, use a data structure that naturally removes duplicates, like a HashSet.

8. Use Iterator::count to count an iterator instead of collecting it to a container.

9. Get the random number generator once before the loop, as has already been mentioned to you. Creating the RNG is a potentially expensive operation and it's better to avoid it.

10. Extract even more functions. Functions allow you to give names to sections of code and also provide natural lines to specify types.

11. It's wasteful to re-create and re-shuffle the array if the input is malformed. Check the input first to avoid that.

12. Creating a vector of numbers that are ASCII characters is very confusing. At the very least, create constants that would give names to the values - 58 is a magic number of the worst kind. Better than that, use byte literals (b'0') to express an ASCII character. Even better, write the code you want: an array of the characters 0 through 9. Then you can collect this into a String.

13. There's a useless &mut when calling input_check.

14. There's no need to declare and define two variables on the same line. Spread them out onto two lines.

15. There's no need to declare guess outside the loop - it doesn't need to persist between loop iterations. Moving it inside also removes the need to make it mutable.

16. Don't match on true / false, especially if you don't care about one branch. That's just an if

17. Declare variables as close to their usage site as they can be.

18. guess isn't trimmed in all the uses. You should trim it immediately and save that in a varaible to avoid that.

19. Don't use process:exit(0) just to exit the program. Just using return in the main method will exit with a success.

extern crate rand;

use rand::Rng;

use std::io;
use std::collections::HashSet;

fn is_positive_valid_number(s: &str) -> bool {
    match s.trim().parse::<u64>() {
        Ok(_) => true,
        Err(_) => {
            println!("Guess is not a valid ");
            false
        }
    }
}

fn has_right_number_of_characters(s: &str, size: usize) -> bool {
    if s.chars().count() != size {
        println!("Guess is not of correct size ");
        false
    } else {
        true
    }
}

fn contains_unique_numbers(s: &str, size: usize) -> bool {
    let unique_characters: HashSet<_> = s.chars().collect();
    if unique_characters.len() != size {
        println!("Guess must have unique characters ");
        false
    } else {
        true
    }
}

fn input_check(s: &str, size: u32) -> bool {
    let size = size as usize;

    is_positive_valid_number(s) &&
        has_right_number_of_characters(s, size) &&
        contains_unique_numbers(s, size)
}


fn read_one_line() -> String {
    let mut line = String::new();
    io::stdin().read_line(&mut line).expect("Failed to read");
    line
}

fn get_in_size() -> Result<u32, ()> {
    println!("Enter the number digit for the Secret number");

    let in_size = read_one_line();

    let size = match in_size.trim().parse() {
        Ok(num) => num,
        Err(_) => {
            println!("Enter a number please");
            return Err(());
        }
    };

    if size < 1 || size > 10 {
        println!("Enter a number between 1 and 10");
        return Err(());
    }

    Ok(size)
}

fn generate_secret_num(rng: &mut rand::ThreadRng, size: usize) -> String {
    let mut secret_num_seed = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9'];
    rng.shuffle(&mut secret_num_seed);

    secret_num_seed[0..size].iter().cloned().collect()
}

fn count_bulls_and_cows(guess: &str, secret_num: &str) -> (usize, usize) {
    let mut bulls = 0;
    let mut cows = 0;

    for (i, j) in guess.chars().zip(secret_num.chars()) {
        if i == j {
            bulls += 1;
        } else if secret_num.contains(i) {
            cows += 1;
        }
    }

    (bulls, cows)
}

fn main() {
    let rng = &mut rand::thread_rng();

    loop {
        let size = match get_in_size() {
            Ok(size) => size,
            Err(_) => continue,
        };

        let secret_num = generate_secret_num(rng, size as usize);
        println!("{:?}", secret_num);
        println!("Secret number generated, let the games begin!");

        loop {
            println!("Enter your guess");
            let guess = read_one_line();
            if !input_check(guess.trim(), size) {
                continue;
            }

            let (bulls, cows) = count_bulls_and_cows(&guess, &secret_num);

            if bulls == size as usize {
                println!("Congrats, you've won!, The number is: {} \nWould you like to play \
                          again Y?",
                         secret_num);
                let op_in = read_one_line();
                if op_in.trim() == "Y" {
                    break;
                } else {
                    println!("See you next time");

                    return
                }
            }

            println!("Bulls: {}, Cows: {}", bulls, cows);
        }
    }
}

---

Going beyond the code, I'd highly recommend replacing the printing of error messages and returning of booleans with an error type and Result.

This has the nice side effect of moving all the printlns to the main method. It also highlighted that in one place you were using a u64 to parse the guess, but elsewhere using a u32.

Note that the error types also have a bit more detail than the previous messages.

#[macro_use]
extern crate quick_error;
extern crate rand;

use rand::Rng;

use std::io;
use std::collections::HashSet;

quick_error!{
    #[derive(Debug)]
    enum Error {
        InvalidSize(err: std::num::ParseIntError) {
            description("Size is not a positive number")
            display("Size is not a positive number: {}", err)
        }
        SizeOutOfRange(size: u32) {
            description("Size must be between 1 and 10")
            display("Size must be between 1 and 10, but it is {}", size)
        }
        InvalidPositiveNumber(err: std::num::ParseIntError) {
            description("Guess is not a positive number")
            display("Guess is not a positive number: {}", err)
        }
        IncorrectSize(expected: usize, actual: usize) {
            description("Guess was not the correct length")
            display("Expected a guess with {} characters, but it was {}", expected, actual)
        }
        NotUnique {
            description("Guess did not contain unique characters")
        }
    }
}

type Result<T> = ::std::result::Result<T, Error>;

fn is_positive_valid_number(s: &str) -> Result<u32> {
    s.trim().parse().map_err(Error::InvalidPositiveNumber)
}

fn has_right_number_of_characters(s: &str, size: usize) -> Result<()> {
    let actual = s.chars().count();
    if actual != size {
        Err(Error::IncorrectSize(size, actual))
    } else {
        Ok(())
    }
}

fn contains_unique_numbers(s: &str, size: usize) -> Result<()> {
    let unique_characters: HashSet<_> = s.chars().collect();
    if unique_characters.len() != size {
        Err(Error::NotUnique)
    } else {
        Ok(())
    }
}

fn input_check(s: &str, size: u32) -> Result<()> {
    let size = size as usize;

    is_positive_valid_number(s)
        .and_then(|_| has_right_number_of_characters(s, size))
        .and_then(|_| contains_unique_numbers(s, size))
}


fn read_one_line() -> String {
    let mut line = String::new();
    io::stdin().read_line(&mut line).expect("Failed to read");
    line
}

fn get_in_size() -> Result<u32> {
    let in_size = read_one_line();

    let size = try!(in_size.trim().parse().map_err(Error::InvalidSize));

    if size < 1 || size > 10 {
        return Err(Error::SizeOutOfRange(size));
    }

    Ok(size)
}

fn generate_secret_num(rng: &mut rand::ThreadRng, size: usize) -> String {
    let mut secret_num_seed = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9'];
    rng.shuffle(&mut secret_num_seed);

    secret_num_seed[0..size].iter().cloned().collect()
}

fn count_bulls_and_cows(guess: &str, secret_num: &str) -> (usize, usize) {
    let mut bulls = 0;
    let mut cows = 0;

    for (i, j) in guess.chars().zip(secret_num.chars()) {
        if i == j {
            bulls += 1;
        } else if secret_num.contains(i) {
            cows += 1;
        }
    }

    (bulls, cows)
}

fn main() {
    let rng = &mut rand::thread_rng();

    loop {
        println!("Enter the number digit for the Secret number");
        let size = match get_in_size() {
            Ok(size) => size,
            Err(e) => {
                println!("{}", e);
                continue;
            },
        };

        let secret_num = generate_secret_num(rng, size as usize);
        println!("{:?}", secret_num);
        println!("Secret number generated, let the games begin!");

        loop {
            println!("Enter your guess");
            let guess = read_one_line();
            if let Err(e) = input_check(guess.trim(), size) {
                println!("{}", e);
                continue;
            }

            let (bulls, cows) = count_bulls_and_cows(&guess, &secret_num);

            if bulls == size as usize {
                println!("Congrats, you've won!, The number is: {} \nWould you like to play \
                          again Y?",
                         secret_num);
                let op_in = read_one_line();
                if op_in.trim() == "Y" {
                    break;
                } else {
                    println!("See you next time");

                    return
                }
            }

            println!("Bulls: {}, Cows: {}", bulls, cows);
        }
    }
}