TransWikia.com

How can I make my error handling more idiomatic in Rustlings exercise "from_into"?

Code Review Asked by Kyle_S-C on March 1, 2021

I’ve got a working solution to the Rustlings from_into exercise (i.e. the code compiles and all the tests pass). The task required that I implemented the From trait for struct Person. I’ve highlighted the relevant section inline with comments below.

// The From trait is used for value-to-value conversions.
// If From is implemented correctly for a type, the Into trait should work conversely.
// You can read more about it at https://doc.rust-lang.org/std/convert/trait.From.html
#[derive(Debug)]
struct Person {
    name: String,
    age: usize,
}

// We implement the Default trait to use it as a fallback
// when the provided string is not convertible into a Person object
impl Default for Person {
    fn default() -> Person {
        Person {
            name: String::from("John"),
            age: 30,
        }
    }
}

// THE BELOW IMPLEMENTATION IS WHAT I'D LIKE INPUT ON
//
// Steps:
// 1. If the length of the provided string is 0, then return the default of Person
// 2. Split the given string on the commas present in it
// 3. Extract the first element from the split operation and use it as the name
// 4. If the name is empty, then return the default of Person
// 5. Extract the other element from the split operation and parse it into a `usize` as the age
// If while parsing the age, something goes wrong, then return the default of Person
// Otherwise, then return an instantiated Person object with the results
impl From<&str> for Person {
    fn from(s: &str) -> Person {
        let s_as_vec = s.split(",").collect::<Vec<&str>>();
        let name = s_as_vec[0].to_string();

        if name.is_empty() {
            Person::default()
        } else if s_as_vec.len() > 1 {
            match s.splitn(2, ",").collect::<Vec<&str>>()[1].parse::<usize>() {
                Ok(age) => Person {name, age},
                Err(_) => Person::default()
            }
        } else {
            Person::default()
        }
    }
}

// END OF WHAT I'D LIKE INPUT ON

fn main() {
    // Use the `from` function
    let p1 = Person::from("Mark,20");
    // Since From is implemented for Person, we should be able to use Into
    let p2: Person = "Gerald,70".into();
    println!("{:?}", p1);
    println!("{:?}", p2);
}

#[cfg(test)]
mod tests {
    use super::*;
    #[test]
    fn test_default() {
        // Test that the default person is 30 year old John
        let dp = Person::default();
        assert_eq!(dp.name, "John");
        assert_eq!(dp.age, 30);
    }
    #[test]
    fn test_bad_convert() {
        // Test that John is returned when bad string is provided
        let p = Person::from("");
        assert_eq!(p.name, "John");
        assert_eq!(p.age, 30);
    }
    #[test]
    fn test_good_convert() {
        // Test that "Mark,20" works
        let p = Person::from("Mark,20");
        assert_eq!(p.name, "Mark");
        assert_eq!(p.age, 20);
    }
    #[test]
    fn test_bad_age() {
        // Test that "Mark.twenty" will return the default person due to an error in parsing age
        let p = Person::from("Mark,twenty");
        assert_eq!(p.name, "John");
        assert_eq!(p.age, 30);
    }

    #[test]
    fn test_missing_comma_and_age() {
        let p: Person = Person::from("Mark");
        assert_eq!(p.name, "John");
        assert_eq!(p.age, 30);
    }

    #[test]
    fn test_missing_age() {
        let p: Person = Person::from("Mark,");
        assert_eq!(p.name, "John");
        assert_eq!(p.age, 30);
    }

    #[test]
    fn test_missing_name() {
        let p: Person = Person::from(",1");
        assert_eq!(p.name, "John");
        assert_eq!(p.age, 30);
    }

    #[test]
    fn test_missing_name_and_age() {
        let p: Person = Person::from(",");
        assert_eq!(p.name, "John");
        assert_eq!(p.age, 30);
    }

    #[test]
    fn test_missing_name_and_invalid_age() {
        let p: Person = Person::from(",one");
        assert_eq!(p.name, "John");
        assert_eq!(p.age, 30);
    }
}

My primary question is: how do I make this more idiomatic? The pair of split[n]() calls seems wrong, but I’m not sure how to handle the various ways it would panic if I don’t have explicit length checks.

One Answer

Congratulations on your Rustlings challenge. Your code seems fine and is easy to understand, however, as you said yourself, some parts of it don't seem idiomatic yet.

However, before I begin this review, I would like to mention that I consider myself a Rust beginner with a lot more experience in other languages. Keep that in mind while you read this review (and feel free to disagree and comment!).

Use clippy

While clippy cannot work miracles, it can warn you about some lints. For example, the patterns in split are single character literals and thus should be written as ',', not ",".

Reuse what you already have

The longest line in your code is the match on parse's result:

// indentation removed for simplicity
match s.splitn(2, ",").collect::<Vec<&str>>()[1].parse::<usize>()

However, we already have s.split(...)'s result at hand. We don't need to split it a second time:

match s_as_vec[1].parse::<usize>()

If your concerned about the n in splitn then use splitn in your binding for s_as_vec. Note that the turbofish ::<usize> isn't necessary, as rustc can infer its type by the use in Person.

Use if-let when applicable

Back to the explicit length check. Instead of

if s_as_vec.len() > 1

we can use get(1) and check whether the element exist via if let:

if let Some(age) = s_as_vec.get(1) {
    match age.parse() {
        Ok(age) => Person { name, age },
        Err(_)  => Person::default
    }
}

This also prevents us from accidentally changing one of the index locations, e.g.

if s_as_vec.len() > 2 {
    match s_as_vec[3].parse() { // whoops, wrong number here
 ...

Defer possible heavy operations if possible

We don't need name to be a String unless we have a valid age. Therefore, we can have let name = s_as_vec[0] until we create the Person. This removes unnecessary allocations.

All remarks above applied

If we apply all remarks above, we end up with the following variant.

impl From<&str> for Person {
    fn from(s: &str) -> Person {
        let s_as_vec = s.splitn(2, ',').collect::<Vec<&str>>();
        let name = s_as_vec[0];

        if name.is_empty() {
            Person::default()
        } else if let Some(age) = s_as_vec.get(1) {
            match age.parse() {
                Ok(age) => Person {name, age},
                Err(_) => Person::default()
            }
        } else {
            Person::default()
        }
    }
}

However, I personally would probably try to minimize the possible return paths and use a single match expression, but that's based on my Haskell experience:

impl From<&str> for Person {
    fn from(s: &str) -> Person {
        let parts = s.splitn(2, ',').collect::<Vec<&str>>();

        match &parts[..] {
            [name, age] if !name.is_empty() => age
                .parse()
                .map(|age| Person {
                    name: name.to_string(),
                    age,
                })
                .unwrap_or_default(),
            _ => Person::default(),
        }
    }
}

Correct answer by Zeta on March 1, 2021

Add your own answers!

Ask a Question

Get help from others!

© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP