Rustlings - from_str.rs, matching conditions

Hello, again with code review questions from rustlings! Please refer to this file for the original code. This is my solution:

impl FromStr for Person {
    type Err = ParsePersonError;
    fn from_str(s: &str) -> Result<Person, Self::Err> {
        if s == "" {
            Err(ParsePersonError::Empty)
        } else {
            match s.split_once(",") {
                Some((name, age)) if matches!(age.parse::<usize>(), Ok(_))  =>
                    if name == "" {
                        Err(ParsePersonError::NoName)
                    } else {
                        Ok(Person { name: String::from(name), age: age.parse().unwrap() })
                    },
                Some((_, age)) if matches!(age.parse::<usize>(), Err(_)) && !matches!(age.split_once(","), Some((_,_))) => {
                    let err = age.parse::<usize>().unwrap_err();
                    Err(ParsePersonError::ParseInt(err))
                },
                _ =>
                    Err(ParsePersonError::BadLen)
            }
        }
    }
}

I am having a lot of fun with pattern matching, but I'm not sure it's the best solution in this case. I'd still like to use match though. The arms are built around split_once. What would be a good solution in which each arm is a different error, and unambiguously that one? How would this exercise be solved with idiomatic rust?

You have got a correct solution and a good enough code. What I think you're trying to achieve is the following:

  • First check if I have a non-empty input, err otherwise
  • Then split it at the first comma, err if no comma
  • Then parse the name and age from the two comma separated things.
  • Name is trivially parseable (its just a string)
  • You need to parse the age: while parsing you check if the age string has a comma in it - if it does it's a different error. Otherwise, parse it - err if it fails, succeed otherwise.

So roughly, there are two phases:

  1. Split the string correctly
  2. Parse the age

These can be two different functions (in general a function should do one and one thing only).
So here is what I think reads better (Rust Playground)

impl FromStr for Person {
    type Err = ParsePersonError;

    fn from_str(s: &str) -> Result<Person, Self::Err> {
        if s.is_empty() {
            Err(ParsePersonError::Empty)
        } else if let Some((name, age)) = s.split_once(",") {
            if name.is_empty() {
                Err(ParsePersonError::NoName)
            } else {
                Ok(Person {
                    name: String::from(name),
                    age: parse_age(age)?,
                })
            }
        } else {
            Err(ParsePersonError::BadLen)
        }
    }
}

fn parse_age(age_str: &str) -> Result<usize, ParsePersonError> {
    if age.contains(',') {
        Err(ParsePersonError::BadLen)
    } else {
        match age.parse() {
            Ok(age) => Ok(age),
            Err(err) => Err(ParsePersonError::ParseInt(err)),
        }
    }
}

Some comments:

  • You don't always need to use match - sometimes if let works better.
  • Please don't do s == "". It is much better to use s.is_empty(). If you use clippy, then it'll tell you the same.
  • You don't need to use split_once to check whether a str contains a pattern. There is a contains function for it. It is a good idea to just look through the standard docs at regular intervals to learn about the different functions you have available.

Take a look at the railway oriented programming. It helps you to linearize the happy path.

impl FromStr for Person {
    type Err = ParsePersonError;

    fn from_str(s: &str) -> Result<Person, Self::Err> {
        if s.is_empty() {
            return Err(ParsePersonError::Empty);
        }

        let (name, age) = s.split_once(',')
            .ok_or(ParsePersonError::BadLen)?;
        let age = age.parse()
            .map_err(ParsePersonError::ParseInt)?;

        if name.is_empty() {
            return Err(ParsePersonError::NoName);
        }

        Ok(Person {
            name: name.into(),
            age,
        })
    }
}
1 Like

Now that's a cool new term I learnt today!