How can I improve this?

Hi, fellow Crustaceans!
I am a newbie of Rust programming language. A nauplius.
Now I am learning THE BOOK. I have finished Chapter 8 and I tried the exercises.
Well, I'd have to say that Rust behaves quite differently from other programming languages in certain aspects and there were twists and turns when I tried to tell Rust what I wanted to do.
I finished exercise 3, but I suspect the code I wrote is not very Rust-y. So can anybody give me some advice on bettering the code?

main.rs

use std::collections::HashMap;
use std::io;
//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.
//utility functions are all place here
fn show_ops (){
    println!("Please choose your op:");
    println!("1 - List departments");
    println!("2 - Add an employee to a department");
    println!("3 - List employees in a department");
    println!("4 - List employees in the company");
    println!("0 - Exit");
    println!("-----------------------");
}
fn get_op_code() -> i32 {
    let mut info = String::new();
    io::stdin().read_line(&mut info).expect("Failed to get input");
    //info.trim().to_string();
    let op_code:i32=match info.trim().parse(){
        Ok(num) => num,
        Err(_) => -1,
    };
    op_code
}    
fn main() {
    println!("Employee System");
    //use this hashmap to store departments' info
    let mut departments = HashMap::new();
    let mut v:Vec=Vec::new();
    //when inserting the vector, do not use &mut v, insert v directly
    let mark = String::from("department");
    departments.insert(mark, v);
    departments.clear(); 
    let mut deplen:usize = 0;
    
    loop{
        show_ops();
        let opcode:i32 = get_op_code();
        if opcode == 0 {//exit
            println!("Good bye!");
            break;
        }else if opcode == 1{//list departments
            deplen=departments.len();
            println!("{} departments in record:",deplen);
            for (key,_) in &departments {
                println!("{}",key);
            }
        }else if opcode == 2{//add an employee
            println!("To add an employee, use instructions like: add Tom to Sales");
            println!("If the department doesn't exist, it will be created.");                      
            let mut info = String::new();
            let mut name:String = String::new();
            let mut nholder:String = String::new();
            let mut dept:String = String::new();
            let mut dholder:String = String::new();
            //get name and department
            io::stdin().read_line(&mut info).expect("Failed to get input");
            let mut i:i32=0;
            //in fact we only need words in pos 2 and 4
            for w in info.split_whitespace() {
                i += 1;
                if i == 2 {
                    name = String::from(w);
                    nholder = String::from(w);
                    println!("Name:{}",name);
                }else if i == 4 {
                    dept = String::from(w);
                    dholder = String::from(w);
                    println!("Dept:{}",dept);
                }else{
                    continue;
                }
            }
            if name == "" || dept == ""{
                println!("Input not complete?? Please try again.");
            }else{
                //check if department already exists
                if departments.contains_key(&dept){
                    //add the employee to an existing dept
                    if let Some(x) = departments.get_mut(&dholder) {
                        x.push(nholder);
                        x.sort();
                    } 
                }else{
                    //create a new dept and add the employee
                    let mut v:Vec=Vec::new();
                    v.push(nholder);
                    v.sort();
                    departments.insert(dholder, v);                
                }
                println!("Information added!!");                   
            }   
        }else if opcode == 3{//list employees in a department
            println!("Which department?");
            let mut info = String::new();
            io::stdin().read_line(&mut info).expect("Failed to get input");
            if departments.contains_key(info.as_str().trim()){
                println!("This department has:");
                for n in departments[info.as_str().trim()].iter(){
                    println!("{}",n);
                }
            }else{
                println!("{} department doesn't exist!!!",info.as_str().trim());
            }
        }else if opcode == 4{//list the whole company
            println!("{} departments in the company:",&departments.len());
            for (key,value) in &departments {
                println!("{} department has:",key);
                for n in value.iter(){
                    println!("{}",n);
                }
            }
        }else if opcode == -1{
            println!("Invalid Op!!!");
        }else{
            continue;
        }
        println!("-----------------------");
    }
}
1 Like

From first look I would advise you to make more use of white space, it helps a lot for readability.

You can also use cargo fmt to keep your code formatting consistent with what everyone else uses when coding in Rust.

1 Like

Thank you!
By whitespace do you mean more blank lines between functions?
I will try cargo fmt.

Yes but also between tokens. Usually we would have a space e.g. after a comma and between curly braces and keywords such as if and else.

1 Like

Thank you!
so if...else should be :

    if SomeCondition {
    } else {
    }

Right?

Usually I insert a blank line after variable declarations and before and after code blocks. So instead of

else if opcode == 2{//add an employee
    println!("To add an employee, use instructions like: add Tom to Sales");
    println!("If the department doesn't exist, it will be created.");                      
    let mut info = String::new();
    let mut name:String = String::new();
    let mut nholder:String = String::new();
    let mut dept:String = String::new();
    let mut dholder:String = String::new();
    //get name and department
    io::stdin().read_line(&mut info).expect("Failed to get input");
    let mut i:i32=0;
    //in fact we only need words in pos 2 and 4
    for w in info.split_whitespace() {
        i += 1;
        if i == 2 {
            name = String::from(w);
            nholder = String::from(w);
            println!("Name:{}",name);
        }else if i == 4 {
            dept = String::from(w);
            dholder = String::from(w);
            println!("Dept:{}",dept);
        }else{
            continue;
        }
    }
    .
    .
    .
}

I would write

// Add an employee
else if opcode == 2 {
    println!("To add an employee, use instructions like: add Tom to Sales");
    println!("If the department doesn't exist, it will be created."); 

    let mut info = String::new();
    let mut name:String = String::new();
    let mut nholder:String = String::new();
    let mut dept:String = String::new();
    let mut dholder:String = String::new();

    // Get name and department
    io::stdin().read_line(&mut info).expect("Failed to get input");
    
    // In fact we only need words in pos 2 and 4
    let mut i:i32=0;
    for w in info.split_whitespace() {
        i += 1;
        if i == 2 {
            name = String::from(w);
            nholder = String::from(w);
            println!("Name:{}",name);
        }
        else if i == 4 {
            dept = String::from(w);
            dholder = String::from(w);
            println!("Dept:{}",dept);
        }
        else{
            continue;
        }
    }
    .
    .
    . 
}

I think it makes the code more readable.

1 Like

cargo fmt is really COOL!!

2 Likes

Good one! Thanks!

A standard tips for discourse-forum-nauplii (great name by the way, I'm betting it will stick!): you can use ONE post to reply to multiple previous posts.
To do so,

  1. start a new reply, and
  2. with that reply open, select text you wish to respond to.
  3. A hoverbox will appear with a "quote" option, that inserts the proper forum quote syntax into your open reply.

You can repeat this multiple times, and Discourse will link all of the references.

2 Likes

Thanks!

1 Like

I would try to represent opcode as an Enum. Than use match instead of the long chain of else if.

Example of what it could look like:

fn get_op_code() -> i32 {
    //...
}

to

fn get_op_code() -> Option<OpCode> {
    //...
}

and

let opcode:i32 = get_op_code();
if opcode == 0 {//exit
    println!("Good bye!");
    break;
}else if opcode == 1{//list departments
    //...
} else if opcode == -1 {
    println!("Invalid Op!!!");
} else {
    continue;
}

to

match get_op_code() {
   Some(OpCode::Exit) => {
        println!("Good bye!");
        break;
   }
   Some(OpCode::ListDepartments) => {
        //...
   }
   None => {
        println!("Invalid Op!!!");
   }
}
3 Likes

Thank you!

You mean I should first define an enum like this:

enum OpCode{
    Exit,
    ListDepartments,
    AddEmployee,
    ListEmployeeInOneDept,
    ListEmployeeInCompany,
    InvalidOp,
    WrongInput,
}

Then change get_op_code() like this:

fn get_op_code() -> Option<OpCode> {
    let mut info = String::new();
    io::stdin().read_line(&mut info).expect("Failed to get input");
    let op_code:OpCode = match info.trim().parse(){
        Ok(0) => OpCode::Exit,
        Ok(1) => OpCode::ListDepartments,
        Ok(2) => OpCode::AddEmployee,
        Ok(3) => OpCode::ListEmployeeInOneDept,
        Ok(4) => OpCode::ListEmployeeInCompany,
        Ok(_) => OpCode::InvalidOp,
        Err(_) => OpCode::WrongInput,
    };
    Some(op_code)
}  

Then in fn main(), I should use

    match get_op_code() {
        Some(OpCode::Exit) => {
             println!("Good bye!");
        }
        Some(OpCode::ListDepartments) => {
             //...
        }
        Some(OpCode::AddEmployee) => {
            //...
        }
        Some(OpCode::ListEmployeeInOneDept) => {
        //...
        }
        Some(OpCode::ListEmployeeInCompany) => {
        //...
        }
        Some(OpCode::InvalidOp) => {
            //...
        }   
        Some(OpCode::WrongInput) => {
            //...
        }              
        None => {
            //...
        }
     }

Is that it?

Yeah exactly!

I might express OpCode::InvalidOp and OpCode::WrongInput as None.

But If you want to keep the cases separate you could actually drop the Option, since the function as written never returns None.


More advanced Rust:

Once you learn about Result and traits, I would split InvalidOp and WrongInput into a separate OpCodeError enum. Then implement FromString for OpCode. Implementing FromString allows you to call parse on a string and convert it into an OpCode.
Doing this will simplify the get_op_code function and prevent you from mixing valid cases with invalid cases.

fn get_op_code() -> Result<OpCode, OpCodeError> {
    let mut info = String::new();
    io::stdin().read_line(&mut info).expect("Failed to get input");
    info.trim().parse()
} 
1 Like

Thank you! I haven't reached Results and Traits yet. I will try harder.

1 Like

You might want to use two nested matches so that you don't have to repeat Some() for each enum value, i.e.

match get_op_code() {
    None => ...
    Some(op_code) => match op_code {
        OpCode::Exit => ...
        OpCode::ListDepartments =>
    }
}
2 Likes
match get_op_code() {
    None => ...
    Some(op_code) => match op_code {
        OpCode::Exit => ...
        OpCode::ListDepartments =>
    }
}

Thanks!!

And you can avoid saying OpCode:: over and over like so (although this is only worth doing if there are a lot of enum values):

use OpCode::*;
match get_op_code() {
    None => ...
    Some(op_code) => match op_code {
        Exit => ...
        ListDepartments =>
    }
}

Of course, you're bringing in a lot of symbols that way, so be judicious.

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.