Code Review for Rust Book 8.3 Company Manager

[CWSODA]

I tried completing the practice for 8.3. The code works but there are parts that I find ugly. I had to check Option<> for adding names which introduced unpleasant nested loops. I also had to clone all the department keys so I can use &mut on the hashmap to sort them. Please share any improvements I can make.

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

fn main() {
    println!(">>> Welcome to Company Manager!!!");
    println!(">>> Enter commands to start. Enter \"help\" for list of commands.");

    // department and list of names
    let mut department_data: HashMap<String, Vec<String>> = HashMap::new();
    let mut is_running = true;

    loop{
        let mut input = String::new();
        if let Err(_) = std::io::stdin().read_line(&mut input){
            println!(">>> Please try again!!");
            continue;
        }

        let mut word_iter = input.split_whitespace();
        if let Some(command_word) = word_iter.next(){
            match command_word {
                "add" => on_add(word_iter, &mut department_data),
                "help" => on_help(word_iter),
                "show" => on_show(word_iter, &mut department_data),
                "quit" => on_quit(word_iter, &mut is_running),
                _ => println!(">>> Invalid Command!")
            }
        }

        if !is_running {
            break;
        }
    }
}

fn on_quit(mut word_iter: SplitWhitespace<'_>, is_running: &mut bool){
    if let Some(_) = word_iter.next() {
        // not help command
        println!(">>> Invalid Command!");
        return;
    }

    *is_running = false;
}

fn on_help(mut word_iter: SplitWhitespace<'_>){
    if let Some(_) = word_iter.next() {
        // not help command
        println!(">>> Invalid Command!");
        return;
    }
    println!(">>> Use \"add <name> to <department>\" command to add an employee to a department");
    println!(">>> Use \"show <department>\" command to show all employees in a department");
    println!(">>> Use \"show all\" command to show all departments and employees");
    println!(">>> Use \"quit\" command to quit the program");
}

fn on_add(mut word_iter: SplitWhitespace<'_>, hash: &mut HashMap<String, Vec<String>>){
    if let Some(name) = word_iter.next(){
        if let Some(require_to) = word_iter.next(){
            if require_to == "to"{
                if let Some(department) = word_iter.next(){
                    (*hash).entry(department.to_string()).or_insert(Vec::new()).push(name.to_string());
                    println!(">>>>> Added {name} to department {department}!");
                    return;
                }
            }
        }
    }

    println!(">>> Use \"add <name> to <department>\" command to add a person to a department");
}

fn on_show(mut word_iter: SplitWhitespace<'_>, hash: &mut HashMap<String, Vec<String>>){
    if let Some(word) = word_iter.next(){
        if let None = word_iter.next(){
            match word{
                "all" => {
                    let mut dep_vec = Vec::new();
                    for departments in hash.keys(){
                        dep_vec.push(departments.clone());
                    }
                    dep_vec.sort();

                    for dep in dep_vec {
                        show_department(&dep, hash);
                    }

                    return;
                },

                _ => {
                    show_department(word, hash);
                    return;
                }
            }
        }
    }

    println!(">>> Use \"show all\" command to show all departments and employees");
    println!(">>> Use \"show <department>\" command to show all employees in a department");
}

fn show_department(department: &str, hash: &mut HashMap<String, Vec<String>>){
    if let Some(vec) = hash.get_mut(department){
        vec.sort();
        print!(">>>>> Employees in {}: ", department);

        for name in vec {
            print!(" {name} ");
        }
        print!("\n");
        std::io::stdout().flush().expect("unknown error in text output");
        return;
    }
    println!(">>> Invalid Department!");
}

[jonnyso]

Well, when you have these nested option instances it might be a good idea to take a look at what the standard library offers you, like map, and_then, then_some, like in the example bellow, which isn't necessaraly better and might be less obvious if you're not familiar with the library:

#![allow(unused)]

use std::str::SplitWhitespace;
use std::collections::HashMap;

fn on_add(mut word_iter: SplitWhitespace<'_>, hash: &mut HashMap<String, Vec<String>>) {
    let params = word_iter.next().and_then(|name| {
        word_iter
            .next()
            .is_some_and(|require_to| require_to == "to")
            .then(|| word_iter.next())
            .and_then(|opt| opt.map(|department| (name, department)))
    });

    match params {
        Some((name, department)) => {
            hash.entry(department.to_string())
                .or_insert(vec![])
                .push(name.to_string());

            println!(">>>>> Added {name} to department {department}!");
        }
        None => println!(
            ">>> Use \"add <name> to <department>\" command to add a person to a department"
        ),
    }
}

Although I think you have a deeper problem with how you're parsing this params, since you expect then in a given order, you could collect the iterator into a vec, and check if the length and the values of each position match.

Most importantly, this is clearly a case for using Results, this function would be better off returning a Result that you can handle in your main loop.

[quinedot]

I know the chapter is about HashMap, but for keeping things sorted, there's BTreeMap (and BTreeSet instead of Vec). Without changing types, you can remove the need to clone all the keys by keeping things sorted upon insert.

        let vec = hash.entry(department.into()).or_default();
        let idx = vec.partition_point(|s| &**s < name);
        vec.insert(idx, name.into());
        println!(">>>>> Added {name} to department {department}!");

You can use patterns like so to get rid of a lot of nesting.

    let word = word_iter.next();
    let junk = word_iter.next();

    let (Some(word), None) = (word, junk) else {
        println!(">>> Use \"show all\" command to show all departments and employees");
        println!(">>> Use \"show <department>\" command to show all employees in a department");
        return
    };

Other miscellaneous notes:

• Standard out is line buffered when going to a terminal (like an interactive program is expected to do), so you don't have to flush if you printed a newline
• It's reasonable to just panic if you get an error from reading standard in; read_line ignores interrupts and retrying probably won't work. (And println! panics when standard out errors.)
• The only time you break the loop is when on_quit is called, sometimes, so I'd just have that function return a bool indicating whether or not to really quit.

Here's what I ended up with after a first pass. (I did practically no testing.)

---

My next suggestion would be to start parsing things more formally, just to separate that tedium out from the rest of your logic. It's boilerplatey and doesn't look much better on its own, but makes everything else nicer.

// Something like this (that you can put in its own module)...
pub(crate) enum Command<'a> {
    Add(&'a str, &'a str),
    Help,
    Show(Option<&'a str>),
    Quit,
    NoOp,
}

impl<'a> Command<'a> {
    pub fn parse(input: &'a str) -> Result<Self, &'static str> {
        // ...
    }
}

        match Command::parse(&input) {
            Err(e) => println!("{e}"),
            Ok(Command::NoOp) => {}
            Ok(Command::Quit) => break,
            Ok(Command::Help) => on_help(),
            Ok(Command::Show(department)) => {
                on_show(department, &department_data)
            }
            Ok(Command::Add(name, department)) => {
                on_add(name, department, &mut department_data)
            }
        }

fn on_add(name: &str, department: &str, hash: &mut HashMap<String, Vec<String>>){
    let vec = hash.entry(department.into()).or_default();
    let idx = vec.partition_point(|s| &**s < name);
    vec.insert(idx, name.into());
    println!(">>>>> Added {name} to department {department}!");
}

[jendrikw]

Use clippy