Join and map command line arguments in Rust

Question

Is there a more natural way to do this in rust? I'm having a lot of difficulty trying to make this look clean. Type mismatches and borrowing have me stumped. What are some refactors that I could make?

Input

• Take in list of space-separated, command-line arguments

Output

• Book (join all arguments except the last)
• Reference (the last argument)
• File (looked up via the book)

I abbreviated the books_to_files mapping to only include a couple examples of books with different word lengths.

Examples

In:                             Out:

Genesis 1:1                     Genesis, 1:1, mhc1.txt
Song of Solomon 2:2-4           Song of Solomon, 2:2-4, mhc3.txt
Acts of the Apostles 3:3-9      Acts of the Apostles, 3:3-9, mhc6.txt

Source Code

use std::env;
use std::collections::HashMap;

fn main() {
    let args: Vec<String> = env::args().collect();

    let book: String;
    if args.len() == 3 {
        book = args[1].clone();
    } else if args.len() == 4 {
        book = args[1].clone() + " " + &args[2].clone();
    } else if args.len() == 5 {
        book = args[1].clone() + " " + &args[2].clone() + " " + &args[3].clone();
    } else if args.len() == 6 {
        book = args[1].clone() + " " + &args[2].clone() + " " + &args[3].clone() + " " + &args[4].clone();
    } else {
        panic!("Wrong number of arguments");
    }
    let reference: String = args[args.len()-1].clone(); 
    let file: String = get_file(&book);

    println!("{}, {}, {}", book, reference, file);
}

fn get_file(book: &String) -> String {
    let books_to_files: HashMap<&str, &str> = [
        ("Genesis", "mhc1.txt"),
        ("First Samuel", "mhc2.txt"),
        ("Song of Solmon", "mhc3.txt"),
        ("Acts of the Apostles", "mhc6.txt"),
        ("First Corinthians", "mhc6.txt"),
        ("Galatians", "mhc6.txt"),
    ].iter().cloned().collect();

    books_to_files.get::<str>(&book.to_string()).unwrap().to_string()
}

```

Answer 1

I've collected a few thoughts about possible refactors you could consider below.

Your if chain isn't the most efficient way

As you've written it, your code will only work for titles between one and four words long. This isn't a disaster, and it will probably work for most titles, but we can rewrite your code to be shorter and handle any length of title. A win-win!

• We can use Iterator::skip() to throw away the executable name, as we don't care about it anyway. See skip() in the documentation.

  let args: Vec<String> = env::args().skip(1).collect();
  

• It'd be a good idea to check that we've been given enough arguments before continuing.

  let arg_len: usize = args.len();
  if arg_len < 2 {
      panic!("At least two arguments must be supplied!");
  }
  

• Now, we can be a bit clever and avoid your if chain all together. Using a range we can get a slice of our args vector corresponding to all but the last element. We can then use join() to turn that slice back into a String. For example, if we started with vec!["Song", "of", "Solomon", "2:2-4"] in args, we would take a slice to get ["Song", "of", "Solomon"] and rejoin them with " " in the middle to get "Song of Solomon".

  let book = args[0..(arg_len - 1)].join(" ");
  

Try to avoid clone() unless necessary

The first thing I noticed when I took a look at your code was that you've used clone() in quite a few places. We've already got rid of a lot of them with the tweak above, and you should try to avoid them all together as allocating when you don't need to is a waste of time and memory. If you can use an &str, do that instead of insisting on all strings being String. As an aside, there is rarely any point bothering with &String — just use &str there.

• Putting the above into action, we can let reference be an &str and avoid a clone.

  let reference: &str = &args[args.len() - 1];
  

• Let's also change get_book's method signature as suggested above.

  fn get_file(book: &str) -> &str {
  

• Now we can get rid of the unnecessary to_string() below.

  books_to_files.get::<str>(&book.to_string()).unwrap()
  

• And tweak this line in main() to accept an &str.

  let file: &str = get_file(&book);
  

Avoiding unwrap

Ideally, get_book ought to return an Option instead, so the caller can choose how they want to handle the error. As it is fatal anyway, unwrapping makes little difference, but it's worth bearing in mind as a future improvement.

Try it Online

Answer 2

Rather than using a hashmap to lookup the books, we can use a match block. (This is assuming that your books are known at compile time.) Also, we can return an Option<&'static str>. The option lets the caller of the function decide what to do on failure, and a &'static str is the type of a string literal.

fn get_file(book: &str) -> Option<&'static str> {
    match book {
        "Genesis" => Some("mhc1.txt"),
        "First Samuel" => Some("mhc2.txt"),
        "Song of Solmon" => Some("mhc3.txt"),
        "Acts of the Apostles" => Some("mhc6.txt"),
        "First Corinthians" => Some("mhc6.txt"),
        "Galatians" => Some("mhc6.txt"),
        _ => None,
    }
}

Based on your response, I'll suggest having the first command line argument be the entire book name. I'd write main something like this. We can use slice patterns to check for the right number of arguments and bind them to variables at the same time. Then, we can try to find the file or print an error message otherwise. If the number of arguments was wrong, we also print an error showing the correct usage of the program.

fn main() {
    let args: Vec<String> = std::env::args().collect();

    if let [_, book, reference] = args.as_slice() {
        if let Some(file) = get_file(book) {
            println!("{}, {}, {}", book, reference, file);
        } else {
            eprintln!("Could not find book!");
        }
    } else {
        eprintln!("Usage: {} <BOOK> <REFERENCE>", args[0]);
    }
}

Answer 3

Here are some improvements:

• If you don't want the first arg, skip it
  • env::args().skip(1).collect()
• Get the reference (last arg) with pop
• Get the book (remaining args) with join
  • args.join(" ")
• Use &str when possible
  • get_file(book: &str)
• Match the book to the file, with match
  • match book {...}
• Use an Option/Some/None so that main can handle the error
  • get_file(...) -> Option<&str>

use std::env;

fn main() {
    let mut args: Vec<String> = env::args().skip(1).collect();
    let reference = args.pop()
        .expect("Expected 2+ parameters: <book of the Bible> <reference>");
    let book = args.join(" ");
    let file = get_file(&book)
        .expect("Not a valid book");
    println!("{}, {}, {}", book, reference, file);
}

fn get_file(book: &str) -> Option<&str> {
    match book {
        "Genesis" => Some("mhc1.txt"),
        "First Samuel" => Some("mhc2.txt"),
        "Song of Solmon" => Some("mhc3.txt"),
        "Acts of the Apostles" => Some("mhc6.txt"),
        "First Corinthians" => Some("mhc6.txt"),
        "Galatians" => Some("mhc6.txt"),
        _ => None
    }
}

Thanks to Aurora0001 and JayDepp for the helpful feedback (that make up 90% of the advice above). This code is a lot more natural now.