Cat clone with unit tests

Question

This is a clone of the cat utility. The goal of this exercise was to get comfortable doing basic tasks in a new language, figure out how to set up testing, etc. So I'm really looking for feedback on writing more idiomatic Rust, if there are better ways to approach things, etc. In particular, my TextFixture class I use to manage lifetimes so I can easily create a Cat object and retreive the output streams, seems like not the best way to do things.Conversely, issues like "numbering will be broken if you have more than 1,000 lines" are not very important to me for this exercise.

use std::fs::File;
use std::io;
use std::io::{BufRead, BufReader};
use std::path::PathBuf;
use std::str;
use structopt::StructOpt;

#[derive(Debug, StructOpt)]
#[structopt(
    name = "cat",
    about = "concatenate files and print on the standard output"
)]
struct CatConfig {
    /// Input file
    #[structopt(name = "FILE", parse(from_os_str))]
    input: Vec<PathBuf>,

    /// display $ at the end of each line
    #[structopt(short = "E", long = "show-ends")]
    show_ends: bool,

    /// display TAB characters as ^I
    #[structopt(short = "T", long = "show-tabs")]
    show_tabs: bool,

    /// number all output lines
    #[structopt(short = "n", long = "number")]
    number: bool,
}

impl Default for CatConfig {
    fn default() -> CatConfig {
        CatConfig {
            input: vec![],
            show_tabs: false,
            show_ends: false,
            number: false,
        }
    }
}


struct Cat<'a> {
    stdout: &'a mut dyn io::Write,
    stderr: &'a mut dyn io::Write,
}

impl Cat<'_> {
    fn new<'a>(stdout: &'a mut dyn io::Write, stderr: &'a mut dyn io::Write) -> Cat<'a> {
        Cat { stdout, stderr }
    }

    fn cat_all(&mut self, config: CatConfig) -> () {
        for path in &config.input {
            match File::open(path) {
                Ok(file) => {
                    let mut input = BufReader::new(file);
                    self.cat(&config, &mut input);
                }
                Err(_) => {
                    let filename = path.to_str().unwrap();
                    self.stderr.write(filename.as_bytes()).unwrap();
                    self.stderr.write(b": No such file or directory\n").unwrap();
                }
            }
        }
    }

    fn cat(&mut self, config: &CatConfig, input: &mut dyn BufRead) -> () {
        let mut line_number = 1;
        for line in input.lines() {
            let string_line = line.unwrap();
            let line_ending: &str = if config.show_ends { "$\n" } else { "\n" };
            let mut line_to_write = [string_line, line_ending.to_string()].concat();

            if config.show_tabs {
                line_to_write = line_to_write.replace("\t", "^I");
            }

            if config.number {
                let number_string = line_number.to_string();
                line_to_write = ["  ", &number_string, "  ", &line_to_write].concat();
            }

            line_number = line_number + 1;

            self.stdout.write(&line_to_write.as_bytes()).unwrap();
        }
    }
}


fn main() -> () {
    let opt = CatConfig::from_args();
    Cat::new(&mut io::stdout(), &mut io::stderr()).cat_all(opt);
}

#[cfg(test)]
mod tests {
    use super::*;
    use std::io::Write;
    use std::path::Path;
    use tempfile::NamedTempFile;

    struct TestFixture {
        stdout: io::Cursor<Vec<u8>>,
        stderr: io::Cursor<Vec<u8>>,
    }

    impl TestFixture {
        fn new() -> TestFixture {
            let stdout = io::Cursor::new(Vec::new());
            let stderr = io::Cursor::new(Vec::new());
            TestFixture { stdout, stderr }
        }

        fn make_cat(&mut self) -> Cat {
            return Cat {
                stdout: &mut self.stdout,
                stderr: &mut self.stderr,
            };
        }

        fn get_output(&self) -> String {
            let clone = self.stdout.clone();
            let output_vec = clone.into_inner();
            String::from_utf8(output_vec).unwrap()
        }

        fn get_error(&self) -> String {
            let clone = self.stderr.clone();
            let output_vec = clone.into_inner();
            String::from_utf8(output_vec).unwrap()
        }
    }

    #[test]
    fn cat_with_no_options() {
        let mut source = b"abc\ndef\n" as &[u8];
        let expected_output = "abc\ndef\n";
        let config = CatConfig::default();

        check_cat_output(&mut source, expected_output, config);
    }

    #[test]
    fn cat_from_nonexistant_file() {
        let mut fixture = TestFixture::new();
        let mut catter = fixture.make_cat();

        let file = Path::new("file_does_not_exist.txt");
        let config = CatConfig {
            input: vec![file.to_path_buf()],
            ..Default::default()
        };

        catter.cat_all(config);

        let expected_error = "file_does_not_exist.txt: No such file or directory\n";
        assert_eq!(expected_error, fixture.get_error());
    }

    #[test]
    fn cat_from_file() {
        let mut fixture = TestFixture::new();
        let mut catter = fixture.make_cat();

        let mut file = NamedTempFile::new().unwrap();
        writeln!(file, "File contents").unwrap();
        let config = CatConfig {
            input: vec![file.path().to_path_buf()],
            ..Default::default()
        };
        catter.cat_all(config);

        assert_eq!("File contents\n", fixture.get_output());
    }

    #[test]
    fn test_cat_shows_ends() {
        let mut source = b"abc\ndef\n" as &[u8];
        let expected_output = "abc$\ndef$\n";

        let config = CatConfig {
            show_ends: true,
            ..Default::default()
        };

        check_cat_output(&mut source, expected_output, config);
    }

    #[test]
    fn test_cat_shows_tabs() {
        let mut source = b"abc\tdef\n" as &[u8];
        let expected_output = "abc^Idef\n";

        let config = CatConfig {
            show_tabs: true,
            ..Default::default()
        };

        check_cat_output(&mut source, expected_output, config);
    }

    fn check_cat_output(source: &mut dyn BufRead, expected_output: &str, config: CatConfig) {
        let mut fixture = TestFixture::new();
        let mut catter = fixture.make_cat();

        catter.cat(&config, source);

        assert_eq!(expected_output, fixture.get_output());
    }

    #[test]
    fn test_cat_number_lines() {
        let mut source = b"abc\ndef\n" as &[u8];
        let expected_output = "  1  abc\n  2  def\n";

        let config = CatConfig {
            number: true,
            ..Default::default()
        };

        check_cat_output(&mut source, expected_output, config);
    }
}

Caleb Stanford · Accepted Answer · 2021-01-30 00:07:05Z

First things first -- run cargo clippy! Generally speaking its suggestions are quite helpful. In this case clippy raises some errors because you are using write instead of write_all to write an entire buffer, which doesn't necessarily work. This appears to be a serious error; see this page for an explanation.
Also caught by clippy: functions don't need -> () (if there's no return type, then () is implicit).
The use of structopt is very clean and natural and fits the task, as well as implementing Default for your CLI arguments for convenient initialization.
A stylistic point: your code is littered with &'a mut dyn Trait objects everywhere. Some Rust programmers write code like this, so I wouldn't say it's necessarily wrong, but I usually recommend using generics when possible because this avoids dynamic overhead at runtime. Instead you get a copy of your function for the actual concrete type that you need it, like with C++ templates. For consideration, here is what your Cat struct looks like using generics instead:
```
struct Cat<'a, F1, F2>
where
    F1: io::Write,
    F2: io::Write,
{
    stdout: &'a mut F1,
    stderr: &'a mut F2,
}

impl<'a, F1, F2> Cat<'a, F1, F2>
where
    F1: io::Write,
    F2: io::Write,
{
    fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
        Cat { stdout, stderr }
    }

...
```
and in your TestFixture:
```
fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
    return Cat::new(&mut self.stdout, &mut self.stderr);
}
```
You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {
You are overusing .unwrap() -- fine for a toy project, but not really fine if you wanted to ship this as the real cat utility; it would occasionally fail and panic with no explanation. Since this is a binary, the easiest fix is just to use .expect("Explain what bad happened here") instead of unwrap -- this should be propagated back to the user. For a library you would want to return Result<(), String> or similar for all your functions so as to make the failure model completely explicit.
In the fn cat function, you are manually incrementing a counter; consider the alternative
```
for (line_number, line) in input.lines().enumerate() {
```
If you do this though, you get numbering starting from zero. So to get numbering starting from 1: let number_string = (line_number + 1).to_string()
I like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:
```
fn collect(self) -> (String, String) {
    let out = String::from_utf8(self.stdout.into_inner())
        .expect("invalid UTF8 found in stdout!");
    let err = String::from_utf8(self.stderr.into_inner())
        .expect("invalid UTF8 found in stderr!");
    (out, err)
}
```
Then use it like: assert_eq!(expected_error, fixture.collect().1).
Finally, I found it counterintuitive that running the binary with no arguments prints nothing. Printing the help would be more useful, and there is probably a way to configure structopt / your main function to have that behavior.

Stack Exchange Network

Cat clone with unit tests

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Cat clone with unit tests

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions