5
\$\begingroup\$

https://doc.rust-lang.org/book/ch08-03-hash-maps.html

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.

This language, its book and the community are just straight up awesome. I had a quick read of other reviews just before posting mine, and I decided it'd be a good idea to run cargo fmt and cargo clippy before posting. +1 for tooling!

There's two drivers for me posting this- firstly, is the data structure I'm using ok?

HashMap<String, Vec<String>>

It feels a little... strange. Because a HashMap is re-sizeable, Strings are re-sizeable and Vecs are re-sizeable. Is this safe and/or efficient? Am I overthinking?

Secondly, I've attempted to handle as many error cases as I can but I wonder whether there's any that I've missed? Can't wait to get to the automated testing chapter, where I can start to maybe tackle those problems. Anyway, without further ado:

// valid commands:
// ADD name TO department, DEL name FROM department
// LIST department, LIST, QUIT
use std::{
    collections::HashMap,
    {io, io::Write},
};

#[derive(Debug)]
struct Command {
    c_type: CommandType,
    args: Option<Vec<String>>,
}

#[derive(Debug)]
enum CommandType {
    Add,
    List,
    Del,
    Quit,
}

impl CommandType {
    fn new(val: String) -> Option<CommandType> {
        match val.as_str() {
            "ADD" => Some(CommandType::Add),
            "DEL" => Some(CommandType::Del),
            "LIST" => Some(CommandType::List),
            "QUIT" => Some(CommandType::Quit),
            _ => None,
        }
    }
}

fn main() {
    let mut employer_table: HashMap<String, Vec<String>> = HashMap::new();

    employer_table.insert(
        String::from("ENGINEERING"),
        vec![
            String::from("Lewis"),
            String::from("Ellen"),
            String::from("Alan"),
        ],
    );

    employer_table.insert(
        String::from("SALES"),
        vec![
            String::from("Jane"),
            String::from("Robert"),
            String::from("Penny"),
        ],
    );

    loop {
        let cmd: String = match user_prompt() {
            Ok(val) => val,
            _ => {
                println!("Bad user input. Please try again...");
                continue;
            }
        };

        let cmd: Command = match command_parser(cmd) {
            Some(val) => val,
            None => {
                println!("Invalid command... Please type ADD, DEL, LIST or QUIT.");
                continue;
            }
        };

        // QUIT is the only command that returns false
        if !command_executor(cmd, &mut employer_table) {
            break;
        }
    }
}

fn user_prompt() -> io::Result<String> {
    let mut buf = String::new();
    let stdin = io::stdin();

    // This is a little prompt to indicate that a terminal row is typable...
    // Copied this guide on SO to get this prompt working:
    // https://stackoverflow.com/a/41387232
    print!("> ");
    io::stdout().flush().unwrap();
    stdin.read_line(&mut buf)?;

    Ok(String::from(buf.trim())) // Remove trailing CRLF, convert str to String
}

// valid commands:
// ADD name TO department, DEL name FROM department
// LIST department, LIST, QUIT
fn command_parser(c: String) -> Option<Command> {
    let args: Vec<&str> = c.split(' ').collect(); // Convert string to array of words
    let arg0 = args[0].to_uppercase();

    match CommandType::new(arg0) {
        Some(c_type) => {
            let mut args = slice_vec_to_string_vec(args);

            if !args.is_empty()  {
                let len = args.len() - 1;
                args[len] = args[len].to_uppercase();
                Some(Command {c_type, args: Some(args) })
            } else {
                Some(Command { c_type, args: None })
            }
        }
        None => None,
    }
}

fn slice_vec_to_string_vec(s: Vec<&str>) -> Vec<String> {
    let mut tmp: Vec<String> = Vec::new();

    for i in s[1..].iter() {
        tmp.push(String::from(*i));
    }

    tmp
}

fn command_executor(cmd: Command, table: &mut HashMap<String, Vec<String>>) -> bool {
    match cmd.c_type {
        CommandType::Add => {
            match cmd.args {
                Some(arg) => {
                    add_employee(table, String::from(&arg[0][..]), String::from(&arg[2][..]))
                }
                None => println!("Invalid arguments..."),
            }
            true
        }
        CommandType::List => {
            match cmd.args {
                Some(arg) => list_all_employees(table, Some(String::from(&arg[0]))),
                None => list_all_employees(table, None),
            }
            true
        }
        CommandType::Del => {
            match cmd.args {
                Some(arg) => {
                    del_employee(table, String::from(&arg[0][..]), String::from(&arg[2][..]))
                }
                None => println!("Invalid arguments..."),
            }
            true
        }
        CommandType::Quit => {
            println!("Thank you for using this shitty system. Goodbye!");
            false
        }
    }
}

fn list_all_employees(table: &HashMap<String, Vec<String>>, opts: Option<String>) {
    match opts {
        Some(opt) => match table.get(&opt) {
            Some(values) => println!("\n{}: {:#?}", opt, values),
            None => println!("{} is an invalid department.", opt),
        },
        None => println!("{:#?}", table),
    }
}

fn add_employee(table: &mut HashMap<String, Vec<String>>, emp: String, dep: String) {
    match table.get_mut(&dep) {
        Some(ptr) => ptr.push(emp),
        None => println!(
            "Failed to get a pointer to the department: {} using employee: {}",
            dep, emp
        ),
    };
}

fn del_employee(table: &mut HashMap<String, Vec<String>>, emp: String, dep: String) {
    match table.get_mut(&dep) {
        Some(d_ptr) => match d_ptr.iter().position(|e| *e == emp) {
            Some(e_ptr) => {
                d_ptr.remove(e_ptr); // Has to have semi colon so that the arm doesn't return anything
            }
            None => println!("Unable to remove that element..."),
        },
        None => println!(
            "Failed to get a pointer to department: {} using employee: {}",
            dep, emp
        ),
    };
}
```
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Your questions

  1. HashMap<String, Vec<String>> is fine. Since this object exists in main at the highest level, it must store owned values instead of references to its content, so String and Vec must be used instead of &str.

  2. For a not yet checked error case see below, under "Missing indexing check".

Remove slice_vec_to_string_vec

First of all, its name is misleading as it silently discards the first element of the input vec. It can be replaced in command_parser by changing this line:

let args: Vec<&str> = c.split(' ').collect(); 

to:

let mut args: Vec<String> = c.split(' ').map(String::from).collect(); 

This directly converts all &str parts to Strings efficiently. Removing the first element afterwards is simple:

args.remove(0);

slice_vec_to_string_vec can now be removed.

Store an empty vec instead of None

Command.args is an Option<Vec<String>> right now, but it could just be a Vec<String> which is empty to represent None. The match cmd.args { usages will then become if !cmd.args.is_empty() {.

Prefer taking &str over String as argument type

A String will be automatically coerced into a &str cheaply, while creating a String from a &str is comparatively expensive and must be done manually. list_all_employees, add_employee, del_employee can all be modified to accept a &str, such that calling them changes from:

del_employee(table, String::from(&cmd.args[0][..]), String::from(&cmd.args[2][..]));

to:

del_employee(table, &cmd.args[0], &cmd.args[2]);

Print errors with eprintln! instead of println!

println! only prints the message to the standard output stream, while eprintln! prints the message to the standard error stream.

Use longer variable names

Spell emp and dep out as employee and department. Also, you are not using pointers, as d_ptr, e_ptr, ptr and your error messages suggest, but rather references. Actual pointers in Rust (*const Type and *mut Type) are rarely used since they may point to invalid data and dereferencing them is unsafe.

let-else in main loop

The following destructuring assignment:

let cmd: String = match user_prompt() {
    Ok(val) => val,
    _ => {
        eprintln!("Bad user input. Please try again...");
        continue;
    }
};

can be expressed more concisely using let-else syntax:

let Ok(cmd) = user_prompt() else {
    eprintln!("Bad user input. Please try again...");
    continue;
};

The else branch must always diverge (meaning return, break, continue, panicking or ending the program otherwise) for this syntax to be usable.

Final code

// valid commands:
// ADD name TO department, DEL name FROM department
// LIST department, LIST, QUIT
use std::{collections::HashMap, io, io::Write};

#[derive(Debug)]
struct Command {
    c_type: CommandType,
    args: Vec<String>,
}

#[derive(Debug)]
enum CommandType {
    Add,
    List,
    Del,
    Quit,
}

impl CommandType {
    fn new(val: &str) -> Option<CommandType> {
        match val {
            "ADD" => Some(CommandType::Add),
            "DEL" => Some(CommandType::Del),
            "LIST" => Some(CommandType::List),
            "QUIT" => Some(CommandType::Quit),
            _ => None,
        }
    }
}

fn main() {
    let mut employer_table: HashMap<String, Vec<String>> = HashMap::new();

    employer_table.insert(
        String::from("ENGINEERING"),
        vec![
            String::from("Lewis"),
            String::from("Ellen"),
            String::from("Alan"),
        ],
    );

    employer_table.insert(
        String::from("SALES"),
        vec![
            String::from("Jane"),
            String::from("Robert"),
            String::from("Penny"),
        ],
    );

    loop {
        let Ok(input) = user_prompt() else {
            eprintln!("Bad user input. Please try again...");
            continue;
        };

        let Some(cmd) = command_parser(input) else {
            eprintln!("Invalid command... Please type ADD, DEL, LIST or QUIT.");
            continue;
        };

        // QUIT is the only command that returns false
        if !command_executor(cmd, &mut employer_table) {
            break;
        }
    }
}

fn user_prompt() -> io::Result<String> {
    let mut buf = String::new();
    let stdin = io::stdin();

    // This is a little prompt to indicate that a terminal row is typable...
    // Copied this guide on SO to get this prompt working:
    // https://stackoverflow.com/a/41387232
    print!("> ");
    io::stdout().flush().unwrap();
    stdin.read_line(&mut buf)?;

    Ok(String::from(buf.trim())) // Remove trailing CRLF, convert str to String
}

// valid commands:
// ADD name TO department, DEL name FROM department
// LIST department, LIST, QUIT
fn command_parser(c: String) -> Option<Command> {
    let mut args: Vec<String> = c.split(' ').map(String::from).collect(); // Convert string to array of words
    let arg0 = args[0].to_uppercase();

    let c_type = CommandType::new(&arg0)?;
    args.remove(0);

    if let Some(last) = args.last_mut() {
        *last = last.to_uppercase();
    }
    Some(Command { c_type, args })
}

fn command_executor(cmd: Command, table: &mut HashMap<String, Vec<String>>) -> bool {
    match cmd.c_type {
        CommandType::Add => {
            if !cmd.args.is_empty() {
                add_employee(table, &cmd.args[0], &cmd.args[2])
            } else {
                eprintln!("Invalid arguments...")
            }
            true
        }
        CommandType::List => {
            if !cmd.args.is_empty() {
                list_all_employees(table, Some(&cmd.args[0]))
            } else {
                list_all_employees(table, None)
            }
            true
        }
        CommandType::Del => {
            if !cmd.args.is_empty() {
                del_employee(table, &cmd.args[0], &cmd.args[2])
            } else {
                eprintln!("Invalid arguments...")
            }
            true
        }
        CommandType::Quit => {
            println!("Thank you for using this shitty system. Goodbye!");
            false
        }
    }
}

fn list_all_employees(table: &HashMap<String, Vec<String>>, opts: Option<&str>) {
    match opts {
        Some(opt) => match table.get(opt) {
            Some(values) => println!("\n{}: {:#?}", opt, values),
            None => eprintln!("{} is an invalid department.", opt),
        },
        None => println!("{:#?}", table),
    }
}

fn add_employee(table: &mut HashMap<String, Vec<String>>, employee: &str, department: &str) {
    match table.get_mut(department) {
        Some(department_ref) => department_ref.push(employee.to_string()),
        None => eprintln!(
            "Failed to get a reference to the department: {} using employee: {}",
            department, employee
        ),
    };
}

fn del_employee(table: &mut HashMap<String, Vec<String>>, employee: &str, department: &str) {
    match table.get_mut(department) {
        Some(department_ref) => match department_ref.iter().position(|e| *e == employee) {
            Some(employee_index) => {
                department_ref.remove(employee_index); // Has to have semicolon so that the arm doesn't return anything
            }
            None => eprintln!("Unable to remove that element..."),
        },
        None => eprintln!(
            "Failed to get a reference to department: {} using employee: {}",
            department, employee
        ),
    };
}

Further possible improvements

Missing indexing check

Entering Add Sally or Del Robert should give an explanatory error message about the missing department, but instead panics with index out of bounds: the len is 1 but the index is 2.

to is ignored in Add Sally to Sales

A word between employee and department must exist, but it can be anything. Consider checking that it is actually to.

\$\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.