7
\$\begingroup\$

I am new to Rust, began learning a few days ago. I have written a simple csv_sorter based off of one I had written for a class previously. Everything runs fine and I have gotten my expected results. I do not know all of the Rust conventions, and am unsure what I may be doing wrong, or what can be done differently.

Would anyone be willing to review my code? Please point out any bad design, poor optimizations, or alternative ideas. (Note, I still want to follow the structure of file > struct > list > output https://github.com/HammerAPI/rustcode/tree/master/csv_sorter

use std::fs::File;
use std::io::{BufRead, BufReader, Write};
use std::process;


// Person struct to hold relevant data
#[derive(Debug)]
struct Person {
    first_name: String,
    last_name: String,
    street: String,
    city: String,
    state: String,
    zip_code: String,
}

// Person constructor
impl Person {
    fn new(first_name: String, last_name: String,
            street: String, city: String, state: String,
            zip_code: String) -> Person {

        Person {
            first_name,
            last_name,
            street,
            city,
            state,
            zip_code,
        }
    }
}




/**
 * Processes command-line arguments
 *
 * # Description
 * This function processes the passed-in command line arguments and attempts
 * to open and create valid input/output files from the names given.
 *
 * # Arguments
 * * `args` - A string array of command-line arguments.
 *
 * # Returns
 * * A tuple of the input file and output file if they are found, else errors.
 */
fn arg_parser(args: &[String]) -> Result<(File, File), &'static str> {

    // Exit if too many or too few arguments were passed
    if args.len() != 3 {
        return Err("Usage: 'cargo run [input file] [output file]");
    }

    // Get the input file
    let input_file = match File::open(format!("{}{}", "src/", &args[1])) {
        Err(why) => panic!("\ncouldn't open file: {}", why),
        Ok(file) => file,
    };

    // Get the output file
    let output_file = match File::create(format!("{}{}", "src/", &args[2])) {
        Err(why) => panic!("\ncouldn't create file: {}", why),
        Ok(file) => file,
    };

    // Return both files as a tuple
    Ok((input_file, output_file))
}




/**
 * Builds a list of Person structs
 *
 * # Description
 * This function reads the input file line by line and creates a Person
 * struct based on the line's contents. It then adds that struct to a vector
 * and repeats for every line in the file. The final vector contains every
 * Person struct read in from the file.
 *
 * # Arguments
 * * `input_file` - The input file to read from.
 *
 * # Returns
 * * A vector of type Person containing all Person structs from the file.
 */
fn build_person_vec(input_file: &mut File) -> Vec<Person> {

    let mut person_vec: Vec<Person> = Vec::new();
    let reader = BufReader::new(input_file);

    for line in reader.lines() {

        let line = line.unwrap();

        let data: Vec<&str> = line.split(", ").collect();

        let p = Person::new(String::from(data[0].trim()),
                            String::from(data[1].trim()),
                            String::from(data[2].trim()),
                            String::from(data[3].trim()),
                            String::from(data[4].trim()),
                            String::from(data[5].trim()));
        person_vec.push(p);
    }
    person_vec
}




/**
 * Sorts the list of Person structs
 *
 * # Description
 * Sorts via Selection Sort.
 *
 * # Arguments
 * * `person_vec` - A vector containing Person structs.
 */
fn sort_person_vec(person_vec: &mut Vec<Person>) {

    for i in 0..person_vec.len() {

        let mut lowest = i;

        for j in (i + 1)..person_vec.len() {

            // Temporary variables to hold first and last names
            let j_last = &person_vec[j].last_name.to_lowercase();
            let j_first = &person_vec[j].first_name.to_lowercase();
            let low_last = &person_vec[lowest].last_name.to_lowercase();
            let low_first = &person_vec[lowest].first_name.to_lowercase();

            // Swap by last name or first name if last names are equal
            if (j_last < low_last) || (j_last == low_last && j_first < low_first){
                lowest = j;
            }
        }
        person_vec.swap(lowest, i);
    }
}




/**
 * Writes data to the output file
 *
 * # Description
 * Writes all Person structs to the output file, catching errors if the file
 * is not available to be written to.
 *
 * # Arguments
 * * `person_vec` - A vector containing Person structs.
 * * `output_file` - The file to write to.
 */
fn write_to_file(person_vec: &mut Vec<Person>, output_file: &mut File) {

    for p in person_vec {

        // Format the peron's information as a string
        let info = format!("{}, {}, {}, {}, {}, {}\n",
            p.first_name, p.last_name, p.street, p.city,
            p.state, p.zip_code);

        // Write to output file
        match output_file.write_all(info.as_bytes()) {
            Err(why) => panic!("\ncouldn't write to file: {}", why),
            Ok(_) => (),
        }
    }
}




fn main() {

    let args: Vec<String> = env::args().collect();

    // Get the input and output files
    let (mut input_file, mut output_file) = arg_parser(&args).unwrap_or_else(|err| {
        println!("\nError: {}", err);
        process::exit(1);
    });

    let mut person_vec = build_person_vec(&mut input_file);

    sort_person_vec(&mut person_vec);

    write_to_file(&mut person_vec, &mut output_file);
}```
\$\endgroup\$
  • \$\begingroup\$ Not worth an additional answer, but your comments on struct Person and impl Person are not doc-comments (use /// for those), also the one on impl Person actually seems to apply only to Person::new, so should be moved there. \$\endgroup\$ – Joe May 3 '20 at 7:13
  • \$\begingroup\$ Also, your block doc-comments aren't parsed correctly by VS Code/rust-analyzer. Not sure if it's a bug or if your formatting isn't valid. \$\endgroup\$ – Joe May 3 '20 at 7:15
6
\$\begingroup\$

From my limited Rust knowledge: (I'm a beginner too; let's learn together)

Compilation

I had to add use std::env to compile the code. Is that a copy-paste error?

Formatting

Your code deviates from the official Rust Style Guide in a few aspects:

  • Separate items and statements by either zero or one blank lines (i.e., one or two newlines). (Blank lines)

  • Indentation of function parameters / arguments:

    -    fn new(first_name: String, last_name: String,
    -            street: String, city: String, state: String,
    -            zip_code: String) -> Person {
    -
    +    fn new(
    +        first_name: String,
    +        last_name: String,
    +        street: String,
    +        city: String,
    +        state: String,
    +        zip_code: String,
    +    ) -> Person {
    
    -        let p = Person::new(String::from(data[0].trim()),
    -                            String::from(data[1].trim()),
    -                            String::from(data[2].trim()),
    -                            String::from(data[3].trim()),
    -                            String::from(data[4].trim()),
    -                            String::from(data[5].trim()));
    +        let p = Person::new(
    +            String::from(data[0].trim()),
    +            String::from(data[1].trim()),
    +            String::from(data[2].trim()),
    +            String::from(data[3].trim()),
    +            String::from(data[4].trim()),
    +            String::from(data[5].trim()),
    +        );
    
    -        let info = format!("{}, {}, {}, {}, {}, {}\n",
    -            p.first_name, p.last_name, p.street, p.city,
    -            p.state, p.zip_code);
    +        let info = format!(
    +            "{}, {}, {}, {}, {}, {}\n",
    +            p.first_name, p.last_name, p.street, p.city, p.state, p.zip_code
    +        );
    
  • Spacing before {:

    -            if (j_last < low_last) || (j_last == low_last && j_first < low_first){
    +            if (j_last < low_last) || (j_last == low_last && j_first < low_first) {
    

You can apply these formatting guidelines by running rustfmt. (I found these by using rustfmt --check, which prints a diff.)

Constructor

In my opinion, Person::new is unnecessary. This:

Person::new(a, b, c, d, e, f)

is not more readable than

Person {
    first_name: a,
    last_name: b,
    street: c,
    city: d,
    state: e,
    zip_code: f,
}

Result::expect

These match expressions:

// Get the input file
let input_file = match File::open(format!("{}{}", "src/", &args[1])) {
    Err(why) => panic!("\ncouldn't open file: {}", why),
    Ok(file) => file,
};

// Get the output file
let output_file = match File::create(format!("{}{}", "src/", &args[2])) {
    Err(why) => panic!("\ncouldn't create file: {}", why),
    Ok(file) => file,
};

can be simplified with Result::expect:

let input_file = File::open(format!("src/{}", &args[1])).expect("Couldn't open file");
let output_file = File::create(format!("src/{}", &args[2])).expect("Couldn't create file");

Similarly:

// Write to output file
match output_file.write_all(info.as_bytes()) {
    Err(why) => panic!("\ncouldn't write to file: {}", why),
    Ok(_) => (),
}

becomes

output_file
    .write_all(info.as_bytes())
    .expect("Couldn't write to file");

Note that expect uses fmt::Debug to print the error information. If you want to use fmt::Display (as your original code does), you can use unwrap_or_else instead of expect, per comment:

.unwrap_or_else(|err| panic!("Couldn't open file: {}", err))

Sorting

You can reinventing the wheel here:

fn sort_person_vec(person_vec: &mut Vec<Person>) {
    for i in 0..person_vec.len() {
        let mut lowest = i;

        for j in (i + 1)..person_vec.len() {
            // Temporary variables to hold first and last names
            let j_last = &person_vec[j].last_name.to_lowercase();
            let j_first = &person_vec[j].first_name.to_lowercase();
            let low_last = &person_vec[lowest].last_name.to_lowercase();
            let low_first = &person_vec[lowest].first_name.to_lowercase();

            // Swap by last name or first name if last names are equal
            if (j_last < low_last) || (j_last == low_last && j_first < low_first) {
                lowest = j;
            }
        }
        person_vec.swap(lowest, i);
    }
}

The lexical comparison feature of tuples can be used here:

fn sort_person_vec(person_vec: &mut Vec<Person>) {
    person_vec.sort_by_key(|person| {
        (
            person.last_name.to_lowercase(),
            person.first_name.to_lowercase(),
        )
    });
}

eprintln!

This:

println!("\nError: {}", err);

shouldn't be printed to stderr:

eprintln!("\nError: {}", err);

Error handling

Consider validating the data in build_person_vec.

\$\endgroup\$
  • 2
    \$\begingroup\$ It should be noted that .expect("Couldn't open file") will format the error using fmt::Debug, not fmt::Display like the original code. If you want to stick with fmt::Display, you could do .unwrap_or_else(|err| panic!("Couldn't open file: {}", err)) instead (still better than the match). \$\endgroup\$ – Joe May 3 '20 at 7:05
  • \$\begingroup\$ @Joe Thanks. I've edited the answer. \$\endgroup\$ – L. F. May 3 '20 at 7:23
  • \$\begingroup\$ You're welcome. The parenthetical "(as panic! does)" isn't quite correct, as panic! will use the format you specify (i.e., with {:?} instead of {} it would also use fmt::Display \$\endgroup\$ – Joe May 3 '20 at 7:29
  • 1
    \$\begingroup\$ @Joe Sorry, I've fixed that. \$\endgroup\$ – L. F. May 3 '20 at 10:19
  • \$\begingroup\$ Wow! Thank you so much for the detailed response. This is all incredibly helpful information, and your statements are crystal clear. I had no idea about rustfmt ! And yes, the use std::env; was a copy paste error- my bad! \$\endgroup\$ – HammerAPI May 3 '20 at 13:09
2
\$\begingroup\$

As always, I recommend using clippy for help. Running it, we get the following output:

warning: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
   --> src/main.rs:188:9
    |
188 | /         match output_file.write_all(info.as_bytes()) {
189 | |             Err(why) => panic!("\ncouldn't write to file: {}", why),
190 | |             Ok(_) => (),
191 | |         }
    | |_________^ help: try this: `if let Err(why) = output_file.write_all(info.as_bytes()) { panic!("\ncouldn't write to file: {}", why) }`

So that's the first change we can make.

/**
 * # CSV Sorter
 *
 * ## Author: Daniel Hammer
 *
 * ### Date: 2020/5/2
 *
 * ### Description:
 * This program reads in a CSV composed of information about people, such as
 * names and addresses. It then stores each entry into a struct, and those
 * structs into a vector. The vector is sorted by last name (or first, if
 * last names are identical) and the newly sorted data is written to an
 * output file.
 */

For module-level documentation, use //! instead of /**.

fn arg_parser(args: &[String]) -> Result<(File, File), &'static str> {

You really shouldn't return strings as your error type—they don't implement Error, so they don't play nicely with other stuff. Use an error handling crate instead, such as anyhow (or implement it yourself).

// Person constructor
impl Person {
    fn new(first_name: String, last_name: String,
           street: String, city: String, state: String,
           zip_code: String) -> Person {

        Person {
            first_name,
            last_name,
            street,
            city,
            state,
            zip_code,
        }
    }
}

// Person constructor is a useless comment. Remove it. And for that matter, there's no need for a constructor like this at all—and it's hard to remember the parameter order. Just fill in the Person struct manually.

    let input_file = match File::open(format!("{}{}", "src/", &args[1])) {
        Err(why) => panic!("\ncouldn't open file: {}", why),
        Ok(file) => file,
    };

    // Get the output file
    let output_file = match File::create(format!("{}{}", "src/", &args[2])) {
        Err(why) => panic!("\ncouldn't create file: {}", why),
        Ok(file) => file,
    };

Don't format paths like that, use Path::join instead.

let data: Vec<&str> = line.split(", ").collect();

You don't need to allocate a vector for that, just use Iterators directly (see my final code for my implementation).

fn sort_person_vec(person_vec: &mut [Person]) {

Just implement Ord on Person so you can just call person_vec.sort().

/**
 * Writes data to the output file
 *
 * # Description
 * Writes all Person structs to the output file, catching errors if the file
 * is not available to be written to.
 *
 * # Arguments
 * * `person_vec` - A vector containing Person structs.
 * * `output_file` - The file to write to.
 */
fn write_to_file(person_vec: &mut Vec<Person>, output_file: &mut File) {

    for p in person_vec {

        // Format the peron's information as a string
        let info = format!("{}, {}, {}, {}, {}, {}\n",
                           p.first_name, p.last_name, p.street, p.city,
                           p.state, p.zip_code);

        // Write to output file
        match output_file.write_all(info.as_bytes()) {
            Err(why) => panic!("\ncouldn't write to file: {}", why),
            Ok(_) => (),
        }
    }
}

Rust uses /// documentation comments (vs //! for modules), not /**. When you run cargo doc or publish your crate, this documentation won't get carried over. It's also not common to use a Description header in Rust, as that's implied. And there's no need to say what each argument is if that's all you're gonna say: it's obvious that the parameter person_vec of type &mut Vec<Person> is a Vec of Persons. Additionally, there's no need for person_vec to be mutable, or even a Vec at all. Instead, you should accept a &[Person]. There's also no reason for output_file to be a file—what if you want to send it over the network instead or compress it (e.g. gzip) before writing it? You should accept a &mut impl Write instead.

let info = format!("{}, {}, {}, {}, {}, {}\n",
                   p.first_name, p.last_name, p.street, p.city,
                   p.state, p.zip_code);

is probably written better with a fmt::Display implementation.

In general, you should also not create BufReaders/BufWriters within a function that reads or writes stuff. Leave the caller to do that.

Final code:

//! # CSV Sorter
//!
//! This program reads in a CSV composed of information about people, such as
//! names and addresses. It then stores each entry into a struct, and those
//! structs into a vector. The vector is sorted by last name (or first, if
//! last names are identical) and the newly sorted data is written to an
//! output file.

use std::cmp::Ordering;
use std::env;
use std::fs::File;
use std::io::{BufRead, BufReader, BufWriter, Write};
use std::path::PathBuf;
use std::process;

/// Person struct to hold relevant data
#[derive(Debug, PartialEq, Eq)]
struct Person {
    first_name: String,
    last_name: String,
    street: String,
    city: String,
    state: String,
    zip_code: String,
}

impl Ord for Person {
    fn cmp(&self, other: &Self) -> Ordering {
        (
            self.last_name.to_lowercase(),
            self.first_name.to_lowercase(),
        )
            .cmp(&(
                other.last_name.to_lowercase(),
                other.first_name.to_lowercase(),
            ))
    }
}

impl PartialOrd for Person {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}

///
/// Processes command-line arguments
///
/// # Description
/// This function processes the passed-in command line arguments and attempts
/// to open and create valid input/output files from the names given.
///
/// # Arguments
/// * `args` - A string array of command-line arguments.
///
/// # Returns
/// * A tuple of the input file and output file if they are found, else errors.
///
fn arg_parser(args: &[String]) -> Result<(File, File), &'static str> {
    // Exit if too many or too few arguments were passed
    if args.len() != 3 {
        return Err("Usage: 'cargo run [input file] [output file]");
    }

    // Get the input file
    let input_file = File::open(PathBuf::from("src").join(&args[1])).expect("Couldn't open file");
    let output_file =
        File::create(PathBuf::from("src").join(&args[2])).expect("Couldn't create file");

    // Return both files as a tuple
    Ok((input_file, output_file))
}

///
/// Builds a list of Person structs
///
/// # Description
/// This function reads the input file line by line and creates a Person
/// struct based on the line's contents. It then adds that struct to a vector
/// and repeats for every line in the file. The final vector contains every
/// Person struct read in from the file.
///
/// # Returns
/// * A vector of type Person containing all Person structs from the file.
fn build_person_vec(reader: &mut impl BufRead) -> Vec<Person> {
    let mut person_vec: Vec<Person> = Vec::new();

    for line in reader.lines() {
        let line = line.unwrap();

        let mut data = line.split(',').map(|s| s.trim());

        let p = Person {
            first_name: String::from(data.next().unwrap()),
            last_name: String::from(data.next().unwrap()),
            street: String::from(data.next().unwrap()),
            city: String::from(data.next().unwrap()),
            state: String::from(data.next().unwrap()),
            zip_code: String::from(data.next().unwrap()),
        };
        person_vec.push(p);
    }
    person_vec
}

///
/// Writes data to the output file
///
/// Writes all Person structs to the output file, catching errors if the file
/// is not available to be written to.
fn write_to_file(person_vec: &[Person], output_file: &mut impl Write) {
    for p in person_vec {
        let info = format!(
            "{}, {}, {}, {}, {}, {}\n",
            p.first_name, p.last_name, p.street, p.city, p.state, p.zip_code
        );

        output_file
            .write_all(info.as_bytes())
            .expect("Couldn't write to file");
    }
}

fn main() {
    let args: Vec<String> = env::args().collect();

    // Get the input and output files
    let (input_file, output_file) = arg_parser(&args).unwrap_or_else(|err| {
        eprintln!("\nError: {}", err);
        process::exit(1);
    });

    let mut person_vec = build_person_vec(&mut BufReader::new(&input_file));

    person_vec.sort();

    write_to_file(&person_vec, &mut BufWriter::new(output_file));
}
```
\$\endgroup\$

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.