8
\$\begingroup\$

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:

  1. To get better at idiomatic Rust (this is the first thing I've written in the language)
  2. Improve the implementation of the chunk_args and multi_line functions (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::<usize>() {
        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 `max_width` of five, the phrase "prolong a hat bear"
// should be split like so:
// ["prolon", "ng a", "hat", "bear"]
fn chunk_args (args: Vec<String>, max_size: usize) -> Vec<String> {
    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() <= 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
}

// Add the proper border to each line
// ["first", "mid", "last"]
//
// / first \
// | mid   |
// \ last  /
fn multi_line (lines: Vec<String>, 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 => ('/', '\\'),
                _ if idx == total_length => ('\\', '/'),
                _ => ('|', '|'),
            };

            format!("{} {}{} {}\n", start, line, padding, end)
        });

    formatted_lines.collect()
}

fn say (args: Vec<String>, desired_width: usize) -> String {
    let chunks = chunk_args(args, desired_width);
    let largest_str = chunks.iter().map(|x| x.chars().count()).max();
    let width = match largest_str {
        Some(x) => { cmp::min(desired_width, x) },
        _ => { desired_width }
    };
    let formatted = match chunks.len() {
        1 => format!("< {} >\n", chunks.join(" ")),
        _ => multi_line(chunks, width),
    };
    let top_border: String = (0..width + 2).map(|_| "_").collect();
    let bottom_border: String = (0..width + 2).map(|_| "-").collect();

    format!(" {}\n{} {}", top_border, formatted, bottom_border)
}

fn main () {
    let args: Vec<String> = std::env::args()
        .skip(1)
        .collect();
    let mut opts = Options::new();

    opts.optflag("h", "help", "Print this help menu");
    opts.optmulti("W", "width", "Width of output", "50");

    let matches = match opts.parse(&args) {
        Ok(m) => { m }
        Err(f) => { panic!(f.to_string()) }
    };

    if matches.opt_present("h") {
        print_usage(opts);
        return;
    }

    let width = match matches.opt_str("W") {
        None => { DEFAULT_LINE_WIDTH },
        Some(w) => { parse_numeric(w, DEFAULT_LINE_WIDTH) }
    };

    let input = if !matches.free.is_empty() {
        matches.free
    } else {
        print_usage(opts);
        return;
    };

    print!("{}\n{}", say(input, width), COW);
}

Small testset:

#[cfg(test)]
#[test]
fn test_chunk_args_padding () {
    let phrase = ["broken", "big", "bar"].iter().map(|&x| x.into()).collect();
    let result = chunk_args(phrase, 5);
    assert_eq!(vec!["broke".to_string(), "n big".into(), "bar".into()], result);
}

#[test]
fn test_say_multi_line () {
    let args = ["broke", "n big", "bar"].iter().map(|&x| x.into()).collect();
    let result = say(args, 5);
    let expected: String = r" _______
/ broke \
| n big |
\ bar   /
-------".into();

    assert_eq!(expected, result);
}

#[test]
fn test_say_multi_line_wide () {
    let phrase = "aggregate rotor hat".split(" ").map(|x| x.into()).collect();
    let result = chunk_args(phrase, 10);
    assert_eq!(vec!["aggregate", "rotor hat"], result);
}

#[test]
fn test_say_single_line () {
    let args = ["foo", "bar", "baz"].iter().map(|&x| x.into()).collect();
    let result = say(args, 40);
    let expected: String = r" _____________
< foo bar baz >
-------------".into();

    assert_eq!(expected, result);
}

cow.txt


\   ^__^
 \  (oo)\_______
    (__)\       )\/\
        ||----w |
        ||     ||


Output

$ rsay hello world
 _____________                     
< hello world >                    
 -------------                     
       \   ^__^                   
        \  (oo)\_______           
           (__)\       )\/\       
               ||----w |          
               ||     ||          
\$\endgroup\$
2

1 Answer 1

9
\$\begingroup\$
  1. 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: `" _______\n/ broke \\\n| n big |\n\\ bar   /\n-------"`,
            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: `" _____________\n< foo bar baz >\n-------------"`,
            right: `" _____________\n< foo bar baz >\n -------------"`)', src/main.rs:171
    
  2. 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 |
            ||     ||
    
  3. 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::*;
        ^~~~~~~~~~~~~~~~~~~~
    
  4. 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.

  5. 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.

  6. 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<T>. These are more flexible.

  7. Combining the above suggestion for Vec<String> is a bit more complicated; instead, the code can accept anything that can look like a &str using AsRef<str>. Note how doing this simplifies tests and avoids extra allocation.

  8. There's no need to specify a type on parse in parse_numeric; it's inferred from the function return type.

  9. Become familiar with the methods on Option and Result.

    1. Providing a default is unwrap_or.
    2. Converting a value when it is present is map.
    3. Combining these is map_or.
    4. Chaining multiple Options or Results is and_then.
    5. Panicking when the value is missing is unwrap or even better expect.
  10. Once these methods are used, parse_numeric has questionable benefit. I'd inline it, which also avoids duplicating the default value.

  11. 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.

  12. The documentation for chunk_args demonstrates that a "max width of 5" produces a 6-character string. And the "ng" of prolong is repeated.

  13. The documentation demonstrates chunk_args is provided with a string, but it appears to be a Vec<String>?

  14. 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.

  15. Avoid duplicating function argument and return types and tests / examples in the function documentation when possible; code can't lie as easily as docs.

  16. 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.

  17. There are no spaces between the function name and the parenthesis. Check out rustfmt.

  18. total_length seems misnamed; should be something like last_index.

  19. There's an unneeded clone when counting chars.

  20. Extract a repeat function. I like this because it avoids the need to specify collecting to a String at all the call sites.

  21. I'd normally use repeat instead of mapping a range, but I'm not sure that matters.

  22. There's no need to specify the type of the Vec children when collecting the arguments; it can be inferred.

  23. Simplify the logic around checking is_empty; it can just be an early exit.

  24. 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.

  25. 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 `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");
    opts.optmulti("W", "width", "Width of output", "50");

    let matches = opts.parse(&args).expect("Unable to parse arguments");

    if matches.opt_present("h") {
        print_usage(opts);
        return;
    }

    let width = matches.opt_str("W")
        .and_then(|w| w.parse().ok())
        .unwrap_or(DEFAULT_LINE_WIDTH);

    if matches.free.is_empty() {
        print_usage(opts);
        return;
    };

    let input = matches.free;

    print!("{}\n{}", say(&input, width), COW);
}

#[cfg(test)]
mod test {
    use super::{chunk_args, say};

    #[test]
    fn test_chunk_args_padding() {
        let phrase = &["broken", "big", "bar"];
        let result = chunk_args(phrase, 5);
        assert_eq!(vec!["broke".to_string(), "n big".into(), "bar".into()], result);
    }

    #[test]
    fn test_say_multi_line() {
        let args = &["broke", "n big", "bar"];
        let result = say(args, 5);
        let expected = r" _______
/ broke \
| n big |
\ bar   /
 -------";

        assert_eq!(expected, result);
    }

    #[test]
    fn test_say_multi_line_wide() {
        let phrase = &["aggregate", "rotor", "hat"];
        let result = chunk_args(phrase, 10);
        assert_eq!(vec!["aggregate", "rotor hat"], result);
    }

    #[test]
    fn test_say_single_line() {
        let args = &["foo", "bar", "baz"];
        let result = say(args, 40);
        let expected = r" _____________
< foo bar baz >
 -------------";

        assert_eq!(expected, result);
    }
}

I don't see the compiler warning when cargo building on OS X; I do see it when building on Debian. How do I avoid that?

That's strange, as I experienced the error on OS X. Perhaps there are differing Rust versions on the two machines?

\$\endgroup\$
2
  • \$\begingroup\$ Wow! Thanks very much for your comprehensive answer! The tests and the documentation typos are entirely my fault for being sloppy when formatting this from the Github repo into Code Review. I'm not sure if it's okay for me to edit the question to fix them up but I definitely can if that'd be helpful. Two questions: 1. what's the best way for us to correspond if I want clarification (i'm assuming comments but don't want to ) 2. I don't see the compiler warning when cargo building on OSX; (checking, I do sit it when doing so on debian). How do I avoid that? \$\endgroup\$ Commented Jul 27, 2016 at 1:07
  • \$\begingroup\$ @NickTomlin okay for me to edit the question — the general rule is not if it would invalidate the answer. best way for us to correspond — yes, here is the appropriate place. Remember that Stack Exchange isn't about the question asker, it's about the (hopefully) hundreds or thousands of people that come afterwards. I'll take your comment / question, lightly edit it, and reply to it in the answer - to make it easier for others to see. Then I'll ping you via a comment that I'll eventually delete after a while. Keeps things tidy. \$\endgroup\$
    – Shepmaster
    Commented Jul 27, 2016 at 2:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.