Clippy suggests: more directly with map_or_else(g, f) instead... recommendation does not compile

[EdmundsEcho]

Great tool! Thank you.

The code where clippy suggests I use a more direct approach:

        frequencies
            .get_mut(word)
            .map(|count| {
                *count += 1;
            })
            .unwrap_or_else(|| {
                frequencies.insert(word.to_owned(), 1);
            });

My interpretation of the suggestion:

        frequencies.get_mut(word).map_or_else(
            || {
                frequencies.insert(word.to_owned(), 1); 
                ^^^^^^^^^^^^  <<<< cannot borrow frequencies as mutable more than once at a time
            },
            |count| {
                *count += 1;
            },
        )

The caveat of course is that there is an easy fix to the error I'm receiving.

Notwithstanding, my first guess is that there is an inconsistency here:

• either the recommendation is right most of the time except in the situation I have here,
• or the map_or_else should work the same way as my first iteration of the code all the time.

I hope this is helpful.

- E

[flip1995]

This looks like you want to do this:

let count = frequencies.entry(word).or_insert(0);
*count += 1;

https://doc.rust-lang.org/std/collections/hash_map/struct.HashMap.html#method.entry

[EdmundsEcho]

Thank you for the pointer. I compiled the code to confirm that unfortunately the suggestion does not solve the issue. The motivation of the code as I have it, is to avoid instantiating a String before determining if I can get away with simply incrementing the count of the key already in the hash. The suggested code requires that word be a String, and thus instantiating accordingly, without regard of the existence of the key.

[flip1995]

You could try to do this with contains_key:

if freq.contains_key(word) {
    let count = freq.get_mut(word).unwrap();
    *count += 1;
} else {
    freq.insert(word.to_string(), 1);
}

[EdmundsEcho]

Thank you. This was useful. At this point only as a potentially useful FYI, the latest code executes consistently about 5-10% slower than the ugly syntax I was using. This experience has likely fallen into the scope of the methods provided by HashMap.

[flip1995]

Yeah iterators can be optimized more efficiently, than if-else-blocks. Have you tested the performance of the entry method (even with the allocation, I could imagine, that this is even faster)?

[EdmundsEcho]

Yes, I tried the code using entry. The performance was similar to the contains_key option; about 5-10% slower... I believe I saw somewhere a RFC suggesting an update or new function for HashMap to address this potentially missed opportunity.

[ghost]

What about this?

    match frequencies.get_mut(word) {
        Some(count) => { *count += 1; }
        None => { frequencies.insert(word.to_owned(), 1); }
    }

It should perform as the same as map_or_else as it's just an "inlining" of that method ( https://doc.rust-lang.org/src/core/option.rs.html#456-461 ).

IMO, it's better style to use match statements than side-effects in map calls.

[EdmundsEcho]

@mikerite - Your latest code performs as quickly as the original code I performed. The syntax is much nicer. Thank you!

FYI: I agree with you re-style of if/then versus match. clippy however suggested I use if/then for this use of match. I'm not sure clippy should opine one way or the other in this case.