2
\$\begingroup\$

As part of my journey in learning the Rust programming language, I decided to make a miniature cat clone (catty) in it. The following is my code, which depends on clap for argument parsing (see below). It currently only supports concatenating 1 file with possibly numbered lines (-n/--number). I tried to be as close as possible to the actual cat program for this:

#[macro_use] extern crate clap;

use std::{io, error, env, fs::read_to_string, path::PathBuf, process};

fn main() {
    process::exit(
        if let Err(err) = cli(env::args().collect::<Vec<_>>()) {
            // CLI parsing errors
            if let Some(clap_err) = err.downcast_ref::<clap::Error>() {
                eprint!("{}", clap_err);
            } else {
                eprintln!("{}", err);
            }
            1
        } else {
            0
        }
    );
}

fn cli(cli_args: Vec<String>) -> Result<(), Box<error::Error>> {
    let matches = clap::App::new("catty")
        .version(crate_version!())
        .about("A minimal clone of the linux utility cat.")
        .arg(clap::Arg::with_name("FILE")
            .help("The file to concatenate to standard output")
            .required(true))
        .arg(clap::Arg::with_name("number")
            .short("n")
            .long("number")
            .help("Numbers all output lines"))
        .get_matches_from_safe(cli_args)?;

    let file_contents = get_file_contents(matches.value_of("FILE").unwrap())?;
    let file_contents: Vec<&str> = file_contents.split("\n").collect();

    let number_lines = matches.is_present("number");

    for (i, line) in file_contents.iter().enumerate() {
        let formatted_line = if number_lines {
            format!("{:>6}  {}", i + 1, line)
        } else {
            line.to_string()
        };

        if i == file_contents.len() - 1 && line.len() > 0 {
            print!("{}", formatted_line);
        } else if !(i == file_contents.len() - 1 && line.len() == 0) {
            println!("{}", formatted_line);
        }
    }

    Ok(())
}

fn get_file_contents(passed_argument: &str) -> Result<String, Box<error::Error>> {
    let mut resolved_path = PathBuf::from(passed_argument);

    if !resolved_path.exists() || !resolved_path.is_file() {
        resolved_path = PathBuf::from(env::current_dir()?);
        resolved_path.push(passed_argument);

        if !resolved_path.exists() || !resolved_path.is_file() {
            return Err(io::Error::new(io::ErrorKind::NotFound, "The passed file is either not a file or does not exist!").into());
        }
    }

    Ok(read_to_string(resolved_path)?)
}

My Cargo.toml is as follows:

[package]
name = "catty"
version = "0.1.0"
authors = ["My Name <my@email.com>"]

[dependencies]

[dependencies.clap]
version = "2.32"
default-features = false
features = ["suggestions"]

Here is what I want to know from this code review:

  1. Is my code idiomatic rust (i.e good error handling, not overly verbose, etc)?
  2. Is my code performant or can it be improved in some way?
\$\endgroup\$
2
\$\begingroup\$

4: use std::{env, error, fs::read_to_string, io, path::PathBuf, process};

Just a personal taste, but I would do

use std::{env, error, io, process};
use std::fs::read_to_string;
use std::path::PathBuf;

Yes, nested includes are nice and easy, but hard to extend. It's up to you.


35: let file_contents: Vec<&str> = file_contents.split('\n').collect();

I would suggest using lines instead.

Also, you can omit &str or change the line completly.

Either

let file_contents: Vec<_> = file_contents.lines().collect();

or

let file_contents = file_contents.lines().collect::<Vec<_>>();

46/48: line.len() > 0 / line.len() == 0

Replace that by !line.is_empty() and line.is_empty().


60: resolved_path = PathBuf::from(env::current_dir()?);

Remove PathBuf::from completly, because current_dir is already a PathBuf

\$\endgroup\$
  • \$\begingroup\$ Also, is there an equivalent of enumerate for an iterator (so I can avoid collecting into a vector)? \$\endgroup\$ – Arnav Borborah Nov 26 '18 at 20:31
  • 1
    \$\begingroup\$ Yes, you can call enumerate on every iterator. The thing is, that you don't know the length of your iterator, which is the reason why I didn't mentioned it. If you are willing to use println! every iteration, you can simplify your code/loop a lot. \$\endgroup\$ – hellow Nov 27 '18 at 7:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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