3
\$\begingroup\$

This question is with respect to Day 1, Part 2 of the 2021 Advent Of Code challenge, which requires comparing values from a rolling window taken from integers streamed as input. This answer is correct, insofar as it emits the intended output, but I'm unclear as to the quality of the code.

(This challenge is no longer scored for points, making it within the rules to post/submit/stream/discuss/otherwise share answers).

My overall goal here is to learn to write reasonably good Rust code; all advice towards that end will be gratefully accepted.

use std::collections::VecDeque;
use std::error;
use std::error::Error;
use std::io;
use std::io::BufRead;
use std::ops::{Add, AddAssign, Sub};
use std::result::Result;

pub struct WindowedSumIter<'a, T, E>
where
    T: Default + Add<Output = T> + Sub<Output = T> + AddAssign + Copy,
{
    source: &'a mut dyn Iterator<Item = Result<T, E>>,
    window: VecDeque<T>,
    sum: T,
    finished: bool,
}

impl<'a, T, E> WindowedSumIter<'a, T, E>
where
    T: Default + Add<Output = T> + Sub<Output = T> + AddAssign + Copy,
{
    pub fn new(
        source: &'a mut dyn Iterator<Item = Result<T, E>>,
        window_size: u32,
    ) -> Result<Self, E> {
        let mut sum: T = T::default();
        let mut window: VecDeque<T> = VecDeque::new();
        let mut finished: bool = false;
        // fill the window at initialization time
        for _ in 0..window_size {
            let item = source.next();
            match item {
                None => {
                    finished = true;
                }
                Some(maybe_value) => match maybe_value {
                    Ok(value) => {
                        sum = sum + value;
                        window.push_back(value);
                    }
                    Err(e) => {
                        return Result::Err(e);
                    }
                },
            }
        }
        Ok(WindowedSumIter {
            source,
            window,
            sum,
            finished,
        })
    }
}

impl<'a, T, E> Iterator for WindowedSumIter<'a, T, E>
where
    T: Default + Add<Output = T> + Sub<Output = T> + AddAssign + Copy,
{
    type Item = Result<T, E>;
    fn next(&mut self) -> Option<Self::Item> {
        if self.finished {
            None
        } else {
            let old_value = self.window.pop_front().unwrap();
            let maybe_new_value = self.source.next();
            match maybe_new_value {
                None => {
                    self.finished = true;
                    Some(Result::Ok(self.sum))
                }
                Some(new_value_or_error) => match new_value_or_error {
                    Ok(new_value) => {
                        let orig_sum = self.sum;
                        self.sum += new_value - old_value;
                        self.window.push_back(new_value);
                        Some(Result::Ok(orig_sum))
                    }
                    Err(e) => Some(Result::Err(e)),
                },
            }
        }
    }
}

fn main() -> Result<(), Box<dyn Error>> {
    let stdin = io::stdin();
    let mut depth_increase_count: i32 = 0;
    {
        let mut integers = stdin.lock().lines().map(|result| {
            result
                .map_err(|e| Box::<dyn error::Error>::from(e))
                .and_then(|s| {
                    s.parse::<i32>()
                        .map_err(|e| Box::<dyn error::Error>::from(e))
                })
        });
        let mut rolling_totals = WindowedSumIter::new(&mut integers, 3)?;
        let mut last_depth = rolling_totals.next().unwrap().unwrap();

        for maybe_integer in rolling_totals {
            let current_depth = maybe_integer.unwrap();
            if current_depth > last_depth {
                depth_increase_count += 1;
            }
            last_depth = current_depth;
        }
    }
    println!("{}", depth_increase_count);
    return Result::Ok(());
}

While all this code should be considered in-scope, I'm particularly curious as to whether there's a better way to generate an iterator over integers parsed from stdin (the statement starting with let mut integers =) while keeping the error handling fairly flat (avoiding the need to have std::io::Errors and std::num::ParseIntErrors handled separately by consumers of that iterator, with distinct levels of unwrapping).


For the record: There is another Advent of Code Day 1 solution at Advent of Code 2021: day 1 however, the linked question does not try to create an iterator to generate rolling windows, so I believe the new question has enough novelty to justify its creation.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Errors

Your code is good, I don't see any room for improvement in error handling.

Idiomatic Rust code may feel verbose, but it's good.

I would use if let in both places to reduce verbosity.

Less complexity in WindowedSumIter

Let's reduce the complexity a little by collecting to an intermediate Result<Vec<i32>, Box<dyn Error>> in main.

That's because you have unseparated concerns -- that is, WindowedSumIter handles both sums and errors. Unseparated concerns make it harder to modify and reuse code in other tasks if one stumbles upon your solution.

Ideally, we would use something like itertools::process_results here. However, we can do the same thing with a few lines in main, possibly with a tiny difference in performance.

FromIterator used by collect has a nifty impl that allows you to collect an iterator of Results into a single Result that wraps a container.

use std::collections::VecDeque;
use std::error::Error;
use std::io;
use std::io::BufRead;
use std::ops::{Add, AddAssign, Sub};
use std::result::Result;

pub struct WindowedSumIter<'a, T>
where
    T: Default + Add<Output = T> + Sub<Output = T> + AddAssign + Copy,
{
    source: &'a mut dyn Iterator<Item = T>,
    window: VecDeque<T>,
    sum: T,
    finished: bool,
}

impl<'a, T> WindowedSumIter<'a, T>
where
    T: Default + Add<Output = T> + Sub<Output = T> + AddAssign + Copy,
{
    pub fn new(
        source: &'a mut dyn Iterator<Item = T>,
        window_size: u32,
    ) -> Self {
        let mut sum: T = T::default();
        let mut window: VecDeque<T> = VecDeque::new();
        let mut finished: bool = false;
        // fill the window at initialization time
        for _ in 0..window_size {
            let item = source.next();
            match item {
                None => {
                    finished = true;
                }
                Some(value) => {
                    sum = sum + value;
                    window.push_back(value);
                },
            }
        }
        WindowedSumIter {
            source,
            window,
            sum,
            finished,
        }
    }
}

impl<'a, T> Iterator for WindowedSumIter<'a, T>
where
    T: Default + Add<Output = T> + Sub<Output = T> + AddAssign + Copy,
{
    type Item = T;
    fn next(&mut self) -> Option<Self::Item> {
        if self.finished {
            None
        } else {
            let old_value = self.window.pop_front().unwrap();
            let maybe_new_value = self.source.next();
            match maybe_new_value {
                None => {
                    self.finished = true;
                    Some(self.sum)
                }
                Some(new_value) => {
                    let orig_sum = self.sum;
                    self.sum += new_value - old_value;
                    self.window.push_back(new_value);
                    Some(orig_sum)
                },
            }
        }
    }
}

fn main() -> Result<(), Box<dyn Error>> {
    let stdin = io::stdin();
    let mut depth_increase_count: i32 = 0;
    {
        let integers: Vec<i32> = stdin.lock().lines().map(|result| {
            result
                .map_err(|e| Box::<dyn Error>::from(e))
                .and_then(|s| {
                    s.parse()
                        .map_err(|e| Box::<dyn Error>::from(e))
                })
        }).collect::<Result<_, _>>()?;
        let mut iter = integers.iter().cloned();
        let mut rolling_totals = WindowedSumIter::new(&mut iter, 3);
        let mut last_depth = rolling_totals.next().unwrap();

        for maybe_integer in rolling_totals {
            let current_depth = maybe_integer;
            if current_depth > last_depth {
                depth_increase_count += 1;
            }
            last_depth = current_depth;
        }
    }
    println!("{}", depth_increase_count);
    return Result::Ok(());
}

My solution using itertools::process_results and TupleWindows

I attempted a solution out of curiosity.

use std::error::Error;
use std::io;
use std::io::BufRead;
use std::mem;
use std::result::Result;

use itertools::{process_results, Itertools};

fn main() -> Result<(), Box<dyn Error>> {
    let stdin = io::stdin();
    let depth_increase_count: usize;
    {
        let integers = stdin.lock().lines().map(|result| {
            result
                .map_err(|e| Box::<dyn Error>::from(e))
                .and_then(|s| {
                    s.parse::<i32>()
                        .map_err(|e| Box::<dyn Error>::from(e))
                })
        });
        depth_increase_count = process_results(integers, |iter| {
            let mut sums = iter.tuple_windows().map(|(a, b, c)| a + b + c);
            let mut prev_sum = sums.next().expect("empty input");
            sums.filter(|&sum| {
                sum > mem::replace(&mut prev_sum, sum)
            }).count()
        })?;
    }
    println!("{}", depth_increase_count);
    return Result::Ok(());
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ This answer is valuable as the use if itertools crate and tuple windows avoid to chain zip and have to work with ((a, b), c) pairs. \$\endgroup\$
    – Dereckson
    Commented Dec 13, 2021 at 8:26
  • 1
    \$\begingroup\$ I had to take a few minutes digging around in the standard-library documentation to discover how this works (eventually landing on doc.rust-lang.org/src/core/result.rs.html#1838-1888). This has been an extremely valuable learning experience; thank you! \$\endgroup\$ Commented Dec 19, 2021 at 21:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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