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?

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);

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