The book exercises - averages

Hi All,
My friend and I are learning rust after many years commercial experience in other languages. Loving it so far! We're comparing notes, but I'd like to get some feedback from the community too.

This is my code to the exercise at the end of chapter 8 of the 'book' here
It feels a reasonably elegant solution although far from 'perfect', but I see that Rust has slightly different techniques to other languages that I might not have picked up yet.

Please can you comment if you'd have done this in a different way? Any ways to improve performance, or any techniques you'd suggest I take a look at?


fn main() {
    let mut lst = vec![
        78, 55, 56, 99, 23, 55, 56, 55, 23, 9, 99, 1, 1, 1, 99, 66, 88,
    ];
    lst.sort();
    let total: i32 = lst.iter().sum();

    let modes = get_mode(&lst);
    let joined: String = modes.1.iter().map(|&x| x.to_string() + ",").collect(); 

    print!(
        "
        Len: \t{} items
        Total: \t{}
        Mean: \t{}
        Median:\t{}
        Modes with {} occurances: {}",
        lst.len(),
        total, 
        mean(&total, &lst),
        median(&lst),
        modes.0,
        joined
    );
}

// calculate the mean average
fn mean(total: &i32, lst: &Vec<i32>) -> i32 {
    total / (lst.len() as i32)
}

// get the median average
fn median(lst: &Vec<i32>) -> i32 {
    lst[lst.len() / 2]
}

// get the mode average
fn get_mode(lst: &Vec<i32>) -> (i32, Vec<i32>) {
    let mut scores = HashMap::new();

    // Use a hashmap to count occurances of each score
    for num in lst.iter() {
        let e = scores.entry(num).or_insert(0 as i32);
        *e += 1;
    }

    // Get the highest number of occurances
    let max_occurances = match scores.values().max() {
        Some(i) => i,
        None => &0,
    };

    // get all keys that have the value equal to max occruances, these are all matches for the mode average
    let m =
        scores
            .iter()
            .filter(|(&_key, &val)| val == *max_occurances)
            .map(|(&k, &_v)| *k).collect();
    
    (*max_occurances, m)
}

Welcome! Please edit your post to use code blocks (three backticks in a row on the lines just before and after each block) so that we can read it more easily.

thank you, edit complete :slight_smile:

1 Like
  • If you ever find yourself writing &Vec<T> in the arguments of a function, stop and write &[T] instead. You can still pass an argument of type Vec<T> with the new type (because Vec<T> has a suitable Deref implementation) and you get the added ability to pass only part of a Vec, or some other contiguous slice of T that doesn't live in a Vec at all. (This is the same reasoning behind preferring &str to &String in arguments, as @steffahn explained in the other thread.)

  • In the signature for mean, you should pass total by value, not by shared reference: total: i32, not total: &i32. Copying an i32 costs nothing at all over passing a pointer, and it makes the implementation simpler. This reasoning applies pretty much whenever you have an argument type that implements Copy, which includes all the primitive integer types, shared reference types, bool, Option<T> whenever T: Copy, etc.

  • This bit:

    let max_occurances = match scores.values().max() {
            Some(i) => i,
            None => &0,
        };
    

    could be written as

    let max_occurances = score.values.max().copied().unwrap_or(0);
    

    After this max_occurances will be an i32 instead of an &i32, which is preferable anyway (see above about Copy types), and you won't need the dereference * when you use it later.

  • In the closure argument to map you can use a double & to avoid dereferencing k explicitly:

    .map(|(&&k, _)| k)
    

    (Also there's no point putting & on a part of the pattern that you're ignoring anyway, like _v or _key in the previous closure. I would leave these as plain _.)

  • This is to some extent a matter of personal preference, but I recommend applying rustfmt to code that you post publicly. An easy way to do this is to paste it into play.rust-lang.org and then Tools > Rustfmt.

1 Like

I would recomment trying out clippy:

    Checking playground v0.0.1 (/playground)
warning: used sort instead of sort_unstable to sort primitive type `i32`
 --> src/main.rs:6:5
  |
6 |     lst.sort();
  |     ^^^^^^^^^^ help: try: `lst.sort_unstable()`
  |
  = note: `#[warn(clippy::stable_sort_primitive)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive

warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
  --> src/main.rs:29:27
   |
29 | fn mean(total: &i32, lst: &Vec<i32>) -> i32 {
   |                           ^^^^^^^^^ help: change this to: `&[i32]`
   |
   = note: `#[warn(clippy::ptr_arg)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
  --> src/main.rs:34:16
   |
34 | fn median(lst: &Vec<i32>) -> i32 {
   |                ^^^^^^^^^ help: change this to: `&[i32]`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
  --> src/main.rs:39:18
   |
39 | fn get_mode(lst: &Vec<i32>) -> (i32, Vec<i32>) {
   |                  ^^^^^^^^^ help: change this to: `&[i32]`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

warning: casting integer literal to `i32` is unnecessary
  --> src/main.rs:44:45
   |
44 |         let e = scores.entry(num).or_insert(0 as i32);
   |                                             ^^^^^^^^ help: try: `0_i32`
   |
   = note: `#[warn(clippy::unnecessary_cast)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast

warning: this pattern reimplements `Option::unwrap_or`
  --> src/main.rs:49:26
   |
49 |       let max_occurances = match scores.values().max() {
   |  __________________________^
50 | |         Some(i) => i,
51 | |         None => &0,
52 | |     };
   | |_____^ help: replace with: `scores.values().max().unwrap_or(&0)`
   |
   = note: `#[warn(clippy::manual_unwrap_or)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or

warning: 6 warnings emitted
1 Like

wow, a great review, thank you. I shall try each of your suggestions this evening and make sure I understand them, so I may be posting back if I need some clarity! (after trying the docs first, of course!)

A hint I give to anybody learning to code, which I try to follow myself, is, "For every hour you spend on a tutorial, spend an hour exploring and asking 'what if?' ". This way, we get 'deeper' and learn more, rather than just rushing on to the next chapter :slight_smile:

1 Like

Thank you,
On the snippet for getting the max_occurances, why does it need "copied()"? I tried without and the unwrap_or(0) needs to be &0 but I'm unsure why. I'm at chapter 8 in 'the book', so maybe that's covered later or I need to read about ownership again?

Similarly with the .map(|&&k ... I don;' follow why I need to double && (double referencing?).

I've only got a little experience with C, mostly I use c# in my day job so this concept is still relatively new to me. Apologies for asking for more of your time, I just like to fully understand what's happening so I can make best use of it.

I'll also look at rustfmt and clippy. I'm using VS code on windows, so maybe there are extensions.

Thanks again for the amazing feedback!

-Tony

Option::copied takes an Option<&T> and turns it into an Option<T>, provided that T: Copy. scores.values() is an iterator of &i32 (references to values stored in the HashMap), and the max method takes an iterator of U to an Option<U>, so the result of scores.values().max() is an Option<&i32> to which copied can be applied.

As for &&k, turns out you don't need the second & after all -- my mistake. Which means your original code with *k shouldn't have compiled...

Thank you again, with the &&k, if I don't have the 2nd &, then I need to use *k, so I think you were right in the first place, but I don't understand why.

Oh, it's because the type of m is inferred as HashMap<&i32, i32> based on this:

    for num in lst.iter() {
        let e = scores.entry(num).or_insert(0 as i32);
        *e += 1;
    }

where num: &i32. If you use for &num in lst.iter() instead and then |(&k, _)| k in the map you should be good to go. Sorry for the confusion!

ETA: maybe the key takeaway from this thread is "when you iterate over a collection in Rust, you usually end up with an iterator of references, and if you're working with Copy types you should eventually turn those references back into values".

1 Like