patternrustMinor
A Rusty implementation of CowSay
Viewed 0 times
rustyimplementationcowsay
Problem
I'm making my first foray into Rust. I wanted to copy a fun and non utilitarian tool so I chose CowSay. It's not feature complete (CowSay has a lot of options) but it's a start.
My primary reasons for asking here are:
(The code is available on github for your cloning pleasure)
``
// should be split like so:
// ["prolon", "ng a", "hat", "bear"]
fn chunk_args (args: Vec, max_size: usize) -> Vec {
let mut lines = Vec::with_capacity(args.len() * 2);
let remainder: String = args.iter()
.fold(String::new(), |mut acc, arg| {
if !acc.is_empty() {
if (arg.chars().count() + 1) + acc.chars().count() , width: usize) -> String {
let total_length = lines.len() - 1;
let formatted_lines = lines
.iter()
.enumerate()
.map(|(idx, line)| {
let current_length = line.clone().chars().count();
let padding: String = (0..width - current_length).map(|_| ' ').collect();
let (start, end) = match idx {
0 => ('/', '\\'),
My primary reasons for asking here are:
- To get better at idiomatic Rust (this is the first thing I've written in the language)
- Improve the implementation of the
chunk_argsandmulti_linefunctions (either by improving them or making me aware that they falling prey to XY thinking)
(The code is available on github for your cloning pleasure)
main.rs``
extern crate getopts;
use std::io::prelude::*;
use std::cmp;
use getopts::Options;
const PROGRAM_NAME: &'static str = "rsay";
const COW: &'static str = include_str!("cow.txt");
const DEFAULT_LINE_WIDTH: usize = 40;
fn print_usage (opts: Options) {
let brief = format!("Usage: {} [-OPTIONS] [ARG...]", PROGRAM_NAME);
print!("{}", opts.usage(&brief));
}
fn parse_numeric(value: String, default: usize) -> usize {
match value.parse::() {
Ok(n) => { n },
Err(_) => { default },
}
}
// Ensure that lines have a maximum length of max_size
// and that there is a one space buffer between args
// e.g. with a // should be split like so:
// ["prolon", "ng a", "hat", "bear"]
fn chunk_args (args: Vec, max_size: usize) -> Vec {
let mut lines = Vec::with_capacity(args.len() * 2);
let remainder: String = args.iter()
.fold(String::new(), |mut acc, arg| {
if !acc.is_empty() {
if (arg.chars().count() + 1) + acc.chars().count() , width: usize) -> String {
let total_length = lines.len() - 1;
let formatted_lines = lines
.iter()
.enumerate()
.map(|(idx, line)| {
let current_length = line.clone().chars().count();
let padding: String = (0..width - current_length).map(|_| ' ').collect();
let (start, end) = match idx {
0 => ('/', '\\'),
Solution
-
The tests fail. That's not a good thing:
-
The output is not as described or expected. I don't even see the code that would move the cow over...
-
There's a warning. Don't ignore warnings; fix them. One of the reasons to have a compiled language is to have the feedback from the compiler.
-
The code makes the assumption that a single
-
Splitting strings on Unicode code points may not make any sense. For example, if the input contains a letter with a combining diacritic, it doesn't make sense to split those on separate lines. Perhaps a combination of Unicode normalization and graphemes would help.
-
Accept a reference to a non-
-
Combining the above suggestion for
-
There's no need to specify a type on
-
Become familiar with the methods on
-
Once these methods are used,
-
If a function is documented for the consumer, use the Rustdoc comment (
-
The documentation for
-
The documentation demonstrates
-
The tests should show the case of a single string with multiple words; it doesn't appear that
-
Avoid duplicating function argument and return types and tests / examples in the function documentation when possible; code can't lie as easily as docs.
-
Remember that Rustdoc can contain code that is executed at compile time. This is often used for inline tests to ensure the examples don't rot.
-
There are no spaces between the function name and the parenthesis. Check out rustfmt.
-
-
There's an unneeded
-
Extract a
-
I'd normally use
-
There's no need to specify the type of the
-
Simplify the logic around checking
-
The
-
``
/// there is a one space buffer between args.
fn chunk_args(args: &[S], max_size: usize) -> Vec
where S: AsRef,
{
let mut li
The tests fail. That's not a good thing:
---- test_say_multi_line stdout ----
thread 'test_say_multi_line' panicked at 'assertion failed: (left == right)
(left: ,
right: " _______\n/ broke \\\n| n big |\n\\ bar /\n -------")', src/main.rs:153
---- test_say_single_line stdout ----
thread 'test_say_single_line' panicked at 'assertion failed: (left == right)
(left: ,
right: " _____________\n\n -------------")', src/main.rs:171
-
The output is not as described or expected. I don't even see the code that would move the cow over...
$ rsay moo a lot more than that --width=10
___________
/ moo a lot \
| more than |
\ that /
-----------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
-
There's a warning. Don't ignore warnings; fix them. One of the reasons to have a compiled language is to have the feedback from the compiler.
warning: unused import, #[warn(unused_imports)] on by default
use std::io::prelude::*;
^~~~~~~~~~~~~~~~~~~~
-
The code makes the assumption that a single
char takes a width of 1. Zero-width spaces would disagree, and there are probably code points wider than one.-
Splitting strings on Unicode code points may not make any sense. For example, if the input contains a letter with a combining diacritic, it doesn't make sense to split those on separate lines. Perhaps a combination of Unicode normalization and graphemes would help.
-
Accept a reference to a non-
Copy type unless the code directly benefits from transferring ownership. For example, accept &str instead of String and &[T] instead of a Vec. These are more flexible.-
Combining the above suggestion for
Vec is a bit more complicated; instead, the code can accept anything that can look like a &str using AsRef. Note how doing this simplifies tests and avoids extra allocation.-
There's no need to specify a type on
parse in parse_numeric; it's inferred from the function return type.-
Become familiar with the methods on
Option and Result.- Providing a default is
unwrap_or.
- Converting a value when it is present is
map.
- Combining these is
map_or.
- Chaining multiple
Options orResults isand_then.
- Panicking when the value is missing is
unwrapor even betterexpect.
-
Once these methods are used,
parse_numeric has questionable benefit. I'd inline it, which also avoids duplicating the default value.-
If a function is documented for the consumer, use the Rustdoc comment (
///). Documentation is Markdown, so use things like foo to highlight code and arguments consistently.-
The documentation for
chunk_args demonstrates that a "max width of 5" produces a 6-character string. And the "ng" of prolong is repeated.-
The documentation demonstrates
chunk_args is provided with a string, but it appears to be a Vec?-
The tests should show the case of a single string with multiple words; it doesn't appear that
rsay "moo alot more than that" --width=10 is handled well.-
Avoid duplicating function argument and return types and tests / examples in the function documentation when possible; code can't lie as easily as docs.
-
Remember that Rustdoc can contain code that is executed at compile time. This is often used for inline tests to ensure the examples don't rot.
-
There are no spaces between the function name and the parenthesis. Check out rustfmt.
-
total_length seems misnamed; should be something like last_index.-
There's an unneeded
clone when counting chars.-
Extract a
repeat function. I like this because it avoids the need to specify collecting to a String at all the call sites.-
I'd normally use
repeat instead of mapping a range, but I'm not sure that matters.-
There's no need to specify the type of the
Vec children when collecting the arguments; it can be inferred.-
Simplify the logic around checking
is_empty; it can just be an early exit.-
The
#[cfg(test)] isn't really doing anything; it's only applied to the next item. The next item happens to be a #[test] function anyway, which will automatically be removed when not compiling in test mode. Normally, the cfg attribute is applied to a test module.-
String and &str can be directly compared; there's no need to allocate.``
extern crate getopts;
use std::cmp;
use getopts::Options;
const PROGRAM_NAME: &'static str = "rsay";
const COW: &'static str = include_str!("cow.txt");
const DEFAULT_LINE_WIDTH: usize = 40;
fn print_usage(opts: Options) {
let brief = format!("Usage: {} [-OPTIONS] [ARG...]", PROGRAM_NAME);
print!("{}", opts.usage(&brief));
}
/// Ensure that lines have a maximum length of /// there is a one space buffer between args.
fn chunk_args(args: &[S], max_size: usize) -> Vec
where S: AsRef,
{
let mut li
Code Snippets
extern crate getopts;
use std::cmp;
use getopts::Options;
const PROGRAM_NAME: &'static str = "rsay";
const COW: &'static str = include_str!("cow.txt");
const DEFAULT_LINE_WIDTH: usize = 40;
fn print_usage(opts: Options) {
let brief = format!("Usage: {} [-OPTIONS] [ARG...]", PROGRAM_NAME);
print!("{}", opts.usage(&brief));
}
/// Ensure that lines have a maximum length of `max_size` and that
/// there is a one space buffer between args.
fn chunk_args<S>(args: &[S], max_size: usize) -> Vec<String>
where S: AsRef<str>,
{
let mut lines = Vec::with_capacity(args.len() * 2);
let remainder: String = args.iter()
.fold(String::new(), |mut acc, arg| {
let arg = arg.as_ref();
if !acc.is_empty() {
if (arg.chars().count() + 1) + acc.chars().count() <= max_size {
return acc + " " + arg;
} else {
lines.push(acc.clone());
acc.clear();
}
}
for c in arg.chars() {
acc.push(c);
if acc.chars().count() == max_size {
lines.push(acc.clone());
acc.clear();
}
}
acc
});
if !remainder.is_empty() {
lines.push(remainder);
}
lines
}
fn repeat(s: &str, len: usize) -> String {
::std::iter::repeat(s).take(len).collect()
}
/// Add the proper border to each line.
///
/// ["first", "mid", "last"] would become
///
/// / first \
/// | mid |
/// \ last /
fn multi_line<S>(lines: &[S], width: usize) -> String
where S: AsRef<str>,
{
let last_index = lines.len() - 1;
let formatted_lines = lines.iter()
.enumerate()
.map(|(idx, line)| {
let line = line.as_ref();
let current_length = line.chars().count();
let padding = repeat(" ", width - current_length);
let (start, end) = match idx {
0 => ('/', '\\'),
_ if idx == last_index => ('\\', '/'),
_ => ('|', '|'),
};
format!("{} {}{} {}\n", start, line, padding, end)
});
formatted_lines.collect()
}
fn say<S>(args: &[S], desired_width: usize) -> String
where S: AsRef<str>,
{
let chunks = chunk_args(args, desired_width);
let largest_str = chunks.iter().map(|x| x.chars().count()).max();
let width = largest_str.map_or(desired_width, |x| cmp::min(desired_width, x));
let formatted = match chunks.len() {
1 => format!("< {} >\n", chunks.join(" ")),
_ => multi_line(&chunks, width),
};
let top_border = repeat("_", width + 2);
let bottom_border = repeat("-", width + 2);
format!(" {}\n{} {}", top_border, formatted, bottom_border)
}
fn main() {
let args: Vec<_> = std::env::args()
.skip(1)
.collect();
let mut opts = Options::new();
opts.optflag("h", "help", "Print this help menu");
Context
StackExchange Code Review Q#135796, answer score: 9
Revisions (0)
No revisions yet.