HashMap with string keys

[mvolkmann]

The code below works, but I suspect someone with more Rust experience would write it differently.
How would you improve this?
I imagine I would also add use of the Iterator min_by_key method, but it probably wouldn't be fewer lines of code.

use std::collections::HashMap;

fn get_shortest(months: HashMap<String, i8>) -> String {
    let mut shortest: i8 = 32;
    let mut name = String::new();
    for (key, val) in months.iter() {
        if val < &shortest { // not happy with needing &
            name = key.to_string(); // not happy with needing .to_string()
            shortest = *val; // not happy with needing *
        }
    }
    name
}

fn main() {
    let mut days_in_month: HashMap<String, i8> = HashMap::new();
    days_in_month.insert("January".to_string(), 31);
    days_in_month.insert("February".to_string(), 28);
    days_in_month.insert("March".to_string(), 31);
    days_in_month.insert("April".to_string(), 30);
    println!("shortest = {}", get_shortest(days_in_month));
}

(Playground)

Output:

shortest = February

Errors:

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.59s
     Running `target/debug/playground`

[SkiFire13]

.iter() is used to iterate over shared references to the elements, however it looks like you want to iterate over owned elements (consuming the map in the process), for that you'll need to call .into_iter()

use std::collections::HashMap;

fn get_shortest(months: HashMap<String, i8>) -> String {
    let mut shortest: i8 = 32;
    let mut name = String::new();
    for (key, val) in months.into_iter() {
        if val < shortest {
            name = key; 
            shortest = val;
        }
    }
    name
}

fn main() {
    let mut days_in_month: HashMap<String, i8> = HashMap::new();
    days_in_month.insert("January".to_string(), 31);
    days_in_month.insert("February".to_string(), 28);
    days_in_month.insert("March".to_string(), 31);
    days_in_month.insert("April".to_string(), 30);
    println!("shortest = {}", get_shortest(days_in_month));
}

[mvolkmann]

Wow, that one change fixed all my concerns!

[mvolkmann]

For completeness, here is what seems to be a better version that takes into account that the HashMap could be empty.

use std::collections::HashMap;

fn get_shortest(months: &HashMap<String, i8>) -> Option<&String> {
    let mut shortest: i8 = 32;
    let mut name = None;
    
    // The months HashMap owns its keys and values.
    // The iter method iterates over shared references to elements.
    // The into_iter methods iterates over owned elements
    for (key, val) in months.into_iter() {
        if *val < shortest {
            name = Some(key);
            shortest = *val;
        }
    }
    name
}

fn main() {
    let mut days_in_month: HashMap<String, i8> = HashMap::new();
    days_in_month.insert("January".to_string(), 31);
    days_in_month.insert("February".to_string(), 28);
    days_in_month.insert("March".to_string(), 31);
    days_in_month.insert("April".to_string(), 30);
    
    if let Some(shortest) = get_shortest(&days_in_month) {
        println!("shortest = {}", shortest); // February
    }
}

[cliff]

As @SkiFire13 pointed out, you can just use a consuming iterator, but assuming you wanted to keep around your HashMap for later, you would need to change your function signature to fn get_shortest(months: &HashMap<String, i8>) -> String.

val is an &i8, and shortest is an i8, so if you want to assign or compare them, you have to make the types the same. I would tend to prefer *val < shortest over val < &shortest, because it reads nicer when I convert the syntax to words in my head. *val < shortest means "the thing pointed at by val is less than shortest", while val < &shortest means "val is less than a reference to shortest," but then you're dealing with comparisons of references, which is a little less obvious than comparing simple numeric values. Either way, it's a little nicer to have either & or * in both places, rather than using each one once.

In this case, since i8 implements Copy, you can just bind val directly to the value from the HashMap in your loop condition:

for (key, &val) in months.iter() {...}

(incidentally, you can also rewrite months.iter() as &months)

for (key, &val) in &months {...}

You can't do the same for key, though, because String doesn't implement Copy. What you can do is postpone allocating a new string until you know which value is the real shortest, and only call .to_string() when you're ready to return it.

pub fn get_shortest(months: &HashMap<String, i8>) -> String {
    let mut shortest: i8 = 32;
    let mut name: &str = "";
    for (key, &val) in months.iter() {
        if val < shortest {
            name = key;
            shortest = val;
        }
    }
    name.to_string()
}

Now, that's still assuming that I want to return an owned String. What I would really really do is return an &str that just references the String in my HashMap, and let the consumer take ownership of it if they need to. For kicks, I'd also use i8::MAX instead of 32 for the initialized value of shortest, so the function is correct for any HashMap that matches the signature.

pub fn get_shortest(months: &HashMap<String, i8>) -> &str {
    let mut shortest: i8 = std::i8::MAX;
    let mut name: &str = ""; 
    for (key, &val) in months.iter() {
        if val < shortest {
            name = key;
            shortest = val;
        }
    }
    name
}

[scottmcm]

@mvolkmann

fn get_shortest(months: &HashMap<String, i8>) -> Option<&String> {
    let mut shortest: i8 = 32;
    let mut name = None;
    
    // The months HashMap owns its keys and values.
    // The iter method iterates over shared references to elements.
    // The into_iter methods iterates over owned elements
    for (key, val) in months.into_iter() {
        if *val < shortest {
            name = Some(key);
            shortest = *val;
        }
    }
    name
}

As you mentioned in the OP, you can do this with min_by_key:

fn get_shortest(months: &HashMap<String, i8>) -> Option<&String> {
    months.iter()
        .min_by_key(|p| p.1)
        .map(|p| p.0)
}

Up to you whether you like that better.