Идиоматическое использование `Option` и` развёртывать` в программе Rust mean, median и mode для 8 главы Книги

Я недавно закончил 8-ю главу Книги и начал выполнять упражнения в конце главы. Этот пост относится к первому упражнению, которое включает в себя написание программы для вывода средних значений списка целых чисел. Мне показалось, что инструкции для этого конкретного упражнения немного расплывчаты, поэтому я попытался упростить его, не включив в свою реализацию функциональность REPL. Во всяком случае, вот мое текущее решение:


// 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.

use std::collections::HashMap;

struct Averages {
  mean: f64,
  median: f64,
  mode: Vec<i32>
}

fn sum(vector: &Vec<i32>) -> Option<i32> {
  match vector.len() {
    0 => None,
    _ => {
      let mut accumulator = 0;

      for element in vector {
        accumulator += element;
      }

      Some(accumulator)
    }
  }
}

fn mean(vector: &Vec<i32>) -> Option<f64> {
  match vector.len() {
    0 => None,
    length =>
      Some((sum(vector).unwrap() as f64) / (length as f64))
  }
}

fn is_even(number: &usize) -> bool {
  number % 2 == 0
}

fn median(vector_original: &Vec<i32>) -> Option<f64> {
  match vector_original.len() {
    0 => None,
    length => {
      let mut vector_cloned = vector_original.to_vec();

      vector_cloned.sort_unstable();
      match is_even(&length) {
        true => {
          let length_halved = length / 2;

          Some(((
            &vector_cloned[length_halved - 1] +
            &vector_cloned[length_halved]) as f64) / 2.0)
        },
        false =>
          Some(vector_cloned[((
            (length as f64) /
            2.0).floor() as usize)] as f64)
      }
    }
  }
}

fn mode(vector: &Vec<i32>) -> Option<Vec<i32>> {
  match vector.len() {
    0 => None,
    _ => {
      let mut counts = HashMap::new();

      for number in vector {
        let count = counts.entry(number).or_insert(1u32);
        *count += 1u32;
      }

      let mut max_count = 0;

      for count in counts.values() {
        if count > &max_count {
          max_count = *count;
        }
      }

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

      for (number, count) in &counts {
        if count == &max_count {
          modes.push(**number);
        }
      }

      Some(modes)
    }
  }
}

fn get_averages(vector: &Vec<i32>) -> Option<Averages> {
  match vector.len() {
    0 => None,
    _ => Some(
      Averages {
        mean: mean(vector).unwrap(),
        median: median(vector).unwrap(),
        mode: mode(vector).unwrap()
      }
    )
  }
}

fn print_averages(vector: &Vec<i32>) {
  match vector.len() {
    0 => println!("Vector is empty."),
    _ => {
      let averages: Averages = get_averages(vector).unwrap();

      println!(
"Mean: {}
Median: {}
Mode(s): {:?}",
        &averages.mean,
        &averages.median,
        &averages.mode
      );
    }
  }
}

fn main() {
  print_averages(&(10..=20).collect());
  print_averages(&vec![5,5,5,4,3,2,1,1,1]);
  print_averages(&vec![]);
}

Я написал текущую реализацию, как и я, когда пытался написать ее так, чтобы каждая из функций могла повторно использоваться независимо друг от друга и в контекстах, возможно, за пределами этой конкретной программы (отсюда, возможно, избыточные вызовы к len). Но теперь мне интересно, используется ли так много unwrapкак я, это идиоматический способ справиться с Options или если есть лучший способ написать это вместо этого. Меня особенно беспокоит, что моя реализация может оказаться хрупкой, потому что я столкнулся с парой паник, когда писал и тестировал ее.

Я также не уверен, оптимально ли я занимаюсь заимствованием и перемещением, а также ссылаюсь и разыменовываю, особенно в mode функция; несмотря на то, что он работает правильно, я немного смущен тем, что он на самом деле делает с данными и ссылками, поэтому некоторые пояснения по этому поводу были бы очень полезны.

Любая другая конструктивная критика в целом также приветствуется, так что, пожалуйста, придирайтесь к своим сердцам!

2 ответа
2

Хорошая работа в разделении того, что вы делали, на автономные функции. Некоторые комментарии по дизайну и стилю:

  • Перво-наперво: всегда запускайте Clippy! (cargo clippy) Его отзывы обычно полезны. Здесь он замечает несколько проблем:

    • Всякий раз, когда у вас есть функция с &Vec<T> в качестве аргумента его почти всегда следует переписывать как отрыв &[T]. Думайте о срезе как о более общей векторной памяти, поэтому функция, которая принимает &[T] более общий, чем тот, который принимает &Vec<T>. Это также имеет побочный эффект небольшого изменения вашей основной функции:

          fn main() {
              let v: Vec<i32> = (10..=20).collect();
              print_averages(&v);
              print_averages(&[5, 5, 5, 4, 3, 2, 1, 1, 1]);
              print_averages(&[]);
          }
      
    • В соответствии ((&vector_cloned[length_halved - 1] + &vector_cloned[length_halved]), вы можете удалить оба амперсанда.

  • В вашем sum функция, возвращающая 0 для суммы пустого списка (так, fn sum(vector: &[i32]) -> i32) чище и стандартнее. Также для этой функции есть встроенная функция: vector.iter().sum()

  • Ваш mean функция понятна, использование match vector.len() умный (альтернатива if vector.is_empty() {, но тогда тебе все равно придется позвонить .len() снова в ветке else. Так что мне больше нравится ваш выбор дизайна.)

  • Однако в отличие от mode и print_averages, здесь понятнее использовать if vector.is_empty() { поскольку вы не используете длину в логике.

  • Соответственно, в median когда вы используете match is_even(&length), здесь, я думаю, более ясно использовать оператор if, поскольку нет значения, связанного с одной из ветвей соответствия, к которой вам нужен доступ.

  • Также в median Я думаю, что часть логики можно прояснить, используя промежуточные переменные вместо сложных выражений. Особенно:

                let ele1 = vector_cloned[length_halved - 1];
                let ele2 = vector_cloned[length_halved];
                Some(((ele1 + ele2) as f64) / 2.0)
    

Чтобы ответить на ваши конкретные вопросы:

Но теперь мне интересно, является ли использование такого количества разверток, как я, идиоматическим способом обработки параметров, или есть лучший способ написать это вместо этого. Меня особенно беспокоит, что моя реализация может оказаться хрупкой, потому что я столкнулся с парой паник, когда писал и тестировал ее.

С помощью unwraps на самом деле это довольно оправдано: вы статически знаете, что эти векторы непусты, поэтому ваш unwraps не должно паниковать. Вы правы, хотя как обычно unwrapлучше избегать. Если хочешь, можешь сделать .expect("unreachable") чтобы указать, что именно пошло не так.

Я также не уверен, оптимально ли я занимаюсь заимствованием и перемещением, а также ссылками и разыменованием, особенно в функции режима.

Отредактировано для добавления: некоторые комментарии к функции режима:

Некоторые улучшения в использовании ссылок:

  • Вы создали HashMap<&i32, u32>; это приводит к странному разыменованию кода. Было бы лучше создать HashMap<i32, u32>. Вы можете убедиться, что делаете это правильно, используя аннотацию типа: let mut counts: HashMap<i32, u32> = HashMap::new();

  • Для первого цикла for вы можете использовать .and_modify для упрощения логики: (вектор заметки – это ссылка / срез):

            let mut counts: HashMap<i32, u32> = HashMap::new();

            for &number in vector {
                counts.entry(number).and_modify(|count| *count += 1).or_insert(1u32);
            }
  • Второго цикла for можно избежать, используя .max() непосредственно на итераторах:

let max_count: u32 = *counts.values().max().unwrap()

  • Как только у вас будет HashMap<i32, u32>, третий цикл for становится намного лучше. Вы можете полностью избежать ссылок, используя хэш-карту в цикле for:
            for (number, count) in counts {
                if count == max_count {
                    modes.push(number);
                }
            }

Но наиболее распространенный способ – итерация по ссылке, которая обычно записывается так:

            for (&number, &count) in &counts {
                if count == max_count {
                    modes.push(number);
                }
            }

Или, что то же самое, если вы предпочитаете:

            for (number, count) in &counts {
                if *count == max_count {
                    modes.push(*number);
                }
            }
  • Вы также можете выполнить последний цикл с фильтром вместо явного цикла for, хотя при обучении всегда полезно записывать циклы for для практики.

  • Также для более крупных проектов, в конечном итоге, лучший способ сделать это, вероятно, будет использовать выделенную коллекцию мультимножеств, например multiset ящик. Тело режима должно быть очень коротким, например let counts: MultiSet<i32> = vector.collect(); counts.iter().max_by_key(...).

    Сначала прочтите ответ 6005 – там освещены самые важные вопросы. Вот некоторые дополнения:

    Форматирование

    rustfmt форматирует ваш код согласно официальному Руководство по стилю Rust, с которым знакомо большинство Rustaceans, чтобы облегчить общение.

    Передача аргумента

    Как указал 6005, сделайте свои параметры &[i32] вместо того
    Vec<i32>. Увидеть Почему не рекомендуется принимать ссылку на
    String (&String), Vec (&Vec), или Box (&Box) в качестве аргумента функции?

    Есть исключение: для median, вы клонируете числа и сортируете клон. Если у вызывающего абонента есть одноразовая коллекция, которая не будет использоваться после вычисления медианы, это будет расточительно. Я предлагаю взять Vec по значению здесь:

    fn median(mut numbers: Vec<i32>) -> Option<f64> {
        // ...
        numbers.sort_unstable();
        // ...
    }
    

    Для Copy такие как usize, ссылаться не нужно. Просто передайте usize вместо.

    mean

    Поскольку ваш sum уже проверяет наличие пустых коллекций, вы можете использовать результат вместо повторной проверки mean:

    fn mean(numbers: &[i32]) -> Option<f64> {
        (sum(vector)? as f64) / (numbers.len() as f64)
    }
    

    где ? оператор автоматически разворачивает Option если это Some и возвращается None из включающей функции в противном случае.

    Арифметика

    Это ненужное использование арифметики с плавающей запятой:

    (((length as f64) / 2.0).floor() as usize)
    

    Целочисленное деление округляется до нуля, поэтому вам нужно просто
    length / 2.

    None против NaN

    Поскольку ты уже возвращаешься f64 для mean и median, альтернатива использованию Option может быть просто вернуться f64::NAN
    когда коллекция пуста, как положено f64 арифметика должна быть
    NAN– в любом случае. None и NAN у обоих есть свои плюсы и минусы. Лично мне нравится использовать NAN чтобы упростить тип, но решение остается за вами.

    Поток управления

    В medianслишком много уровней отступов делают код менее читаемым. В matches можно устранить, используя if и return.

    Объединив предыдущие пункты, вот примерно как я бы написал
    median:

    fn median(numbers: Vec<i32>) -> f64 {
        let length = numbers.len();
    
        if length == 0 {
            return f64::NAN;
        }
    
        numbers.sort_unstable();
    
        if is_even(length) {
            let left = numbers[length / 2];
            let right = numbers[length / 2 + 1];
    
            (left as f64 + right as f64) / 2.0
        } else {
            numbers[length / 2] as f64
        }
    }
    

    что, надеюсь, выглядит проще.

    mode

    usize должен быть более естественным выбором для типа значения карты.

    В стандартной библиотеке есть некоторые функции, которые можно использовать для упрощения кода, в том числе max, filter, и
    map:

    fn mode(numbers: &[i32]) -> Option<Vec<i32>> {
        let freqs = HashMap::<i32, usize>::new();
    
        for number in numbers {
            freqs.entry(number).or_insert(0) += 1;
        }
    
        let max_count = freqs.values().max()?;
        let mode = freqs
            .iter()
            .filter(|(_, &count)| count == max_count)
            .map(|(&number, _)| number)
            .collect();
        Some(mode)
    }
    

    get_averages & print_averages

    Пустые коллекции не распространены – я бы не стал тратить на них две ветки. Просто дайте коду пройти:

    fn get_averages(numbers: Vec<i32>) -> Option<Averages> {
        Some(Averages {
            mean: mean(vector)?,
            median: median(vector)?,
            mode: mode(vector)?,
        })
    }
    

    То же самое касается print_averages – вы тратите всего три ветки на непустые коллекции. Лично я считаю, что многострочный строковый литерал нарушает эстетику кода, поэтому я получил:

    fn print_averages(numbers: Vec<i32>) {
        match get_averages(numbers) {
            None => println!("Empty collection"),
            Some(Average { mean, median, mode }) => {
                println!("Mean: {}", mean);
                println!("Median: {}", median);
                println!("Modes: {:?}", mode);
            }
        };
    }
    

    Комментарии и тесты документов

    В качестве бонуса вы можете добавить doc комментарии и тесты чтобы задокументировать свой код, если у вас есть свободное время.

    • Хороший ответ, хорошее использование ? чего я не касался

      – 6005

    • 1

      @ 6005 Похоже, что сейчас мы единственные активные рецензенты на Rust. Продолжаем в том же духе 🙂

      – НЧ

    Добавить комментарий

    Ваш адрес email не будет опубликован. Обязательные поля помечены *