Refactor match expression with HashMap

code review
1

Hello there, Newbie rustacian here!
Going through some refactoring I found no simpler way to express the match expression bellow. Would anybody have a suggestion to make my code look prettier?

pub struct Election<T> {
    pub voters: HashMap<&'static str, Option<T>>,
    pub winner_threshold: u32,
}

impl<T> Election<T> {
    pub fn new(voters: &Vec<&'static str>, winner_threshold: u32) -> Election<T> {
        let voters: HashMap<&'static str, Option<T>> = (*voters).iter()
                                                                .map(|&v| (v, None))
                                                                .collect();
        Election { voters, winner_threshold, }
    }

    pub fn vote(&mut self, voter: &str, vote: T) {
        match &self.voters[voter] {
            None => *self.voters.get_mut(voter).unwrap() = Some(vote),
            _ => (),
        }
    }
}

Other suggestions, apart the match expression, are obviously welcomed too :slight_smile: .

2

  1. Always accept &[T] over &Vec<T>. The former is strictly superior than the latter.

  2. The&self.voters[voter] will panic if the voter is already in the hashmap. To avoid panic, use following pattern and handles all three cases explicitly.

match self.voters.get_mut(voter) {
    Some(Some(vref)) => *vref = vote,
    Some(None) => {},
    None => {...}
}
1 Like
3

std::collections::hash_map::Entry might help... Combined with Option::get_or_insert() to replace the match expression.

pun fn vote(&mut self, voter: &str, vote: T) {
    self.voters.entry(voter).or_insert(Some(vote)).get_or_insert(vote);
}

e: Since you are preallocating the HashMap, you can also simplify this a bit more:

pun fn vote(&mut self, voter: &str, vote: T) {
    self.voters.entry(voter).and_modify(|v| {
        v.get_or_insert(vote);
    });
}
2 Likes
4

If you're not going to insert into the hasmap, you may as well just use HashMap::get_mut because that expresses your intent more clearly.

if let Some(voter) = self.voters.get_mut(voter) {
    voter.get_or_insert(vote)
}
2 Likes
5

Hello KrishnaSannasi, thanks for the answer!
This approach looks the clearest for me by far. Although, I'd like to add a voter's vote only if he haven't voted yet. Isn't there a way to take Some(voter) only if self.voters.get_mut(voter) points to Some(None)?

6

That's what the code does. Option::get_or_insert only fills in the Option if it is in the None state. So if the voter already voted this is s no op, and if they have not voted, this fills in the Option

1 Like
7

Thanks for you answer parasyte !
Definitely the second is the more accurate and explicit.

8

It works!
Sorry for the confusion, I'll stick with this one then. thank you very much KrishnaSannasi :slight_smile:

9

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