3
\$\begingroup\$

As I'm learning Rust using the second edition of the Rust book I solved the following exercise:

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.

I could find this solution: Rust exercise department employee management (which did not receive a lot of feedback).

Do you have any feedback on my implementation? The code works, but I can imagine that some refactoring would be good. It already took me a lot of trial and error to get this working :-)

Suggestions welcome!

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


fn main() {
    let mut employees:HashMap<String, Vec<String>> = HashMap::new();
    println!("Start adding employees or type 'Done' when finished!");
    loop {
        let mut input = String::new();
        io::stdin().read_line(&mut input).expect("error: unable to read user input");
        let parsed_input = dissect_input(input.clone());
        match parsed_input {
            Ok(v) => {
                let (dept, name) = v;
                employees.entry(dept.to_string())
                    .or_insert_with(Vec::new)
                    .push(name)
            }
            Err(e) => {
                if e == "End" {
                    println!("Processed all input, good!\n\n");
                    break;
                } else {
                    println!("Input error!");
                    continue;
                }
            }
        }
    }
    println!("Type the name of a department to get its employees.");
    println!("Type 'All' to get all employees by department.");
    println!("Type 'Quit' to quit.");
    loop {
        let mut command = String::new();
        io::stdin().read_line(&mut command).expect("error: unable to read user input");
        match command.trim() {
            "Quit" => break,
            "All" => {
                for (dept, names) in &employees {
                    let mut names = names.clone();
                    names.sort();
                    for name in names {
                        println!("{}: {}", dept, name);
                    }
                }
            }
            _ => {
                match employees.get(&command.trim().to_string()) {
                    Some(names) => {
                        for name in names { println!("{}: {}", command.trim().to_string(), name); }
                    }
                    None => println!("I don't recognize that!")
                    }
                }
            }
        }
    println!("Have a nice day!");
    }


fn dissect_input(s: String) -> Result<(String, String), String> {
    match s.trim().as_ref() {
        "Done" => Err("End".to_string()),
        _ => {
            let words: Vec<&str> = s.split_whitespace().collect();
            if words.len() != 4 {
                Err("PEBCAK".to_string())
            } else {
            Ok((words[3].to_string(), words[1].to_string())) // “Add Sally to Engineering”
            }
        }
    }

}

// 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.
```
\$\endgroup\$
5
\$\begingroup\$

To start, I have some general tips. Run your code through rustfmt to get consistent formatting and make code easier to read. Run your code through clippy to get tips on common mistakes (clippy had nothing to say about your code, nice!)

One overall thing I'd suggest is to not be afraid to use a custom type in a case like this. Instead of returning a Result<(String, String), String> where the Err has multiple meanings based on value, just go ahead and make an Enum for your commands. I'll show you this in my final example, along with merging the two command sections (which the exercise is kinda fuzzy on whether it cares which way you do it).

Onto specific things:

You can get an iterator over lines of stdin by .lock().lines(), which can make this simpler, but that's a matter of preference over using read_line. If you are going to use read_line, you might as well reuse your input buffer, since that's the whole point of it taking a buffer as a parameter instead of just returning a String. That way you aren't reallocating each time.

let mut stdin = io::stdin()
let mut input = String::new();
loop {
    input.clear();
    stdin.read_line(&mut input).unwrap();
    // ...
}

dissect_input should take a &str instead of a String, since you only need to view it.

Down in the listing part, you do &command.trim().to_string(), which is unneeded in a few ways. .to_string() takes you from a &str to a String, and then the & brings you right back to a &str, so you could just do command.trim(). Also, you can just bind in the match you already have, like dept => employees.get(dept)

Overall, this is a really good first attempt! It can be hard to first wrap your head around the things that make rust different, like lifetimes and slices, but just getting things working is a great step.

Here's how I might choose to do this exercise.

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

// required trait for .lines()
use std::io::BufRead;

fn main() {
    let mut employees: HashMap<String, Vec<String>> = HashMap::new();
    let stdin = io::stdin();
    println!("Type 'Add <name> to <department>' to add an employee");
    println!("Type 'List <department>' to list the employees of a department");
    println!("Type 'All' to list all employees by department");
    println!("Type 'Quit' to quit");
    for line in stdin.lock().lines() {
        let input = line.expect("error: unable to read user input");
        match Command::from_input(&input) {
            // or_default is just a convenience, does the same as or_insert_with(Vec::default)
            Some(Command::Add { dept, name }) => employees.entry(dept).or_default().push(name),
            Some(Command::List(dept)) => match employees.get(&dept) {
                Some(names) => {
                    for name in names {
                        println!("{}: {}", dept, name);
                    }
                }
                None => println!("I don't recognize that department!"),
            },
            Some(Command::All) => {
                for (dept, names) in &employees {
                    let mut names = names.clone();
                    names.sort();
                    for name in names {
                        println!("{}: {}", dept, name);
                    }
                }
            }
            Some(Command::Quit) => break,
            // consider using eprintln, which prints to stderr
            None => println!("Input error!"),
        }
    }
    println!("Have a nice day!");
}

enum Command {
    // Using named fields instead of Add(String, String) because dept and name
    // are the same type and could get mixed up.
    Add { dept: String, name: String },
    List(String),
    All,
    Quit,
}

impl Command {
    fn from_input(s: &str) -> Option<Self> {
        let words: Vec<&str> = s.trim().split_whitespace().collect();
        // "Slice destructuring / slice pattern matching" for more info
        match words.as_slice() {
            ["All"] => Some(Command::All),
            ["Quit"] => Some(Command::Quit),
            ["List", dept] => Some(Command::List(dept.to_string())),
            ["Add", name, "to", dept] => Some(Command::Add {
                dept: dept.to_string(),
                name: name.to_string(),
            }),
            _ => None,
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, that was very helpful! I see how that enum and its ::from_input makes things a lot neater. Your changes for sure make things more readable, elegant and most probably more flexible to any future changes. I'll need to practice some more before I think of solutions like that on my own :-) One question: is the or_default method based on the type annotation which was specified when creating the hashmap? \$\endgroup\$ – Wouter De Coster Feb 11 '19 at 15:05
  • 1
    \$\begingroup\$ Its a little unclear what you're asking, but I believe yes. Here's the source, in case you want to see. Notice that it requires V: Default. \$\endgroup\$ – JayDepp Feb 11 '19 at 21:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.