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.
1 Like

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,
        })
    }
}
4 Likes

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

Hi,

pub fn main() {
    let s = "mark,20,hacker";
    let b = s.split_once(",").ok_or("error");
    println!("{:?}", b);
}

result is :
Ok(("mark", "20,hacker"))

So it will not properly produce BadLen. It will produce a ParseInt error.

Yeah I missed the condition. You can check it before the parse anyway. Searching a character would not be heavier than the double parsing.

I came up with something without using split_once(','), but instead with split(','). It's not as readable, but it passed the tests for BadLen errors. :slight_smile:

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 mut fields = s.split(',');

      if let Some(first_field) = fields.next() {
        if first_field.is_empty() {
          return Err(ParsePersonError::NoName);
        }

        if let Some(second_field) = fields.next() {
          // Check for a third field.
          if let Some(_) = fields.next() {
            return Err(ParsePersonError::BadLen);
          };

          let age = second_field.parse::<usize>().map_err(ParsePersonError::ParseInt)?;

          return Ok(Person {
             name: first_field.into(),
             age
          });
        } else {
          return Err(ParsePersonError::BadLen);
        };
      }

      return Err(ParsePersonError::BadLen);
    }
}
1 Like

I did this code, but I don't like how I have to specify ::BadLen 2 times, and the if statements. Also, i know the .unwrap() is not recomended, but is always ok since split returns always something.
Any advice?

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 mut iter = s.split(",");
        let name = iter.next().unwrap();
        let age = iter.next()
            .ok_or(ParsePersonError::BadLen)?.parse::<usize>()
            .map_err(|err| ParsePersonError::ParseInt(err))?;
        if name.is_empty() {
            return Err(ParsePersonError::NoName) 
        }
        if iter.next().is_some(){
            return Err(ParsePersonError::BadLen)
        }
        Ok(Person {name : name.into() , age : age})
    }
}

This compiles and passes all tests on rustlings 4.6.0 and uses elements of the railway programming, though I'm sure it could be improved upon:

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

        let parts: Vec<&str> = s.split(',').collect();

        if parts.len() != 2 {
            return Err(ParsePersonError::BadLen);
        }

        if parts[0].is_empty() {
            return Err(ParsePersonError::NoName);
        }

        let name = parts[0].into();
        let age = parts[1].parse().map_err(ParsePersonError::ParseInt)?;
        
        Ok(Person {
            name,
            age,
        })
    }
}
1 Like

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.