Feedback on Rustling exercise - hashmaps3

code review
1

Hey everyone, I am new to Rust and programming in general. Nearly finished the Rust book and am working through the Rustlings exercises. I just completed hashmaps3, after a bit of a struggle and would love some feedback on my code and approach.

I ended up implementing an add() method on the struct and calling it in add_modify()'s closure body.

I also removed the name from the struct definition as it's not really necessary and eliminates the need to clone it a second time.

use std::collections::HashMap;

// A structure to store team name and its goal details.
#[derive(Debug)]
struct Team {
    goals_scored: u8,
    goals_conceded: u8,
}

// create a method on Team struct that also adding fields together. 
impl Team {
    pub fn add(&mut self, conceded: u8, scored: u8) -> Team {
        self.goals_conceded = self.goals_conceded + conceded;
        self.goals_scored = self.goals_scored + scored; 

        return Team {goals_conceded: self.goals_conceded, goals_scored: self.goals_scored}
    }

}

fn build_scores_table(results: String) -> HashMap<String, Team> {
    // The name of the team is the key and its associated struct is the value.
    let mut scores: HashMap<String, Team> = HashMap::new();

    for r in results.lines() {
        let v: Vec<&str> = r.split(',').collect();
        let team_1_name = v[0].to_string();
        let team_1_score: u8 = v[2].parse().unwrap();
        let team_2_name = v[1].to_string();
        let team_2_score: u8 = v[3].parse().unwrap();
        scores.entry(team_1_name.clone())
            // if entry exists add fields to fields. 
            .and_modify(|t| { t.add(team_2_score, team_1_score);})
            .or_insert(Team{goals_scored: team_1_score, goals_conceded: team_2_score});

        scores.entry(team_2_name.clone())
            .and_modify(|t| { t.add(team_1_score, team_2_score);})
            .or_insert(Team{goals_scored: team_2_score, goals_conceded: team_1_score});
        

    };
    return scores
}

Any feedback/tips would be great, thank you :slight_smile:

1 Like
2

The code can be simplified quite a bit; there are several unnecessary allocations/clones/explicit returns. The return value of Team::add() seems to be ignored, but I didn't change the signature. Here's the updated code.

2 Likes
3

Why does your Team::add method return a copy of self? In the context it seems unnecessary (though maybe in a greater context beyond the snippet it does?).

Other than that, there are a few style issues (explicit returns and you didn't run rustfmt), but these are obviously minor.

Instead of allocating a Vector, you could use the iterator directly:

let mut v = r.split(',');
        
let team_1_name = v.next().unwrap().to_string();
let team_2_name = v.next().unwrap().to_string();
  
let team_1_score: u8 = v.next().unwrap().parse().unwrap();
let team_2_score: u8 = v.next().unwrap().parse().unwrap();

You don't need to clone your strings here.

Otherwise looks good to me :slightly_smiling_face:

1 Like
4

Thanks for the feedback. The Team::add() implementation is much cleaner. And I guess i do not need any clones whatsoever, that's nice!

Thanks for looking for it over :slight_smile: .

5

All good points. using the iterator makes sense and the ability to just call next() makes it more legible.

I am not clear on the first point:

Why does your Team::add method return a copy of self ?

Would this not return an owned struct Team?

Ya that extra clone definitely was not necessary.

Thanks for the tips and the review.

6

The point is, it seems unnecessary to return anything, because you ignore the return value anyway (you call the add() method twice, but you don't do anything with its return value in either case).

2 Likes
7

Oh of course that makes sense. So I could even change the return type to () and remove the *self.

1 Like
8

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.