Is my code production quality?

code review
1

Can I improve following code? Is it possible to avoid converting String to Vector?

use std::fs::File;
use std::borrow::Cow;
use std::io::{self, BufRead};
use std::io::Read;
use std::io::Write;
use std::path::Path;
use rand::Rng;

fn get_word<'a>(filename: &'a str) -> Option<String> {
    match  File::open(filename) {
        Ok(file) => {
            let lines = io::BufReader::new(file).lines();
            let linesVec: Vec<_> = lines.map(|x| x.unwrap()).collect();
            let mut rng = rand::thread_rng();
            let num = rng.gen_range(0, linesVec.len() - 1);
            // println!("{}", num);
            //println!("{}", linesVec[num]);
            return Some(linesVec[num].clone());
        },
            Err(e) => {
                eprintln!("Could not open file: {}", e);
                return None;
            }
    }
}

fn main() {
    if let word = get_word("words.txt").unwrap() {
        let mut guess = 7;
        let mut length = word.len();
        let mut gword : Vec<char> = word.chars().map(|x| '_').collect();
        let mut oword : Vec<char> = word.chars().map(|x| x).collect();
        loop {
            if guess == 0 {
                println!("Sorry you have run out of guesses :(");
                println!("{:?}", oword); 
                break;
            }
            if length == 0 {
                println!("Congratulations you have guess correct word: {}", word);
                break;
            }
            println!("The word so far is: {:?}", gword);
            println!("You have {} guesses left", guess);
            print!("Please guess a character: ");
            io::stdout()
                .flush()
                .expect("Error flushing stdout. ");
            let mut c = String::new();
            io::stdin()
                .read_line(&mut c)
                .expect("Error reading char.");
            let ch = c.chars().nth(0).unwrap();
            //println!("{}", ch);
            match oword.iter().position(|& c| c == ch) {
                Some(ind) => {
                    oword[ind] = '_';
                    gword[ind] = ch;
                    length -= 1;
                },
                None => {
                    guess -= 1;
                }
            }
        }
        println!("{:?}", gword); 
    }
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
warning: unused import: `std::borrow::Cow`
 --> src/main.rs:2:5
  |
2 | use std::borrow::Cow;
  |     ^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: unused import: `std::io::Read`
 --> src/main.rs:4:5
  |
4 | use std::io::Read;
  |     ^^^^^^^^^^^^^

warning: unused import: `std::path::Path`
 --> src/main.rs:6:5
  |
6 | use std::path::Path;
  |     ^^^^^^^^^^^^^^^

warning: irrefutable if-let pattern
  --> src/main.rs:28:5
   |
28 | /     if let word = get_word("words.txt").unwrap() {
29 | |         let mut guess = 7;
30 | |         let mut length = word.len();
31 | |         let mut gword : Vec<char> = word.chars().map(|x| '_').collect();
...  |
66 | |         println!("{:?}", gword); 
67 | |     }
   | |_____^
   |
   = note: `#[warn(irrefutable_let_patterns)]` on by default

warning: unused variable: `x`
  --> src/main.rs:31:55
   |
31 |         let mut gword : Vec<char> = word.chars().map(|x| '_').collect();
   |                                                       ^ help: if this is intentional, prefix it with an underscore: `_x`
   |
   = note: `#[warn(unused_variables)]` on by default

warning: variable `linesVec` should have a snake case name
  --> src/main.rs:13:17
   |
13 |             let linesVec: Vec<_> = lines.map(|x| x.unwrap()).collect();
   |                 ^^^^^^^^ help: convert the identifier to snake case: `lines_vec`
   |
   = note: `#[warn(non_snake_case)]` on by default

warning: 6 warnings emitted

    Finished dev [unoptimized + debuginfo] target(s) in 1.00s
     Running `target/debug/playground`
Could not open file: No such file or directory (os error 2)
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/main.rs:28:41
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2

Aside from following the suggestions generated by the compiler as warnings:

Instead of panicking, a fault-tolerant alternative would be to just skip useless lines:

lines 
    .filter_map(std::convert::identity)
    .collect()

As for the random pick, I'm fairly certain the rand crate has special helper functions to pick something directly from the vector, instead of you having to manually set up the range and then indexing into the vector. Use that, if possible.

Please, add a description as to what your program is + instructions at the top of your program with documentation comments (or in a separate README):

//! This is my version of the hangman game.
//!
//! It reads a random line from a user-provided file called "words.txt" in the working directory. …

You can and should pull the let mut c = String::new(); out of the loop and simply call c.clear() after each iteration.

Truncates this String, removing all contents.

While this means the String will have a length of zero, it does not touch its capacity.

3 Likes
3

Is the warning I'd point out. To be production ready you need to consider error handling instead of unwrapping. At the moment, if the file is empty your code will panic. It could prompt the user to add some content to the file and exit gracefully instead.

But take a look at them all, the compiler will show helpful warnings :slight_smile:

1 Like
4

I haven't looked at your main function, but the get_word function can be rewritten like this:

use rand::prelude::*;

fn get_word(filename: &str) -> io::Result<String> {
    let file = File::open(filename)?;
    let lines = io::BufReader::new(file).lines();
    let lines: Vec<String> = lines.collect::<io::Result<_>>()?;
    
    let mut rng = rand::thread_rng();
    match lines.choose(&mut rng) {
        Some(line) => Ok(line.clone()),
        None => Err(io::Error::new(io::ErrorKind::InvalidData, "File is empty"))
    }
}

The changes here are:

  1. Use Result and ? for error handling.
  2. Collect into Result<Vec<String>> instead of unwrapping.
  3. Replace random index with SliceRandom::choose.
  4. Return error on empty file instead of having gen_range panic due to the range being empty.
  5. Remove the print from get_word, instead returning an error.
  6. Replace snake case linesVec with lines.
  7. Took out unnecessary return.
  8. Removed the lifetime as it can be elided in this case.

Generally your code cannot be production quality if it emits any warnings, and production quality code should return errors rather than panicking.

2 Likes
5

Ah, also, there was a bug in the original code. The gen_range function already includes the - 1, so you code cannot ever return the last word.

6

Thanks Alice for pointing that out.

7

If you want to avoid allocating a vec of lines, the cost is that you'll have to make one pass to get the number of lines, and a second pass to get the random nth line. As a middle ground, you could read into a reusable buffer, allocate a single vec to store the offsets of all the newlines in the file, then seek directly to the line you want.

8

Another solution would be to write an iterator adapter that stops at the first error and stores it in a variable. The cost this time is that it will require generating a random number for each line.

use rand::prelude::*;

fn get_word(filename: &str) -> io::Result<String> {
    let file = File::open(filename)?;
    let mut lines = io::BufReader::new(file).lines();
    
    let mut maybe_err = Ok(());
    let lines = std::iter::from_fn(|| {
        match lines.next()? {
            Ok(line) => Some(line),
            Err(e) => {
                maybe_err = Err(e);
                None
            }
        }
    });
    
    let mut rng = rand::thread_rng();
    let res = match lines.choose(&mut rng) {
        Some(line) => Ok(line.clone()),
        None => Err(io::Error::new(io::ErrorKind::InvalidData, "File is empty"))
    };
    maybe_err.and(res)
}
9

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.