The Rust Book: HashMap Challenge

Question

I'm working my way through the Rust Book:

Ch.8 Challenge:

Using a hash map and vectors, create a text interface to allow a user to add employee names to a department in the 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.

use std::collections::HashMap;
#[derive(Debug)]
enum Command {
 Add(String, String),
 List,
 Unknown
}
fn command_filter(input: String) -> Command {
 let mut command_string = input.split_whitespace();
 match command_string.next() {
 Some("Add") => {
 match (command_string.next(), command_string.last()){
 (Some(name), Some(dept)) => Command::Add(name.to_string(), dept.to_string()),
 _ => Command::Unknown
 }
 },
 Some("List") => Command::List,
 _ => Command::Unknown
 }
}
fn process_command(command: Command, employee_map: &mut HashMap<String, String>) {
 match command {
 Command::Add(name, dept) => {
 employee_map.insert(name, dept);
 ()
 },
 Command::List => {
 for (name, dept) in employee_map {
 println!("Name: {} Dept: {}", name, dept);
 };
 },
 Command::Unknown => println!("Unknown command!"),
 };
}
fn main() {
 let mut employee: HashMap<String, String> = HashMap::new();
 let command = command_filter(String::from("Add John to Eng"));
 process_command(command, &mut employee);
 let command = command_filter(String::from("Add Devon to Bio"));
 process_command(command, &mut employee);
 let command = command_filter(String::from("List")); 
 process_command(command, &mut employee); 
}

As with most devs starting to learn Rust I am finding difficulty with borrowing. I do not come from a C/C++ background and so I'm also finding pointers versus actual object a little tricky too.

Some specific thoughts:

1. I am suspicious that match statements inside match statements is not good practice.
2. The Command::Add branch in process_command feels a little hacky with the () return. Edit: After thinking more about this problem, I think if I wrapped the branch responses in Ok() then I could return a Result from that function.

Do you have any feedback?

• Rene Saarsoo: To get better feedback I suggest you add some comments of your own. What do you think might be lacking? Are there things you're not quite sure of? Which part did you find the hardest to implement?

Answer 1

General

1. It feels strange to have functions just hanging around; why not create some methods on types? I moved command_filter to From and moved process_command to Command::process. The latter would probably make more sense as a process method on a EmployeeMap type, though.

Command

1. It's unclear what the two values are for Command::Add - use named fields instead.

2. Could contain &strs since they live shorter than the string they parsed.

   Ah I see, and I would only need a new string (and hence allocation?) if the lifetime of the string was shorter than the slice?

   You would need to use a String (which is an allocation, yes) if the Command needs to contain the string value longer than the input string value is available. In this case, all of your input strings are hard-coded literals so they are available "forever". In a different case, you might have read some user input into another String. So long as the Command went out of scope before that String moved, it would be good.

   Lifetimes are hard

   Yes... and no. The thing is that languages like C or C++ have the exact same problems, but the languages don't stop you from doing The Wrong Thing. Languages with a garbage collector don't allow you to achieve the same level of efficiency. Rust's lifetimes allow you to be efficient while preventing memory unsafety.

command_filter

1. Could take a &str as an argument since it doesn't make use of the allocation of the String.

2. This advances the split_whitespace iterator before checking the result of a previous next call:

   match (command_string.next(), command_string.last())
   

   This isn't guaranteed to do what you want:

   calling next() again may or may not eventually start returning Some(Item) again at some point.

   You should use Iterator::fuse.

3. Could choose to flatten out the match to one level, but this would require advancing the iterator when you don't need to. Nested matches aren't inherently bad.

process_command

1. You don't need to say () as the return expression, that's the default value for a statement (lines ending in ;).

main

1. There's no reason to specify the type of employees, the compiler can infer it.

2. employees is a collection, so it should be a plural noun.

use std::collections::HashMap;
#[derive(Debug)]
enum Command<'a> {
 Add { name: &'a str, dept: &'a str },
 List,
 Unknown,
}
impl<'a> From<&'a str> for Command<'a> {
 fn from(input: &'a str) -> Self {
 let mut command_string = input.split_whitespace().fuse();
 match command_string.next() {
 Some("Add") => {
 match (command_string.next(), command_string.last()) {
 (Some(name), Some(dept)) => Command::Add { name, dept },
 _ => Command::Unknown,
 }
 },
 Some("List") => Command::List,
 _ => Command::Unknown,
 }
 }
}
impl<'a> Command<'a> {
 fn process(self, employees: &mut HashMap<String, String>) {
 match self {
 Command::Add { name, dept } => {
 employees.insert(name.to_owned(), dept.to_owned());
 }
 Command::List => {
 for (name, dept) in employees {
 println!("Name: {} Dept: {}", name, dept);
 }
 }
 Command::Unknown => println!("Unknown command!"),
 };
 }
}
fn main() {
 let mut employees = HashMap::new();
 let command = Command::from("Add John to Eng");
 command.process(&mut employees);
 let command = Command::from("Add Devon to Bio");
 command.process(&mut employees);
 let command = Command::from("List");
 command.process(&mut employees);
}

• Mungo Dewar: Wow, thank you so much! General: I forgot that you can implement methods on enums. That makes a ton of sense. Command: 1. Nice! Didn't know that could be done/was valid syntax. 2. Ah I see, and I would only need a new string (and hence allocation?) if the lifetime of the string was shorter than the slice? Lifetimes are hard. command_filter: 1. Does this mean memory allocation? 2. I don't understand, I thought I created the Iterator and immediately checked the next value - how does fuse protect me? process_command: Of course, whoops.
• Shepmaster: @MungoDewar updated to address some feedback.
• Mungo Dewar: Wanted to share updated code. I've included the comments listed here and have also hidden the Hashmap implementation behind the App struct. github.com/DewarM/rust_manage_employee
• Shepmaster: @MungoDewar I believe you are also encouraged to provide your own answer here, expanding on that comment and explaining why you made the changes you did.

Answer 2

Hide hashmap implementation

The code can be improved by removing the requirement for the user to operate on a hashmap or to even know that a hashmap is used.

How? This should be hidden from the user by an "interface".

Why? This is a good coding practice as it would allow the underlying structure/code to change (for example, the hashmap could be swapped for a db) but there would be no API changes to the interface.

I have implemented this change and the changes suggested by @Shepmaster here: github