Review of pig latin converter (chapter 8)

code review
1

Ok, the good news is that it seems to work according to the specs in the book...

Thing is, it looks ugly :frowning:

use std::io;

fn main() {
    loop {
        println!("Please enter sentence to pigify:");
        let mut input = String::new();
        io::stdin()
            .read_line(&mut input)
            .expect("Failed to read line");

        let input = input.trim();
        if input.is_empty() {
            break;
        };

        for word in input.split_ascii_whitespace() {
            let mut ending = String::new();
            let mut pigword = String::new();
            for char in word.chars() {
                if ending.is_empty() {
                    match char.to_ascii_uppercase() {
                        'A' | 'E' | 'I' | 'O' | 'U' => {
                            ending = "h".to_string().to_owned();
                            pigword.push_str(char.to_string().as_str())
                        }
                        _ => ending = char.to_string().to_owned(),
                    }
                    ending.push_str("ay");
                } else {
                    pigword.push_str(char.to_string().as_str());
                }
            }
            pigword.push_str(&ending);
            print!("{} ", pigword);
        }
        println!();
    }
}

What bothers me most is the repeated type conversions... I also wanted to use Option to handle the first pass ("ending" being empty) but it got totally out of hand quickly so I went back to more familiar logic. Would an Option make any sense here or is the current IF good enough?

2

In the inner loop you can use String::push(char) to simplify things a little and avoid converting to String unnecessarily.

for char in word.chars() {
    if ending.is_empty() {
        let first_ending_char = match char.to_ascii_uppercase() {
            'A' | 'E' | 'I' | 'O' | 'U' => {
                pigword.push(char);
                'h'
            }
            _ => char,
        };
        ending.push(first_ending_char);
        ending.push_str("ay");
    } else {
        pigword.push(char);
    }
}

I think using is_empty is clearer than using an Option in this case.

2 Likes
3

Some amount of type conversion is to be expected because Rust likes to be explicit, in order to avoid surprises or hidden behaviour. However, you can simplify a little:

  • you can push a char: pigword.push_str(char.to_string().as_str()) becomes pigword.push(char)
  • you don't need .to_owned() after .to_string(), because the former transforms an object to an owned one, usually by cloning, and a String is already owned: it's not a reference.

That gives something like this, where I moved the code that returns the piglatin from a word into a function and added a unit test on it. That allows you to modify your algorithm and verify it's still fine.

(you can run the code by changing the mode at the top right, clicking on the "...")

You can also simplify the logic a little, since ending is determined by the first character, but that's more a matter of preference or style. Here, the .unwrap() to extract the first character should be fine since the string is valid and non-empty, so it has at least one valid character.

As for the Option vs is_empty, I would keep it as you did.

5 Likes
4

Thank you both, @jumpnbrownweasel and @Redglyph : I was indeed hoping to make it "rustier" and your answers go in this direction.

Although familiar with unit tests in other languages, they come up much later in the book, not really on my radar now.

1 Like