Review of "hr manager" :) (chapter 8.3, 3rd exercise)

Here I am with another terribly boring exercise, any thoughts on this?
Although I am moderately satisfied with the results, anything that at this stage can be done better?
By that I mean, using the concepts illustrated in the book up until this chapter.
EDIT: I have already renamed the unused variable as suggested.

use std::collections::hash_map::Entry;
use std::{collections::HashMap, io};

fn add(personnel: &mut HashMap<String, Vec<String>>, dept: &str, person: &str) {
    let dept_employees = personnel.entry(dept.to_string());

    match dept_employees {
        Entry::Vacant(entry) => {
            entry.insert(vec![person.to_string()]);
        }
        Entry::Occupied(mut entry) => {
            let people = entry.get_mut();
            if !people.contains(&person.to_string()) {
                people.push(person.to_string());
            }
        }
    }
}

fn list(personnel: &HashMap<String, Vec<String>>, dept: Option<&str>) {
    match dept {
        Some(dept) => {
            if let Some(dept_employees) = personnel.get(dept) {
                for employee in dept_employees {
                    println!("- {}", employee);
                }
            } else {
                println!("No such department.");
            }
        }
        None => {
            for (dept, people) in personnel.iter() {
                println!("Department: {}", dept);
                for person in people {
                    println!("  - {}", person);
                }
            }
        }
    }
}

fn handle_command(input: &str, personnel: &mut HashMap<String, Vec<String>>) {
    let mut parts = input.split_whitespace();

    // Step 1: get the command
    let command = match parts.next() {
        Some(cmd) => cmd,
        None => {
            println!("No command given.");
            return;
        }
    };

    // Step 2: match command and pull arguments accordingly
    match command {
        "add" => {
            let person = match parts.next() {
                Some(name) => name,
                None => {
                    println!("Missing name for 'add'.");
                    return;
                }
            };

            let to_keyword = parts.next();

            let dept = match parts.next() {
                Some(dept) => dept,
                None => {
                    println!("Missing department for 'add'.");
                    return;
                }
            };

            println!("Adding {} to department {}", person, dept);

            add(personnel, dept, person);
        }

        "list" => {
            let dept = parts.next();

            match dept {
                Some(dept) => println!("Listing personnel in department: {}", dept),
                None => println!("Listing all departments"),
            }

            list(personnel, dept);
        }

        _ => println!("Unknown command: {}", command),
    }
}

fn main() {
    let mut personnel: HashMap<String, Vec<String>> = HashMap::new();
    println!("Usage: ");
    println!("adding: add Name to Dept");
    println!("listing: list Dept, list for all Dept");
    println!("quit: empty line");
    loop {
        let mut input = String::new();
        io::stdin()
            .read_line(&mut input)
            .expect("Failed to read line");

        let input = input.trim();
        if input.is_empty() {
            break;
        };

        handle_command(input, &mut personnel);
    }
}

(Playground)

Output:

Usage: 
adding: add Name to Dept
listing: list Dept, list for all Dept
quit: empty line
Missing department for 'add'.
Missing department for 'add'.
Adding sara to department sales
Adding john to department devel
Adding peter to department devel
Listing personnel in department: sale
No such department.
Listing personnel in department: sales
- sara
Listing personnel in department: devel
- john
- peter
Listing all departments
Department: devel
  - john
  - peter
Department: sales
  - sara

Errors:

   Compiling playground v0.0.1 (/playground)
warning: unused variable: `to_keyword`
  --> src/main.rs:65:17
   |
65 |             let to_keyword = parts.next();
   |                 ^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_to_keyword`
   |
   = note: `#[warn(unused_variables)]` on by default

warning: `playground` (bin "playground") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.74s
     Running `target/debug/playground`

1 Like

One short bit:

The exercise said to list alphabetically. A BTreeMap would be a good fit here, but I guess that counts as "not introduced yet", so... maintain a sorted Vec?

fn add(personnel: &mut HashMap<String, Vec<String>>, dept: &str, person: &str) {
    let entry = personnel.entry(dept.to_string()).or_default();
    if let Err(idx) = entry.binary_search_by(|emp| (**emp).cmp(person)) {
        entry.insert(idx, person.to_owned());
    }
}

Your parsing doesn't detect garbage after the command. If you wanted to spend more time on it, you could also support multi-word names by using "to" as the required delimiter. (OTOH manual parsing is always a tedious affair, so I would understand if you didn't care to.)

A general comment: IMO things would be cleaner with some nominal structs, like say a Personnel with a HashMap<String, Vec<String>> field to start.

Micro-optimization to reuse the same String allocation:

+    let mut input = String::new();
    loop {
-        let mut input = String::new();
+        input.clear();
2 Likes

In add, you take person: &str, but you always end up doing name.to_string(). Instead, you could do person: String to make the cost up-front and to allow users to reuse existing allocations.

Alternatively, if you're okay with a somewhat more advanced solution, you could use person: impl ToString and just call person.to_string() once (which you should do anyways). That still allows your function to directly take things like string literals (as in your example), but without wasting an allocation when a user passes a String. However, it does slightly hide the allocation cost from the user, which you might find undesirable.

1 Like

These are pretty basic modifications since I am also learning, what I did was too change the use on top, use a struct that wraps the hash map, use methods.

structs were already explained so that should be fine.

I also formatted the initial usage string a bit so that it's more user-friendly :slight_smile:

I added type aliases just because I think it was very simple and it can be learned just from there, and also added the debug to the struct, so when can use the initial const to have minor debugging information.

It's also likely I may have introduced some bugs.

use std::collections::{HashMap, hash_map::Entry};
use std::io;

// simple aliases even though it's not introduced yet
type Department = String;
type Personnel = Vec<String>;

const DEBUG_MODE:bool=true;
// extra level to give context
// and add methods
#[derive(Debug)]
struct Institution {
    department_personnel: HashMap<Department, Personnel>,
}

// implement the methods
impl Institution {
    // add employee to department
    fn add_employee(&mut self, dept: &str, employee: &str) {
        let dept_employees = self.department_personnel.entry(dept.to_string());
        let personString = String::from(employee);
        
        match dept_employees {
            Entry::Vacant(entry) => {
                entry.insert(vec![personString]);
            }
            Entry::Occupied(mut entry) => {
                let people = entry.get_mut(); // mutable vec
                if !people.contains(&personString) {
                    people.push(personString);
                }
            }
        }
        if DEBUG_MODE {
            println!("{self:?}");
        }
    }
    // List employees by the department, only specified dept.
    fn list_employees(&self, dept: Option<&str>) {
        let personnel = &self.department_personnel;
        match dept {
            Some(dept) => {
                if let Some(dept_employees) = personnel.get(dept) {
                    for employee in dept_employees {
                        println!("- {}", employee);
                    }
                } else {
                    println!("No such department.");
                }
            }
            None => {
                for (dept, people) in personnel.iter() {
                    println!("Department: {}", dept);
                    for person in people {
                        println!("  - {}", person);
                    }
                }
            }
        }
    }

    fn handle_command(&mut self, input: &str) {
        let mut parts = input.split_whitespace();

        // Step 1: get the command
        let command = match parts.next() {
            Some(cmd) => cmd,
            None => {
                println!("No command given.");
                return;
            }
        };

        // Step 2: match command and pull arguments accordingly
        match command {
            "add" => {
                let person = match parts.next() {
                    Some(name) => name,
                    None => {
                        println!("Missing name for 'add'.");
                        return;
                    }
                };

                let _to_keyword = parts.next();

                let dept = match parts.next() {
                    Some(dept) => dept,
                    None => {
                        println!("Missing department for 'add'.");
                        return;
                    }
                };

                println!("Adding {} to department {}", person, dept);

                self.add_employee(dept, person);
            }

            "list" => {
                let dept = parts.next();

                match dept {
                    Some(dept) => println!("Listing personnel in department: {}", dept),
                    None => println!("Listing all departments"),
                }

                self.list_employees(dept);
            }

            _ => println!("Unknown command: {}", command),
        }
    }
}

fn main() {
    let mut institution = Institution {
        department_personnel: HashMap::new(),
    };
    println!("Usage\n");
    println!("adding\n\t*`add <Name> to <Dept>`.\n\t* Example: `add Jerome to Milky_Way`");
    println!("listing\n\t* `list <Dept>` or `list` for all Dept.\n\t* Example: `list Milky_Way`");
    println!("\nto quit, press Return/Enter key.");
    loop {
        let mut input = String::new();
        io::stdin()
            .read_line(&mut input)
            .expect("Failed to read line");

        let input = input.trim();
        if input.is_empty() {
            break;
        };

        institution.handle_command(input);
    }
}
1 Like

ToString::to_string always has to allocate, because it takes &self.

ToString also uses the fmt::Display machinery to create a String, (see this blanket impl) which is usually inefficient for existing string types.[1]

The trait bound that is useful here is Into<String>, which doesn't allocate for String and allows passing &str.[2]


  1. In practice, std uses specialization internally to avoid this for String and &str, but this is just an optimization and isn't guaranteed. ↩︎

  2. and also some other types, including &String, &mut str, Box<str>, and Cow<'a, str>. All these impls only allocate if necessary. ↩︎

3 Likes

You can simplify the parsing process significantly by using pattern matching on your command. Although this requires a collect into a Vec, readability is arguably more important than efficiency.

Here's an example:

use std::{
    collections::HashMap,
    io::{self, Write},
};

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

    loop {
        print!("> Enter a command: ");
        io::stdout().flush().unwrap();

        let mut line = String::new();
        io::stdin().read_line(&mut line).unwrap();

        let tokens: Vec<_> = line.split_whitespace().collect();

        match tokens.as_slice() {
            ["Add", worker, "to", department] => departments
                .entry(department.to_string())
                .or_default()
                .push(worker.to_string()),
            ["Add", ..] => println!("Error, usage: Add <worker> to <department>"),
            ["List", department] => {
                if let Some(workers) = departments.get_mut(*department) {
                    workers.sort();
                    for name in workers {
                        println!("{name}");
                    }
                }
            }
            ["List", ..] => println!("Error, usage: List <department>"),
            ["Quit"] => break,
            _ => println!("Error: Unknown command, use Add, List or Quit"),
        }
    }
}
4 Likes

My bad, thank you for the correction!

Thanks to all who responded, here are my remarks:

@quinedot

  • I admit I was so focused on what I perceived to be the most difficult/bothersome part (the parsing) that I completely missed the sorting requirement, though it is unclear if it refers to departments, employees or both.
  • The book never mentions BTreeMap at all (it cannot possibly mention everything) so your suggestion is valid, I'll try incorporating it.
  • I freely admit to not caring about the parsing being rudimentary, for precisely the reasons you mentioned.
  • The closure is definitely new to me, so I'll work around it. (still have to really understand it, I mean)

@Kyllingene and @cod10129

  • this part flew a bit over my head, not really sure if I can fully appreciate the topic right now.

@anon81327159

  • I'll adopt the named struct suggestion and skip the "impl", this has not been covered yet.
  • Thanks for the debug tip, I had seen it around, must remember to use it more often.

@danfabo

  • nice, the parsing does look cleaner this way!
  • there is no way to list all departments, but change is minimal

Revised version incorporates the suggestions which best fit and, hopefully, mostly respects my self-imposed limitation of using only already explained constructs.