Tips to improve my code


#1

Hi Rustaceans !

I’m currently learning the language (right now I’m at the chapter 7 of the Rust Book), and at the same time I like to experiment a bit to apply what I’ve learned.

Here is a piece of code I’ve wrote, I know it is far from perfect and that’s why I create this post, to ask you for tips and advice to improve my code, make it shorter, and more efficient.

fn main() {
    // Variable that count the number of turn in the loop
    let mut attempts = 0;
    // Hard-coded word to be guessed by the user
    let word = String::from("programming");
    // Vector that will contain the guessed letters
    let mut guessed: Vec<char> = Vec::with_capacity(word.len());
    // Filling the vector with '_' characters (for display purpose)
    for _ in 0..word.len() {
        guessed.push('_');
    }

    println!("Guess the word!");
    // Creating a string that will be used to display the number of letters in the word
    // like so: _ _ _ _ _ _ _ _ _ _ _
    let mut guessed_default_str = String::new();
    for i in 0..word.len() {
        // Add the '_' character in the string
        guessed_default_str.push(guessed[i]);
        // If it's not the last one, add a space
        if i < word.len() {
            guessed_default_str.push(' ');
        }
    }
    println!("{}", guessed_default_str);

    // Start the game loop
    loop {
        println!("Enter a letter");
        // Prepare the guess variable that will contain the user's input
        let mut guess = String::new();
        // Ask the user to enter something
        std::io::stdin().read_line(&mut guess).expect("Failed to read line");
        // Convert the user input to a char if possible, otherwise ignore
        let guess: char = match guess.trim().parse() {
            Ok(character) => character,
            Err(_) => continue,
        };
        // Increment the attempts variable
        attempts = attempts + 1;
        // Checking if the letter is in the word
        for (index, character) in word.chars().enumerate() {
            // If not in the word, proceed to the next loop turn
            if character != guess {
                continue;
            }
            // Add the correctly guessed letter at the correct index of the vector
            guessed[index] = character;
        }
        // Prepare a string to display the state of guessed letters
        let mut guessed_str: String = String::new();
        for i in 0..word.len() {
            // Add the guessed letter or '_' character
            guessed_str.push(guessed[i]);
            // If not the last one, add a space
            if i < word.len() {
                guessed_str.push(' ');
            }
        }
        println!("{}", guessed_str);
        // If there is no '_' in the vector anymore, it means all the letters have been found,
        // so the game stops with a message showing the number of attempts
        if !guessed.contains(&'_') {
            println!("You found the word in {} attempts!", attempts);
            break;
        }
    }
}

It’s just a little game in which the user has to guess the letters of the hard-coded word.

Thanks in advance for your help! :smiley:


#2

string.len() is not a number of characters. It’s only a size of string’s buffer, but characters take variable number of bytes.

Also whenever you create and fill a Vec from something else, you can probably use map instead. Try:

let mut guessed: Vec<_> = word.chars().map(|_| '_').collect();

#3

Iterating over a range is definitely not what you want here. For this, you should iterate over the characters (not bytes) in your input string, i.e.

for ch in word.chars() {
    // Code goes here
}

#4

Thanks for your answer!

I haven’t seen this yet: .map(|_| '_') especially the |_| part, I guess I will learn it further in the book but could you enlighten me on this :pray:

The _ means any type in the Vec<_> ?


#5

This is more a stylistic issue than anything else but I’d probably want to cut down on the number of comments. I know lab supervisors for a lot of CS units tend to say you should comment every line, but there is a very common saying that “every comment is an apology for writing hard-to-understand code”.

For example, you could skip this entire comment by just choosing a better name:

// Hard-coded word to be guessed by the user
let word = String::from("programming");

// vs

let target_word = String::from("programming");

I’m currently employed as a programmer and over time you notice that it’s too easy for comments to become out of sync with (or even directly contradict) the code they’re commenting. I guess that’s also why people sometimes say another name for them is “lies” :stuck_out_tongue_winking_eye:


You probably want to break the program out into functions. For example, you could have a print_hangman_word() function which will print out the string, replacing unknown letters with underscores (at least, I assume the idea is to write a hangman-like program).

Likewise you could have a get_input() function which gets the guessed letter/word from the user and converts it to a form which is easier for the rest of the program to use (e.g. Result<char>, letting the caller deal with errors appropriately).

It’s not a bad thing to pull two or three lines out into their own function if it aids in readability.


Instead of storing the strings as a String, you will probably find it easier to store them as an array of characters. That way you don’t need to deal with string indexing and other annoying UTF-8 issues.


I know what you’re doing with the final !guessed.contains(&'_') check, but I’m not sure if this is the best way of checking whether you’re finished. It’s kinda brittle and what happens when your target word actually contains an underscore? It would probably be better to keep a tally of the number of letters guessed so far and the total number of letters, then check that one equals the other.


In this case it means you are mapping over each character in the word and for each character we don’t actually care what it is (the first _ in |_| "_" is the closure’s input parameter), each character should be converted into an underscore.


#6

Wow thanks for the huge reply :wink:

I was more looking into Rust related advice rather than general coding habits (should have been more explicit in my title) but still thanks for taking the time writing this.

It’s just prototyping, that’s why I didn’t really bother structuring my code, also, I just added the comments for the post to avoid questions about what the code is actually doing, but I agree with you, explicit code > comments :+1:

That’s indeed the idea :slight_smile:

Good point, I am not at ease with Rust’s collections yet so I opted for a String but an array of chars would have been easier.

Totally agree with you if it was a real program I would have done something better, but like I said, it’s just a prototype to learn Rust.

After what I just read in the doc, Rust’s closures are kind of like the arrow functions in JS ?

|x| x + 1

is like

(x) => x + 1

#7

The name _ in Rust is special and it’s sort of a “whatever” symbol. When used as variable name (or function arguments) it discards it (and the compiler doesn’t warn about unused variable). When used as a type it makes compiler guess the type (Vec<_> is a vec of something, and the compiler will auto-complete that to be Vec<char> in your case).


#8

Yep, that’s the easiest way of describing it. Except instead of using the arrow you use a pair of |'s… I think that syntax originally came from Ruby.

It’s really nice to see you asking for feedback and trying to improve your Rust-foo!

Another thing… You could replace the guessed_default_str loop (just after printing “Guess the word!”) with word.chars().join(" ") using the itertools crate. Although you mention this is a quick prototype, so I don’t know whether you want to pull in outside crates or not.


#9

That’s some nifty features! Starting to really like Rust :slight_smile:


#10

Damn I was looking for something like that, too bad it’s not in the standard library, would have expected so. :disappointed:
But thanks for pointing it out, might come in handy when I write something more serious.

Coming from high-level language (mostly JS) it’s kind of a big change, using references, handling memory etc, that’s why I prefer to ask for advice from Rust experts haha :smiley:


#11

It actually is, but it’s defined for a slice, not an iterator (docs) and unless you’re using a Vec of chars elsewhere it feels kinda unnecessary to first collect the word.chars() into a temporary Vec<char> just so you can concatenate them all into a string immediately after.