Hangman coding as beginner exercise

Hi,

I'm in the process of learning Rust for a few weeks. I wanted to take a break with the book and regain confidence after having performed badly at the new quizzes, hence I decided to code on my own the little hangman game.

Would someone could give me his opinion on my code please ?

// Hangman game by ml4nc3
use std::io;

fn main() {
   // Creating mystery word
   let mystery_word = "Bachibouzouk".to_lowercase();
   let mystery_vec = mystery_word.chars().collect::<Vec<char>>();

   // Initializing chances counter
   let mut chances = 10;

   // Creating a Vector with as much _ as letters in mystery word
   let mut word = (0..mystery_word.len()).map(|_| '_').collect::<Vec<char>>();

   // Looping while remaining letters and remaining chances
   while remains_unkown_letters(&word) && chances > 0 {
       
       // Display word
       display(&word);

       // Asking user to enter a letter
       println!("Enter a letter : ");
       let mut user_input: String = String::new();
       io::stdin()
           .read_line(&mut user_input)
           .expect("Failed");

       // Converting input string to char with first letter only
       let input_letter = user_input
                                   .to_lowercase()
                                   .trim()
                                   .chars()
                                   .collect::<Vec<char>>()[0];

       // Checking if letter is present in word
       match letter_verif(&mystery_vec, input_letter) {
           LetterResult::LetterFound(occurrences) => {
               println!("{input_letter} found {occurrences} times !");
               word = update_word(&mystery_vec, &word, input_letter);
           },
           LetterResult::LetterNotFound => {
               chances -= 1;
               println!("Too bad !");
           }
       }
   }
   // Displaying end game message
   match chances > 0 {
       true => println!("Congratulations !"),
       false => println!("The hangman is dead...")
   }

 }

fn remains_unkown_letters(word: &Vec<char>) -> bool {
   for &letter in word {
       if letter == '_' {return true;}
   }
   return false;
}


enum LetterResult {
   LetterNotFound,
   LetterFound(u16)
}

fn letter_verif(word: &Vec<char>, input_letter: char) -> LetterResult {
   let mut counter = 0;
   for &letter in word{
       if letter == input_letter {
           counter +=1;
       }
   }

   match counter {
       0 => return LetterResult::LetterNotFound,
       _ => return LetterResult::LetterFound(counter)
   }
}

fn update_word(mystery_vec: &Vec<char>, word: &Vec<char>, input_letter: char) -> Vec<char> {
   let mut updated_word: Vec<char> = Vec::new();
   for (i, &letter) in word.iter().enumerate() {
       if letter == '_' && mystery_vec[i] == input_letter {
           updated_word.push(input_letter);
       } else {
           updated_word.push(letter);
       }
   }
   updated_word
}

fn display(word: &Vec<char>) {
   for &letter in word {
       print!("{} ", letter.to_uppercase());
   }
   println!();
}

Thanks in advance !

Regards
Nicolas

My first step to improve code is always running clippy. It's output is pretty self-explanatory, so I'll skip it here. A few additional thoughts:

  • check your spelling: letter_verif, unkown
  • '_' should be a constant to prevent typos
  • remains_unknown_letters can be simplified to word.contains(&'_')
  • collect::<Vec<char>>()[0]; is a useless allocation. Use next() instead. This code is also where it can crash, when the user gives no input.
  • I'd use variable shadowing (redeclaration) for mystery_word/mystery_vec and user_input/input_letter
  • In letter_verif, use word.iter().filter(|letter| **letter == input_letter).count()
  • Also, change u16 to usize, which the primitive used for sizes and counts.
  • While I like the enumerate, in this case zip is a better option. And you can also mutate the vec instead of building a new one.
6 Likes

Thank you for this feedback !
I didn't heard about clippy until now. That's an easy step to run to get better !

I'm not satisfied with what I done on managing the possible None value for user input. As you suggested using next(), I thought it was the opportunity of managing the risk with the Option it returns.

So I tried to add a loop until the value entered is correct. Both solution below are working, however I'm not entirely happy with them.

First I went with a while loop and a new variable to manage the exit.

        let mut invalid_input = true;
        let mut input_letter: char = '_';
        
        // Looping until user enters a correct value
        while invalid_input {
            let mut user_input: String = String::new();

            println!("Enter a letter : ");
            io::stdin()
                .read_line(&mut user_input)
                .expect("Failed");

            // Converting input string to char with first letter only, managing missing value
            match user_input.to_lowercase().trim().chars().next() {
                None => continue,
                Some(text) => {
                    invalid_input = false;
                    text
                }
            };
        }

Then tried with a simple loop and a break statement but I have a warning on unneeded mut statement (although it should be mut as I change its value in the match bloc}.

        let mut input_letter: char;
        
        // Looping until user enters a correct value
        loop {
            let mut user_input: String = String::new();

            println!("Enter a letter : ");
            io::stdin()
                .read_line(&mut user_input)
                .expect("Failed");

            // Converting input string to char with first letter only, managing missing value
            match user_input.to_lowercase().trim().chars().next() {
                None => continue,
                Some(text) => {
                    input_letter = text;
                    break;
                }
            }
        }

Is there a cleaner way of managing this situation ?

input_letter is only ever assigned once because of the break following it.
Another way if writing this is using the loop expression with break value (I don't know how it's officially called):

    let input_letter = loop {
        // Looping until user enters a correct value
        let mut user_input: String = String::new();

        println!("Enter a letter : ");
        io::stdin()
            .read_line(&mut user_input)
            .expect("Failed");

        // Converting input string to char with first letter only, managing missing value
        if let Some(text) = user_input.to_lowercase().trim().chars().next() {
            break text;
        };
    };
2 Likes

There are too many possible improvements to enumerate here. Here is how I would have written it.

Three comments worth making explicitly:

  • Your indentation was off by a single space (!)
  • The naming convention is not ideal by far. Hungarian notation is useless (there is a type system), so don't call things "xyz_vec". I haven't fixed this. Names should represent semantics, not low-level technical aspects.
  • You should not take Vec by immutable reference as an argument, it's useless and harmful (it has no more capabilities than a plain slice, but if the callers only have a slice, you now forced them to allocate a Vec just to pass by reference).

Oh great, I missed that ! It is indeed much simpler and cleaner this way ! Thanks !

Thanks for all these !

> You should not take Vec by immutable reference as an argument

Oh, actually I thought we should always try to go immutable in some kind of functional style (although I never coded a functional language)...

Also it appears to me that you avoid using Vec as argument, but it get "converted" (?) to an array ? Does this means we can't push to those kind of argument ? Should I conclude that I should always try to use arrays as arguments for vectors values unless I need to change the content size ?

Also you replaced some match syntax by if/else : when should we prefer one over the other ?

The zip & fold technic is still a bit magic to me, I'll check that deeper. Thanks !

That's not what I was talking about. You should have continued to the rest of my explanation:

That is to say, if you pass by immutable reference, then pass a slice, because then the caller can pass any type convertible to a slice. If you expect a &Vec<_>, then whoever doesn't have a literal Vec (but say, a VecDeque or a plain array) will be forced to allocate a Vec, whereas that shouldn't have been necessary if you had accepted a reference to a slice.

That is also not true. Rust's exclusive mutability means that most of the arguments in favor of immutability are simply moot; you don't have to religiously avoid mutability.

Although sometimes working with values instead of mutability is better, e.g. when applying a chain of transformations to an iterator. But the primary reason for that is purely clarity and readability.

match <boolean expression> { true => ..., false => ... } is redundant. That's exactly what if is for.

There are no arrays in my code. Maybe you are confusing arrays with slices. And no, you can't push to the end of a slice, but this can be trivially inferred from its documentation.

Ok yes I think it's clearer now, thank you.

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.