patternrustMinor
Randomly selecting an adjective and noun, combining them into a message
Viewed 0 times
combiningintomessagerandomlyadjectiveselectingandnounthem
Problem
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:
``
///
/// # Arguments
/// *
///
/// # Return value
/// Returns
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_str
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
/// * - The path to the file to read
///
/// # Return value
/// Returns Vec where each string is a line in the file
fn read_lines(file_path: &Path) -> Vec {
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 ///
/// # Arguments
/// *
file_path: &Path - The path of file to read///
/// # Return value
/// Returns
String containing contents of filefn 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_str
Solution
Everything looks pretty straight-forward, there are just some idiomatic changes to be made.
-
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!
-
The type is also encoded in the name in certain places, which risks being redundant. —
-
Instead of requiring
-
It's inefficient to evaluate the path's
-
There's no reason to create a variable to return it in the next statement; just return it directly —
-
Call trait methods directly when possible, instead of using the Universal Function Call Syntax (UFCS) —
-
-
If only one branch of a
-
Functions shouldn't take a
-
Functions shouldn't take a
-
Use
-
There's no need to specify the item type when collecting into a collection like a vector. You can use the placeholder
-
-
It's better to get the random number generator just once and reuse it.
-
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.
I'd also probably roll all the file stuff into one method:
Note that this also allows me to use
-
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!
-
The type is also encoded in the name in certain places, which risks being redundant. —
file_path: &Path.-
Instead of requiring
&Path, use AsRef to accept more types and improve the ergonomics at the call site.-
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.-
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.-
Call trait methods directly when possible, instead of using the Universal Function Call Syntax (UFCS) —
foo.description() instead of Error::description(&foo).-
read_to_string doesn't return an Ok(content), it returns the number of bytes read. This variable name is misleading.-
If only one branch of a
match statement is used, it's better to use an if let statement.-
Functions shouldn't take a
String if they just reading from it, it's better to take a &str as it is more accepting.-
Functions shouldn't take a
Vec if they just reading from it, it's better to take a &[T] as it is more accepting.-
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.-
There's no need to specify the item type when collecting into a collection like a vector. You can use the placeholder
_ instead — Vec.-
selfcare is confusingly named.-
It's better to get the random number generator just once and reuse it.
-
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(file_path: P) -> Vec
where P: AsRef
{
let content = read_file(file_path);
split_lines(&content)
}
fn open_file(file_path: P) -> File
where P: AsRef
{
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(file_path: P) -> String
where P: AsRef
{
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
.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 ."
fn selfcare(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 ".
///
/// 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(file_path: P) -> Vec
where P: AsRef
{
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 anCode Snippets
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);
}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()
}/// 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));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);Context
StackExchange Code Review Q#125065, answer score: 3
Revisions (0)
No revisions yet.