Calculate mean, median, and mode from list of integers

Question

I am trying to solve a problem put in The Rust Programming Language #2

Given a list of integers, use a vector and return the mean (average), median (when sorted, the value in the middle position), and mode (the value that occurs most often; a hash map will be helpful here) of the list.

I decided to put the values in a simple struct and return a Result. I want to be sure I'm using all the language features within the calculations and I want to reduce the amount of casts if possible.

use std::collections::HashMap;

/// structure to represent the values after calculating the mean,
/// median, and mode of some integers.
/// the mode Vec holds each individual mode.
#[derive(Debug)]
pub struct Calculations {
    mean: f64,
    median: f64,
    mode: Vec<i32>,
}

/// calculates the mean, median, and mode of values.
/// returns a Result holding a Calculations or an error string
/// when given an empty slice
pub fn calculate_mmm(values: &[i32]) -> Result<Calculations, &str> {
    // if input is an empty slice, return an error message
    if values.is_empty() { return Err("Found empty slice") }

    // add all numbers together and divide by length
    let mean = values.iter().fold(0, |p, &q| p + q) as f64 / values.len() as f64;

    // sort all values and return middle element or average of middle 2 elements
    let median = {
        let mut sorted = values.to_vec();
        sorted.sort();

        if sorted.len() % 2 == 0 {
            (sorted[sorted.len() / 2] + sorted[(sorted.len() / 2) + 1]) as f64 / 2.0
        } else {
            sorted[(sorted.len() as f64 / 2.0) as usize] as f64
        }
    };

    // holds modes
    let mut mode: Vec<i32> = Vec::new();

    // holds each number and the number of times it occures
    let mut occurrences = HashMap::new();

    for i in values {
        let count = occurrences.entry(i).or_insert(0);
        *count += 1;
    }

    // the maximum times a value occurres
    let mut max_value = 0;

    for &value in occurrences.values() {
        if value > max_value {
            max_value = value;
        }
    }

    // find the numbers which occur the maximum number of times
    for (key, value) in occurrences {
        if value == max_value {
            mode.push(*key);
        }
    }

    // return all values
    Ok(
        Calculations { mean: mean as f64, median: median as f64, mode: mode }
    )
}

Answer 1

1. There's no main method and more importantly, there are no tests. How do you know if the code works?

2. It's probably an artifact of learning a new language, but the code is overly documented. There's no reason to have comments describing what the implementation of the code is. It's more important to describe why a given implementation was chosen.

3. I would break out smaller functions to have somewhere for names to hang off of.

4. Documentation aspects:

   1. "returns a ..." in docs is not useful; the return type already says that.

   2. It is good to say why an error can occur though. You can also create a named error type that might have a variant like Error::EmptySlice, which is more self-describing.

   3. "structure to ..." is docs is not useful; it is a struct.

   4. "represent" is all programs do, so is redundant.

   5. Avoid referring to concrete implementation details ("... is a Vec ...")

5. You have a good use of fold, but sum exists.

6. Median is the mode of the middle of the sorted vector. Now that there are functions, you can reuse them.

7. Why convert to a floating point, divide, then back to a usize? (sorted.len() as f64 / 2.0) as usize)

8. Don't declare the type of mode; let the compiler's type inference handle it.

9. Use Iterator::filter instead of the if in the loop.

10. Use Iterator::map instead of the manual destructuring in the for loop for (key, value).

11. Use Iterator::collect to create the Vec instead of creating it and pushing to a Vec.

12. I'd oneline the counting with the HashMap.

13. Use Iterator::max and unwrap_or to find the biggest value instead of doing it by hand.

14. You could track the maximum value during the first loop. This would save one iteration, but it's not as pretty.

15. The casts to f64 wen creating the structure are redundant; the values are already f64.

use std::collections::HashMap;

/// The mean, median, and mode of some integers.
#[derive(Debug)]
pub struct Calculations {
    mean: f64,
    median: f64,
    mode: Vec<i32>,
}

fn mean(values: &[i32]) -> f64 {
    let sum: i32 = values.iter().sum();
    sum as f64 / values.len() as f64
}

/// Returns the middle element of the elements in sorted order (or the
/// average of the middle 2 elements)
fn median(values: &[i32]) -> f64 {
    let mut sorted = values.to_vec();
    sorted.sort();

    let mid = sorted.len() / 2;

    if sorted.len() % 2 == 0 {
        mean(&sorted[(mid - 1)..(mid + 1)])
    } else {
        sorted[mid] as f64
    }
}

fn mode(values: &[i32]) -> Vec<i32> {
    let mut occurrences = HashMap::new();

    for i in values {
        *occurrences.entry(i).or_insert(0) += 1;
    }

    let max_value = occurrences.values().cloned().max().unwrap_or(0);

    occurrences.into_iter()
        .filter(|&(_, v)| v == max_value)
        .map(|(&k, _)| k)
        .collect()
}

/// Calculates the mean, median, and mode of values.
/// It is an error to pass an empty slice
pub fn calculate_mmm(values: &[i32]) -> Result<Calculations, &str> {
    if values.is_empty() {
        Err("Found empty slice")
    } else {
        Ok(Calculations {
            mean: mean(values),
            median: median(values),
            mode: mode(values),
        })
    }
}

fn main() {
    println!("{:?}", calculate_mmm(&[3, 2, 1, 2]));
    println!("{:?}", calculate_mmm(&[3, 2, 1]));
}