I need help refactoring for error handling in Rust

Question

I would like to refactor this Rust code for calculating the largest series product and make it as efficient and elegant as possible. I feel that

lsp(string_digits: &str, span: usize) -> Result<u64, Error>

could be done in a way to make it much more elegant than it is right now. Could lsp be implemented with only one series of chained iterator methods?

#[derive(Debug, PartialEq)]
pub enum Error {
    SpanTooLong,
    InvalidDigit(char),
}

fn sp(w: &[u8]) -> u64 {
    w.iter().fold(1u64, |acc, &d| acc * u64::from(d))
}

pub fn lsp(string_digits: &str, span: usize) -> Result<u64, Error> {
    let invalid_chars = string_digits
        .chars()
        .filter(|ch| !ch.is_numeric())
        .collect::<Vec<_>>();
    if span > string_digits.len() {
        return Err(Error::SpanTooLong);
    } else if !invalid_chars.is_empty() {
        return Err(Error::InvalidDigit(invalid_chars[0]));
    } else if span == 0 || string_digits.is_empty() {
        return Ok(1);
    }

    let vec_of_u8_digits = string_digits
        .chars()
        .map(|ch| ch.to_digit(10).unwrap() as u8)
        .collect::<Vec<_>>();
    let lsp = vec_of_u8_digits
        .windows(span)
        .max_by(|&w1, &w2| sp(w1).cmp(&sp(w2)))
        .unwrap();
    Ok(sp(lsp))
}

There isn't a concrete question here other than asking us to review your code. This might be a better fit for Code Review SE. — E_net4, Mar 09 '19 at 16:58
@E_net4 Sorry for my seemingly toxic behavior. I had never used that part of SE before, I'll make sure to check it out for future inquiries. — Ahmed Ihsan Tawfeeq, Mar 10 '19 at 18:27
Don't worry, there was nothing toxic in your behavior. :) I was simply concerned about the question's fitness on this site. — E_net4, Mar 10 '19 at 19:10
Does this answer your question? [How do I perform iterator computations over iterators of Results without collecting to a temporary vector?](https://stackoverflow.com/questions/48841367/how-do-i-perform-iterator-computations-over-iterators-of-results-without-collect) — Stargateur, Jan 22 '20 at 07:59

darkwisebear · Accepted Answer · 2019-03-11T13:21:18.737

1

Not sure if this is the most elegant way, but I've given it a try, hope the new version is equivalent to the given program.

Two things will be needed in this case: First, we need a data structure that provides the sliding window "on the fly" and second a function that ends the iteration early if the conversion yields an error.

For the former I've chosen a VecDeque since span is dynamic. For the latter there is a function called process_results in the itertools crate. It converts an iterator over results to an iterator over the unwrapped type and stops iteration if an error is encountered.

I've also slightly changed the signature of sp to accept any iterator over u8.

This is the code:

use std::collections::VecDeque;
use itertools::process_results;

#[derive(Debug, PartialEq)]
pub enum Error {
    SpanTooLong,
    InvalidDigit(char),
}

fn sp(w: impl Iterator<Item=u8>) -> u64 {
    w.fold(1u64, |acc, d| acc * u64::from(d))
}

pub fn lsp(string_digits: &str, span: usize) -> Result<u64, Error> {
    if span > string_digits.len() {
        return Err(Error::SpanTooLong);
    } else if span == 0 || string_digits.is_empty() {
        return Ok(1);
    }

    let mut init_state = VecDeque::new();
    init_state.resize(span, 0);

    process_results(string_digits.chars()
        .map(|ch| ch.to_digit(10)
            .map(|d| d as u8)
            .ok_or(Error::InvalidDigit(ch))),
        |digits|
            digits.scan(init_state, |state, digit| {
                state.pop_back();
                state.push_front(digit);
                Some(sp(state.iter().cloned()))
            })
            .max()
            .unwrap()
    )
}

edited Mar 11 '19 at 13:21

answered Mar 09 '19 at 17:09

darkwisebear

131
5

This works for most cases, except for the case when all series products would produce 0. For instance, for lsp("99099") it would produce Ok(81) instead of Ok(0), that's because the state transition would be: [9] -> [9, 9] -> [0, 9, 9] -> [9, 0, 9] -> [9, 9, 0]. – Ahmed Ihsan Tawfeeq Mar 10 '19 at 19:10
The solution would be to pad the first n states (the no. of states before the state.len() == span) with zeros, thus their sp would always be 0 and will not be counted into lsp. – Ahmed Ihsan Tawfeeq Mar 10 '19 at 19:13
As follows: `if state.len() == span { state.pop_back(); } else { for _i in 1..span { state.push_front(0u8); } }` – Ahmed Ihsan Tawfeeq Mar 10 '19 at 19:14
Also, naming the parsed u8 digits as chars is very misleading :) – Ahmed Ihsan Tawfeeq Mar 10 '19 at 19:19
Other than that, great job +1 – Ahmed Ihsan Tawfeeq Mar 10 '19 at 19:19
*correction: I meant lsp("99099", 3) for the example I gave. – Ahmed Ihsan Tawfeeq Mar 10 '19 at 19:37
1

Yep correct, I'll change the answer to reflect both the corner case and the name. – darkwisebear Mar 11 '19 at 13:19

I need help refactoring for error handling in Rust

1 Answers1