Randomly selecting an adjective and noun, combining them into a message

Question

This is my first Rust program. I have previously written a lot of Python, and some C. I would like some advice on whether I am doing things idiomatically, and any improvements I could make! I also appreciate any suggestions with regards to style.

The code takes two text files as cmd line args. One file contains an adjective on each line, the other file contains a noun on each line. The code reads the files and randomly chooses an entry from each. It then combines them to give a message in this format, which is printed:

"You are a adjective noun"

For example:

"You are a beautiful avocado"

Here is the code:

extern crate rand;

use rand::Rng;
use std::env;
use std::error::Error;
use std::fs::File;
use std::io::prelude::*;
use std::path::Path;

/// Takes file path, returns a vector of strings, each string a line in file.
///
/// # Arguments
/// * `file_path: &Path` - The path to the file to read
///
/// # Return value
/// Returns `Vec<String>` where each string is a line in the file
fn read_lines(file_path: &Path) -> Vec<String> { 
    let content = read_file(file_path);
    split_lines(content)
}

/// Takes file path, returns corresponding `File` object.
///
/// # Arguments
/// * `file_path: &Path` - The path to file to read
///
/// # Return value
/// Returns `File` corresponding to `file_path`
fn open_file(file_path: &Path) -> File {
    let display = file_path.display();
    let file = match File::open(file_path) {
        Err(why) => panic!("Couldn't open file {}: {}", display,
                           Error::description(&why)),
        Ok(file) => file,
    };
    file
}

/// Takes `file path, returns string containing the file's content.
///
/// # Arguments
/// * `file_path: &Path` - The path of file to read
///
/// # Return value
/// Returns `String` containing contents of file
fn read_file(file_path: &Path) -> String {
    let mut file = open_file(file_path);
    let display = file_path.display();
    let mut content = String::new();
    match file.read_to_string(&mut content) {
        Err(why) => panic!("Couldn't read file {}: {}", display,
                           Error::description(&why)),
        Ok(content) => content,
    };
    content
}

/// Split a string on newlines and return vector of resulting strings.
///
/// # Arguments
/// * `string: String` - string to be split
///
/// # Return value
/// Returns `Vec<String>`
fn split_lines(string: String) -> Vec<String> {
    string
        .lines()
        .map(ToOwned::to_owned)
        .collect()
}

/// Chooses random adjective and noun from file, combines into message.
///
/// This function does not return the message, but prints it to stdout.
/// The message is in the format "You are a <adjective> <noun>."
///
/// # Arguments
/// * `adj: Vec<String>` - vector of adjectives
/// * `nouns: Vec<String` - vector of nouns
///
/// # Return value
/// ()
fn selfcare(adj: Vec<String>, nouns: Vec<String>) {
    let adjective = rand::thread_rng().choose(&adj).unwrap();
    let noun = rand::thread_rng().choose(&nouns).unwrap();
    println!("You are a{} {}", adjective, noun)
}


/// Print random inspiring message in format "You are a <adjective> <noun>".
///
/// Takes command line args `adj_file` and `noun_file`, respectively the paths
/// to the file containing the adjectives and the containing the nouns.
fn main() {
    let mut args: Vec<String> = env::args().collect();
    if args.len() == 1 {
        args.push("adjectives.txt".to_string());
        args.push("nouns.txt".to_string());
    }
    let adj_file = Path::new(&args[1]);
    let noun_file = Path::new(&args[2]);
    let adj = read_lines(adj_file);
    let nouns = read_lines(noun_file);
    selfcare(adj, nouns)
}

Answer 1

Everything looks pretty straight-forward, there are just some idiomatic changes to be made.

1. The function documentation, especially the argument and return value documentation, is mostly useless. In a statically typed language, the types provide a large amount of implicit documentation. Variable and method names tell another large chunk. Don't repeat all that compiler-checked documentation in your prose. Maybe even think about introducing new types for documentation purposes!

2. The type is also encoded in the name in certain places, which risks being redundant. — file_path: &Path.

3. Instead of requiring &Path, use AsRef<Path> to accept more types and improve the ergonomics at the call site.

4. It's inefficient to evaluate the path's display before it's needed. Since it's only used for error cases, only evaluate it in error cases. This is another way of saying to scope variables as tightly as possible.

5. There's no reason to create a variable to return it in the next statement; just return it directly — match {...} instead of let foo = match {...}; foo.

6. Call trait methods directly when possible, instead of using the Universal Function Call Syntax (UFCS) — foo.description() instead of Error::description(&foo).

7. read_to_string doesn't return an Ok(content), it returns the number of bytes read. This variable name is misleading.

8. If only one branch of a match statement is used, it's better to use an if let statement.

9. Functions shouldn't take a String if they just reading from it, it's better to take a &str as it is more accepting.

10. Functions shouldn't take a Vec<T> if they just reading from it, it's better to take a &[T] as it is more accepting.

11. Use expect instead on unwrap as it will help you find logic errors much easier when they happen. They may also help end users figure out problems on their own.

12. There's no need to specify the item type when collecting into a collection like a vector. You can use the placeholder _ instead — Vec<_>.

13. selfcare is confusingly named.

14. It's better to get the random number generator just once and reuse it.

15. When writing the sentence to the output, the code doesn't care if it's a string, just that it's printable. Accept a generic type that is printable instead.

---

extern crate rand;

use rand::Rng;
use std::env;
use std::error::Error;
use std::fs::File;
use std::io::prelude::*;
use std::path::Path;

fn read_lines<P>(file_path: P) -> Vec<String>
    where P: AsRef<Path>
{
    let content = read_file(file_path);
    split_lines(&content)
}

fn open_file<P>(file_path: P) -> File
    where P: AsRef<Path>
{
    let file_path = file_path.as_ref();
    match File::open(file_path) {
        Err(why) => panic!("Couldn't open file {}: {}", file_path.display(), why.description()),
        Ok(file) => file,
    }
}

fn read_file<P>(file_path: P) -> String
    where P: AsRef<Path>
{
    let file_path = file_path.as_ref();
    let mut file = open_file(file_path);
    let mut content = String::new();
    if let Err(why) = file.read_to_string(&mut content) {
        panic!("Couldn't read file {}: {}", file_path.display(), why.description());
    }
    content
}

fn split_lines(string: &str) -> Vec<String> {
    string
        .lines()
        .map(ToOwned::to_owned) // Nice
        .collect()
}

/// Chooses random adjective and noun from file, combines into message.
///
/// This function does not return the message, but prints it to stdout.
/// The message is in the format "You are a <adjective> <noun>."
fn selfcare<S>(adj: &[S], nouns: &[S])
    where S: std::fmt::Display
{
    let mut rng = rand::thread_rng();
    let adjective = rng.choose(adj).expect("No adjective found");
    let noun = rng.choose(nouns).expect("No noun found");
    println!("You are a {} {}", adjective, noun)
}

/// Print random inspiring message in format "You are a <adjective> <noun>".
///
/// Takes command line args `adj_file` and `noun_file`, respectively the paths
/// to the file containing the adjectives and the containing the nouns.
fn main() {
    let mut args: Vec<_> = env::args().collect();
    if args.len() == 1 {
        args.push("adjectives.txt".to_string());
        args.push("nouns.txt".to_string());
    }
    let adj = read_lines(&args[1]);
    let nouns = read_lines(&args[2]);
    selfcare(&adj, &nouns);
}

---

I'd also probably roll all the file stuff into one method:

fn read_lines<P>(file_path: P) -> Vec<String>
    where P: AsRef<Path>
{
    let file_path = file_path.as_ref();

    let file = match File::open(file_path) {
        Err(why) => panic!("Couldn't open file {}: {}", file_path.display(), why.description()),
        Ok(file) => file,
    };

    let buf = BufReader::new(file);

    buf.lines().map(|line| {
        match line {
            Ok(l) => l,
            Err(why) => panic!("Couldn't read file {}: {}", file_path.display(), why.description()),
        }
    }).collect()
}

Note that this also allows me to use BufRead::lines, which is probably more efficient than reading the entire file into memory and then splitting it.

---

I'm also not the biggest fan of burying print statements in my code. You even have comments warning the caller about that! Instead, check out this exciting change:

/// Chooses a random adjective and noun.
fn selfcare<A, N, F, T>(adj: &[A], nouns: &[N], f: F) -> T
    where F: FnOnce(&A, &N) -> T
{
    let mut rng = rand::thread_rng();
    let adjective = rng.choose(adj).expect("No adjective found");
    let noun = rng.choose(nouns).expect("No noun found");
    f(adjective, noun)
}

// ...

selfcare(&adj, &nouns, |adjective, noun| println!("You are a {} {}", adjective, noun));

This pushes the printing to the outermost layers of your program, where it belongs :-) It also opens a new name for selfcare - random_adjective_and_noun, thus allowing you to delete the comment. Note that the requirement for being able to print the value disappears, and I took the opportunity to allow the adjective and noun to be different types (which is a bit of YAGNI).

---

Thinking about your main method, it seems a little strange to push into the args vector to provide default values. Instead, I might combine Vec::get and Option::map_or to provide defaults:

let args: Vec<_> = env::args().collect();
let adj_filename = args.get(1).map_or("adjectives.txt", |f| &**f);
let noun_filename = args.get(2).map_or("nouns.txt", |f| &**f);
let adj = read_lines(adj_filename);
let nouns = read_lines(noun_filename);

This has the benefit of not needing to allocate the fallback filenames, and it handles the case of providing the first filename and not the second. In fact, you don't have to even collect the arguments anymore, you could just use the args iterator directly. However, putting them into a vector is pretty standard.