Rust book: Employees and departments exercise

Question

I am learning Rust from the official book, and I have solved the following recommended challenge from the end of chapter 8:

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.

Are there any bad practices in my code? The code works, and this is the closest form to idiomatic Rust that I could come up with.

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

enum Command {
    Add { name: String, department: String },
    List { department: String },
    All,
    Quit,
}

impl Command {
    fn from_string(s: &String) -> Option<Self> {
        let el: Vec<&str> = s.trim().split_whitespace().collect();
        match el.as_slice() {
            ["Add", name, "to", department] => Some(Self::Add { name: name.to_string(), department: department.to_string() }),
            ["List", department] => Some(Self::List { department: department.to_string() }),
            ["All"] => Some(Self::All),
            ["Quit"] => Some(Self::Quit),
            _ => None,
        }
    }

    fn process(&self, deps: &mut &mut HashMap<String, Vec<String>>) -> bool {
        fn print_department(deps: &mut &mut HashMap<String, Vec<String>>, department: String) {
            if let Some(v) = deps.get_mut(&department) {
                v.sort();
                println!("Department {}", department);
                for emp in v {
                    println!("{}", emp);
                }
            } else {
                println!("Invalid department");
            }
        }
        match &self {
            Self::Add { name, department } => {
                deps.entry(department.to_string()).or_insert(vec![]).push(name.to_string());
                false
            },
            Self::List { department } => {
                print_department(deps, department.to_string());
                false
            },
            Self::All => {
                for (k, _) in deps.clone() {
                    print_department(deps, k.to_string());
                }
                false
            },
            Self::Quit => {
                true
            }
        }
    }
}

fn main() {
    let mut deps: HashMap<String, Vec<String>> = HashMap::new();
    let mut cmd = String::new();
    loop {
        cmd.clear();
        io::stdin().read_line(&mut cmd).expect("Cannot read command");
        match Command::from_string(&cmd) {
            Some(cmd) => match cmd.process(&mut &mut deps) {
                true => break,
                _ => (),
            },
            None => println!("Invalid command"),
        };
    }
}

Answer 1

Your code is good for the most parts, however there are some improvements that can be made:

---

This can be simplified:

Some(cmd) => match cmd.process(&mut &mut deps) {
    true => break,
    _ => (),
},

to

Some(cmd) => {
    if deps.process(cmd) {
        break;
    }
}

---

fn process(&self, deps: &mut &mut HashMap<String, Vec<String>>) -> bool {

No need for the double &mut.

---

fn process(&self, deps: &mut &mut HashMap<String, Vec<String>>) -> bool {
    fn print_department(deps: &mut &mut HashMap<String, Vec<String>>, department: String) {

The functions should not be nested.

---

I would define type aliases for department and person to make the code more readable.

type Department = String;
type Person = String;

fn process(&self, deps: &mut HashMap<Department, Vec<Person>>) -> bool {...}
fn print_department(deps: &mut HashMap<Department, Vec<Person>>, department: Department) {...}

---

process() should take self (consume the Command) to avoid cloning the Strings in it.

match &self {
    Self::Add { name, department } => {
        deps.entry(department.to_string()).or_insert(vec![]).push(name.to_string()); // No need to call to_string() here if we use self
        false
    },
    Self::List { department } => {
        print_department(deps, department.to_string()); // No need to call to_string() here if we use self
        false
    },
    Self::All => {
        for (k, _) in deps.clone() {
            print_department(deps, k.to_string()); // No need to call to_string() here if we use self
        }
        false
    },
    Self::Quit => {
        true
    }
}

---

You should restructure your code so that you are not cloning `deps` here:

Self::All => {
    for (k, _) in deps.clone() {
        print_department(deps, k.to_string());
    }
    false
},

Furthermore, you can use .keys() to get only the department name. You can also check if there are any departments and if not, display a message.

I would suggest creating a wrapper around the map of departments and implementing the methods on it, see Final code bellow.

---

Final code:

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

enum Command {
    Add { name: String, department: String },
    List { department: String },
    All,
    Quit,
}

impl Command {
    fn from_string(s: &str) -> Option<Self> {
        let el: Vec<_> = s.trim().split_whitespace().collect();

        match el.as_slice() {
            ["Add", name, "to", department] => Some(Self::Add {
                name: name.to_string(),
                department: department.to_string(),
            }),
            ["List", department] => Some(Self::List {
                department: department.to_string(),
            }),
            ["All"] => Some(Self::All),
            ["Quit"] => Some(Self::Quit),
            _ => None,
        }
    }
}

type Department = String;
type Person = String;

#[derive(Clone, Debug, Default)]
struct Departments {
    departments: HashMap<Department, Vec<Person>>,
}

impl Departments {
    pub fn new() -> Departments {
        Self {
            departments: HashMap::new(),
        }
    }

    pub fn process(&mut self, command: Command) -> bool {
        match command {
            Command::Add { name, department } => {
                self.departments
                    .entry(department)
                    .or_insert(vec![])
                    .push(name);
                false
            }
            Command::List { department } => {
                self.print_department(&department);
                false
            }
            Command::All => {
                if self.departments.is_empty() {
                    println!("No departments");
                } else {
                    for department in self.departments.keys() {
                        self.print_department(department);
                    }
                }

                false
            }
            Command::Quit => true,
        }
    }

    fn print_department(&self, department: &Department) {
        if let Some(mut v) = self.departments.get(department).cloned() {
            v.sort();
            println!("Department {}", department);
            for emp in v {
                println!("{}", emp);
            }
        } else {
            println!("Invalid department");
        }
    }
}

fn main() {
    let mut deps = Departments::new();
    let mut cmd = String::new();

    loop {
        cmd.clear();
        io::stdin()
            .read_line(&mut cmd)
            .expect("Cannot read command");
        match Command::from_string(&cmd) {
            Some(cmd) => {
                if deps.process(cmd) {
                    break;
                }
            }
            None => println!("Invalid command"),
        };
    }
}

---

If you want to make even more improvements, I would suggest replacing the vector of people with a structure that keeps the order when adding elements, to avoid sorting the vector every time. You can also create structs for Department and Person and implement Display on them, which would be more idiomatic than having a separate function that handles the printing.

I would also recommend using rust-clippy to help you write more idiomatic code.