5
\$\begingroup\$

This is a minimal reimplemtation of wc. It supports only stdin at the moment and no command line arguments, that's for a later version.

This is my first complete Rust program/package, so I'm interested in any comments, including but not limited to:

  • documentation,
  • comments,
  • general style and types,
  • tests,
  • any other remark.

Cargo.toml

[package]
name = "wc"
version = "0.1.0"

[dependencies]

src/lib.rs (on playground)

use std::io::Read;

/// The statistics returned by `wordcount`.
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub struct WordCountStats {
    /// number of bytes in the input
    pub bytes: usize,
    /// number of groups of consecutive non-whitespace characters
    pub words: usize,
    /// number of newline characters (`\n`)
    pub newlines: usize,
}

/// Returns the word count statistics of the given `reader`.
///
/// ```
/// use wc::{wordcount,WordCountStats};
///
/// assert_eq!(
///    wordcount("Hello, World!".as_bytes()).unwrap(),
///    WordCountStats {
///        bytes: 13,
///        words: 2,
///        newlines: 0,
///    }
/// );
/// ```
///
/// The statistics follow `wc` (`man 1 wc`) output:
///
/// * bytes is always the number of bytes (not utf8 characters or similar)
/// * words is the number of positive length consecutive non-whitespace runs
/// * newlines is the number of newlines (NOT the number of lines)
///
/// `wordcount` uses `bytes()` internally and tries not to
/// add any buffering to the `reader`. If you use an unbuffered
/// device, consider using `BufRead` around your content.
///
/// # Errors
/// If a `byte` couldn't get read you will get a `Err(std::io::Error)`.
/// This can happen if the socket disconnects suddenly, a filesystem
/// error occurred, or your scanner couldn't continue to read the stripes
/// from your cat.
pub fn wordcount<R>(reader: R) -> std::io::Result<WordCountStats>
where
    R: Read,
{
    let mut bytes = 0;
    let mut words = 0;
    let mut newlines = 0;
    let mut spacemode = true;

    for byte in reader.bytes() {
        bytes += 1;

        let c = byte?;

        if (c as char).is_whitespace() {
            spacemode = true
        } else if spacemode {
            // A non-whitespace character after a whitespace character sequence.
            words += 1;
            spacemode = false
        }

        if c as char == '\n' {
            newlines += 1
        }
    }

    Ok(WordCountStats {
        bytes,
        words,
        newlines,
    })
}

#[cfg(test)]
mod tests {
    use WordCountStats;
    fn wc_string(input: &str) -> ::WordCountStats {
        ::wordcount(input.as_bytes()).unwrap()
    }
    #[test]
    fn empty_input() {
        assert_eq!(
            wc_string(""),
            WordCountStats {
                bytes: 0,
                words: 0,
                newlines: 0,
            }
        )
    }
    #[test]
    fn single_letter_input() {
        assert_eq!(
            wc_string("a"),
            WordCountStats {
                bytes: 1,
                words: 1,
                newlines: 0,
            }
        )
    }
    #[test]
    fn single_space_input() {
        assert_eq!(
            wc_string(" "),
            WordCountStats {
                bytes: 1,
                words: 0,
                newlines: 0,
            }
        )
    }
    #[test]
    fn two_letters_separated_by_spaces() {
        assert_eq!(
            wc_string("a \t b"),
            WordCountStats {
                bytes: 5,
                words: 2,
                newlines: 0,
            }
        )
    }
    #[test]
    fn two_line_input() {
        assert_eq!(
            wc_string("\n"),
            WordCountStats {
                bytes: 1,
                words: 0,
                newlines: 1,
            }
        )
    }

    #[test]
    fn complicated_input() {
        assert_eq!(
            wc_string("Hello, World!\nHow are you today?\nI hope you're fine!"),
            WordCountStats {
                bytes: 52,
                words: 10,
                newlines: 2,
            }
        )
    }
}

src/bin/main.rs

This is only a stub main to check the library. I will expand it in a later version. You can review it, but I haven't really focused on it.

extern crate wc;

fn main() {
    let filename = "-";
    let stdin = std::io::stdin();
    let handle = stdin.lock();
    match wc::wordcount(handle) {
        Err(e) => eprintln!("{}: {}", filename, e),
        Ok(wc::WordCountStats {
            bytes,
            words,
            newlines,
        }) => println!("{:8} {:8} {:12} {}", newlines, words, bytes, filename),
    }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

For such a program, the definition of whitespace is crucial. Your code does not document what it means by whitespace. Inspecting the code shows that, since you iterate by bytes, you presumably are not handling any of the more interesting Unicode characters:

printf %b '\u200b \u200b \u200b \u200b \u200b' | cargo run
   0        5           34 -

I'd probably derive Default for the struct, call it, then mutate the fields of the struct in-place in the function, instead of having 3 separate variables:

It feels really strange to not immediately perform error checking inside the for loop. I'd move the ? to the first expression. I'd also shadow the name byte because the name c makes it sound like a char, which it isn't. This frees up the variable name c to be the type-casted version.

I'd move the free function to an associated function of WordCountStats.

Use more vertical spacing in your tests. Vertical spaces help our eyes easily pick out different related sections from each other.

Since you imported WordCountStats in your tests, you don't need to path-prefix it with ::.

I'm a fan of the small test utility function you've added.

use std::io::Read;

/// The statistics returned by `WordCountStats::from_reader`.
#[derive(Debug, PartialEq, Eq, Clone, Copy, Default)]
pub struct WordCountStats {
    /// number of bytes in the input
    pub bytes: usize,
    /// number of groups of consecutive non-whitespace characters
    pub words: usize,
    /// number of newline characters (`\n`)
    pub newlines: usize,
}

impl WordCountStats {
    /// Returns the word count statistics of the given `reader`.
    ///
    /// ```
    /// use wc::WordCountStats;
    ///
    /// assert_eq!(
    ///    WordCountStats::from_reader("Hello, World!".as_bytes()).unwrap(),
    ///    WordCountStats {
    ///        bytes: 13,
    ///        words: 2,
    ///        newlines: 0,
    ///    }
    /// );
    /// ```
    ///
    /// The statistics follow `wc` (`man 1 wc`) output:
    ///
    /// * bytes is always the number of bytes (not utf8 characters or similar)
    /// * words is the number of positive length consecutive non-whitespace runs
    /// * newlines is the number of newlines (NOT the number of lines)
    ///
    /// `WordCountStats::from_reader` uses `bytes()` internally and tries not to
    /// add any buffering to the `reader`. If you use an unbuffered
    /// device, consider using `BufRead` around your content.
    ///
    /// # Errors
    /// If a `byte` couldn't get read you will get a `Err(std::io::Error)`.
    /// This can happen if the socket disconnects suddenly, a filesystem
    /// error occurred, or your scanner couldn't continue to read the stripes
    /// from your cat.
    pub fn from_reader<R>(reader: R) -> std::io::Result<WordCountStats>
    where
        R: Read,
    {
        let mut stats = WordCountStats::default();
        let mut spacemode = true;

        for byte in reader.bytes() {
            let byte = byte?;
            let c = byte as char;

            stats.bytes += 1;

            if c.is_whitespace() {
                spacemode = true
            } else if spacemode {
                // A non-whitespace character after a whitespace character sequence.
                stats.words += 1;
                spacemode = false
            }

            if c == '\n' {
                stats.newlines += 1
            }
        }

        Ok(stats)
    }
}

#[cfg(test)]
mod tests {
    use WordCountStats;

    fn wc_string(input: &str) -> WordCountStats {
        ::WordCountStats::from_reader(input.as_bytes()).unwrap()
    }

    #[test]
    fn empty_input() {
        assert_eq!(
            wc_string(""),
            WordCountStats {
                bytes: 0,
                words: 0,
                newlines: 0,
            }
        )
    }

    #[test]
    fn single_letter_input() {
        assert_eq!(
            wc_string("a"),
            WordCountStats {
                bytes: 1,
                words: 1,
                newlines: 0,
            }
        )
    }

    #[test]
    fn single_space_input() {
        assert_eq!(
            wc_string(" "),
            WordCountStats {
                bytes: 1,
                words: 0,
                newlines: 0,
            }
        )
    }

    #[test]
    fn two_letters_separated_by_spaces() {
        assert_eq!(
            wc_string("a \t b"),
            WordCountStats {
                bytes: 5,
                words: 2,
                newlines: 0,
            }
        )
    }

    #[test]
    fn two_line_input() {
        assert_eq!(
            wc_string("\n"),
            WordCountStats {
                bytes: 1,
                words: 0,
                newlines: 1,
            }
        )
    }

    #[test]
    fn complicated_input() {
        assert_eq!(
            wc_string("Hello, World!\nHow are you today?\nI hope you're fine!"),
            WordCountStats {
                bytes: 52,
                words: 10,
                newlines: 2,
            }
        )
    }
}

The real test cases were therefore whether cargo run returns the same as wc

Make that a test, then. I like using quickcheck:

use std::{
    io::Write,
    process::{Command, Stdio},
};

fn wc_program(bytes: &[u8]) -> WordCountStats {
    let mut child = Command::new("wc")
        .stdin(Stdio::piped())
        .stdout(Stdio::piped())
        .spawn()
        .unwrap();

    child.stdin.as_mut().unwrap().write_all(bytes).unwrap();

    let out = child.wait_with_output().unwrap();
    let out = String::from_utf8(out.stdout).unwrap();
    let mut nums = out.split_whitespace().map(|n| n.parse::<usize>().unwrap()).fuse();

    WordCountStats {
        newlines: nums.next().unwrap(),
        words: nums.next().unwrap(),
        bytes: nums.next().unwrap(),
    }
}

quickcheck! {
    fn prop(xs: Vec<u8>) -> bool {
        let me = WordCountStats::from_reader(&xs[..]).unwrap();
        let them = wc_program(&xs);
        me == them
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the review! Ad "since you iterate by bytes": it's a re-implementation of wc, which only uses bytes too. Whether a byte is considered whitespace depends on the locale, and as far as I know char::is_whitespace is Rust's equivalent of C's isspace(), which should be used by implementations. The real test cases were therefore whether cargo run returns the same as wc. That detail should get added to the documentation, though. \$\endgroup\$
    – Zeta
    Apr 15, 2018 at 7:58
  • \$\begingroup\$ @Zeta and you have to decide if you are going to force people to read wc's man page before being able to use your code or not. I've added a way of addressing your "real test" comment though. \$\endgroup\$
    – Shepmaster
    Apr 15, 2018 at 13:52
  • \$\begingroup\$ That's why I said that I should add the behaviour to the documentation (if this was something I'd publish), as it's currently hidden too much. TIL that there's quickcheck in Rust too, thanks! \$\endgroup\$
    – Zeta
    Apr 15, 2018 at 15:06

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.