2
\$\begingroup\$

I'm in the process of reading The Rust Programming Language, and decided to do the 3rd task at the end of the 8th chapter. The description is as follows:

Using a hash map and vectors, create a text interface to allow a user to add employee names to a department in a company. For example, “Add Sally to Engineering” or “Add Amir to Sales.” Then let the user retrieve a list of all people in a department or all people in the company by department, sorted alphabetically.

Please tell me if my code is any good.

use std::collections::HashMap;
use std::io;
use std::io::Write;

use regex::Regex;


fn main() {
    let add_regex = Regex::new(r"^(?i)Add\s(\w(?:\w|\s)*\w?)\sto\s(\w+)$").unwrap();
    let list_regex = Regex::new(r"^(?i)List\s(\w+)$").unwrap();
    let all_regex = Regex::new(r"^(?i)Show all$").unwrap();
    let mut departments: HashMap<String, Vec<String>> = HashMap::new();

    loop {
        let mut input = String::new();

        print!("Enter command: ");
        io::stdout().flush();
        io::stdin().read_line(&mut input);

        input = String::from(input.trim());

        let add_match = add_regex.captures(&input);
        let list_match = list_regex.captures(&input);
        let all_match = all_regex.captures(&input);

        if add_match.is_some() {
            let unwrapped_match = add_match.unwrap();
            let employee = &unwrapped_match[1];
            let dpt = &unwrapped_match[2];

            match departments.get_mut(dpt) {
                Some(v) => {
                    v.push(String::from(employee));
                    v.sort();
                },
                None => {
                    let emp_list = vec![String::from(employee)];
                    departments.insert(String::from(dpt), emp_list);
                }
            }
        } else if list_match.is_some() {
            let unwrapped_match = list_match.unwrap();
            let employees = departments.get(&unwrapped_match[1]);

            if !employees.is_some() {
                println!("No such department");
                continue;
            }

            println!("Department employees: {}", employees.unwrap().join(", "));
            io::stdout().flush();
        } else if all_match.is_some() {
            for (dpt, employees) in &departments {
                println!("{}: {}", dpt, employees.join(", "));
            }
        } else {
            println!("Please enter a valid command");
        }
    }
}

Sample output:

Enter command: add f to ff
Enter command: add b to ff
Enter command: list ff
Department employees: b, f
Enter command: add Darth Vader to ff
Enter command: list ff
Department employees: Darth Vader, b, f
Enter command: add d to zz
Enter command: add 1 to zz
Enter command: show all
ff: Darth Vader, b, f
zz: 1, d
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

At a glance: that function is way too long and deeply nested. Three levels of loop/branch nesting is, in my experience, the most that any function should have; and the more nested it is the shorter it should be.

Consider extracting branch and loop bodies to separate named functions.


Your code structure also seems to be “determine what command to execute” (add / list / etc), then “execute said command”. I’d make this explicit, have the regexp parsing of an input line in a separate function that returns the command type and parameters. Dispatching this would be a natural use for match over your own Command type. (That said, in a realistic CLI you might need the commands available to be extensible so don’t get too married to this pattern. I’m guessing you’ll get to dynamic polymorphism later in the book.)

\$\endgroup\$

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.