2
\$\begingroup\$

This quiz is on The Rust Programming Language online ebook.

Given a list of integers, use a vector and return the mean (the average value), 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.

As a novice Rust programmer, I'd like get my code reviewed and learn more about programming Rust.

There are a couple of things I'd like to know:

  • getting input using io::stdin().read_line() is ok for this case,
  • if there's better looping (user-input) and quitting pattern,
  • handling user input errors with helpful error message (Your input is invalid. Please enter whitespace-separated integers.) then re-ask with prompt, instead of panicking,
  • whether type casting using as is commonly-used (ex: i32 -> f32),
  • implementation of get_median() and get_mode() can be simpler than now

Here's my code:

use std::collections::HashMap;
use std::io::{self, prelude::*};

fn main() {
    let stdin = io::stdin();

    'mainloop: loop {
        print!("Enter space-separated numbers (q for quit)> ");
        io::stdout().flush().expect("flush failed");

        let mut line = String::new();
        stdin.read_line(&mut line).expect("Failed to read line");

        if line == "q\n" {
            break 'mainloop;
        }

        // TODO: handling ParseIntError
        let nums: Vec<i32> = line
            .split_whitespace()
            .map(|s| s.parse::<i32>().unwrap())
            .collect();

        let mean = nums.iter().sum::<i32>() as f32 / nums.len() as f32;

        println!(
            "numbers: {:?}, mean: {}, median: {}, mode: {}",
            nums,
            mean,
            get_median(&nums),
            get_mode(&nums)
        );
    }
}

fn get_median(v: &Vec<i32>) -> f32 {
    if v.len() < 1 {
        return 0.0;
    }

    let mut vec = v.clone();
    vec.sort();
    if vec.len() % 2 == 1 {
        return *vec.get(vec.len() / 2).unwrap() as f32;
    }
    return (*vec.get(vec.len() / 2 - 1).unwrap() + *vec.get(vec.len() / 2).unwrap()) as f32 / 2.0;
}

fn get_mode(v: &Vec<i32>) -> i32 {
    let mut map = HashMap::new();
    for num in v {
        let count = map.entry(num).or_insert(0);
        *count += 1;
    }
    return **map.iter().max_by_key(|(_, v)| *v).unwrap().0;
}
  • This code is on GitHub also.
  • I found there are similar questions related to the same quiz on Code Review, but the implementations differ. So I think they're not duplicated question.
\$\endgroup\$
2
\$\begingroup\$

Good job! I'm sure there are problems with my take, but, for what it's worth, here's how I might suggest improving things a bit.

use std::collections::HashMap;
use std::io::{self, Write}; // Personal preference, but I prefer avoiding glob imports
use std::num::ParseIntError;

fn main() {
    let stdin = io::stdin();
    let mut stdout = io::stdout();

    // Allocate a String buffer once and keep reusing it, instead of allocating multiple times.
    let mut line = String::new();

    // No need to name the loop since it's the only one. Loop naming
    // isn't very idiomatic in rust; it's typically used only when there
    // are loops within loops and naming is needed to solve ambiguity.
    loop {
        print!("Enter space-separated numbers (q for quit)> ");
        // Unwrap here seems OK, since if flush() fails, something went terribly wrong.
        stdout.flush().unwrap();

        // Clear the String buffer to prepare for reuse.
        line.clear();
        // Unwrap here seems OK, for similar reasoning as above.
        stdin.read_line(&mut line).unwrap();

        if line == "q\n" {
            break;
        }

        // Let the closure passed to map return a Result, then collect into a
        // Result<Vec<_>, ParseIntError>, which will be Ok if all parsing succeeded
        // or Err if any parsing failed. After that, match on your Result to handle errors.
        let mut nums = match line
            .split_whitespace()
            .map(|s| s.parse::<i32>())
            .collect::<Result<Vec<i32>, ParseIntError>>()
        {
            Ok(vec) => {
                if vec.is_empty() {
                    // Error handling: If user didn't give you any input, try again.
                    continue;
                }
                vec
            }
            Err(_) => {
                // Error handling: If user gave you bad input, try again.
                println!("Invalid input. Only integer values are accepted. Please try again.");
                continue;
            }
        };

        // Casting i32 to f32 is fine.
        let mean = nums.iter().sum::<i32>() as f32 / nums.len() as f32;
        let median = get_median(&mut nums);
        let mode = get_mode(&nums);

        println!(
            "numbers: {:?}, mean: {}, median: {}, mode: {:?}",
            &nums, // No need to consume nums (even though in this case you never use it again)
            mean,
            median,
            mode,
        );

        break;
    }
}

// Avoid the clone by passing a mutable reference
fn get_median(vec: &mut Vec<i32>) -> f32 {
    // is_empty() is slightly more idiomtic than vec.len() < 1
    if vec.is_empty() {
        return 0.0;
    }

    vec.sort();

    // * `vec.len() / 2` is reusable code; so make it it's own varible
    // * Instead of using `vec.get(index).unwrap()`, use `vec[index]`,
    //   as we know our index is valid.
    let index = vec.len() / 2;

    if vec.len() % 2 == 1 {
        vec[index] as f32
    } else {
        (vec[index - 1] as f32 + vec[index] as f32) / 2.0
    }
}

// * Whenever you want to pass &Vec<_> as an argument to a function,
//   consider passing &[_] instead. It works with more types.
// * Mode is not necessarily unique; so return a Vec<i32>.
fn get_mode(slice: &[i32]) -> Vec<i32> {
    if slice.is_empty() {
        // bail early if the input is empty; so that we avoid
        // unnecessarily allocating a HashMap.
        return vec![];
    }

    // Waste a little space by allocating from the start a HashMap
    // of maximum possible size; this avoids the possibility of
    // having to rellocate once the size has grown bigger than
    // capacity. It's a micro optimization, but in general in rust
    // if you know how big a heap allocated thing (like Vec, String, HashMap)
    // etc. needs to be in advance; take advantage of that information.
    let mut map = HashMap::with_capacity(slice.len());
    for num in slice {
        let count = map.entry(num).or_insert(0);
        *count += 1;
    }

    // Unwrap is OK because we know the map is non-empty.
    let max_value = map.values().map(|v| *v).max().unwrap();

    // Use into_iter() as we don't need the map anymore and
    // that will more efficiently create the vec than
    // if we had used iter()
    let mut vec = map
        .into_iter()
        .filter_map(|(k, v)| if v == max_value { Some(*k) } else { None })
        .collect::<Vec<i32>>();

    // Sort because it makes the output deterministic
    vec.sort();

    vec
}
```
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.