Spaghetti code help

This is probably one of the hard-to-make-nice cases but any ideas how to make this kind of thing nicer?

The idea is to parse a string like translate 35 56 into Command::Translate(35, 56) with preppy proper error handling. This pyramid I came up with is just so fugly I am thinking there must be some nice trick to use in these cases?

    // this is fugly
    fn parse_translate(&self, mut parts: SplitAsciiWhitespace) -> Result<Command, ExecuteError> {
        if let Some(x_str) = parts.next() {
            if let Ok(x) = x_str.parse::<i32>() {
                if let Some(y_str) = parts.next() {
                    if let Ok(y) = y_str.parse::<i32>() {
                        Ok(Command::Translate(x, y))
                    } else {
                        Err(ExecuteError::InvalidParam("Invalid Y value"))
                    }
                } else {
                    Err(ExecuteError::InvalidParam("No Y specified"))
                }
            } else {
                Err(ExecuteError::InvalidParam("Invalid X value"))
            }
        } else {
            Err(ExecuteError::InvalidParam("No X specified"))
        }
    }

You can use the ? operator and methods like Option::ok_or and Result::or to clean this up:

fn parse_translate(&self, mut parts: SplitAsciiWhitespace) -> Result<Command, ExecuteError> {
    let x_str = parts.next().ok_or(ExecuteError::InvalidParam("No X specified"))?;
    let x = x_str.parse::<i32>().or(Err(ExecuteError::InvalidParam("Invalid X value")))?;
    let y_str = parts.next().ok_or(ExecuteError::InvalidParam("Invalid Y value"))?;
    let y = y_str.parse::<i32>().or(Err(ExecuteError::InvalidParam("No Y specified"))?;
    Ok(Command::Translate(x, y))
}
3 Likes

That is certainly more densely packed code.

I'm not sold on the idea that it is more readable.

1 Like

You can also skip the intermediate variables, and just chain the calls to next and parse. Most likely, running rustfmt would reformat it to something like this:

fn parse_translate(&self, mut parts: SplitAsciiWhitespace) -> Result<Command, ExecuteError> {
    let x = parts
        .next()
        .ok_or(ExecuteError::InvalidParam("No X specified"))?
        .parse::<i32>()
        .map_err(|_| ExecuteError::InvalidParam("Invalid X value"))?;
    let y = parts
        .next()
        .ok_or(ExecuteError::InvalidParam("No Y specified"))?
        .parse::<i32>()
        .map_err(|_| ExecuteError::InvalidParam("Invalid Y value"))?;
    Ok(Command::Translate(x, y))
}

I'd argue that the main reason this is more readable is because the code for reporting an error is very near to the code the error is referring to - compared to the original version, where nesting ifs results in the errors being declared in the opposite order that they are relevant.

5 Likes

Thanks, I like this solution. Avoids the pyramid of depth and makes things nice and understandable.