Yet Another Pig-Latin Review

Hello, as lot of you could have guessed by the title I'm trying to learn rust and this is one of the most common first exercise for newbies, how would you rate this? and mostly how could I improve in this little code snippet?

// Convert strings to pig latin. The first consonant of each word is moved to the end of the word and “ay” is added, so “first” becomes “irst-fay.” Words that start with a vowel have “hay” added to the end instead (“apple” becomes “apple-hay”). Keep in mind the details about UTF-8 encoding!

fn is_vowel(c: char) -> bool {
    match c.to_ascii_lowercase() {
        'a' | 'e' | 'i' | 'o' | 'u' => true,
        _ => false,
    }
}
fn main() {
    let mut word = String::from("Здравствуйте");

    let mut chars = word.chars();
    match chars.next() {
        Some(c) => match is_vowel(c) {
            true => word.push_str("-hay"),
            _ => {
                let mut result = String::new();
                chars.for_each(|character| result.push(character));
                result.push_str("-");
                result.push(c);
                result.push_str("ay");
                word = result.clone();
            }
        },
        None => println!("The string is empty"),
    }

    println!("The word converted into pig-latin is \"{}\"", word)
}

There is a macro you can use for matches - in is_vowel
matches!(foo, ['a', 'e', 'i', 'o', 'u'])

  • Run clippy and apply its suggestions
  • Matching on a boolean (match is_vowel(c)) can just be an if-else
  • result.clone(); → the clone is unnecessary
2 Likes
let result = chars.collect::<String>();
word = format!("{result}-{c}ay");

is more concise than that large block in the middle.

The pattern

chars.for_each(|character| result.push(character));

is covered by the .extend method

result.extend(chars);

The overall pig-latin functionality should be in its own function, and not coupled to the fact that the input is already present in a mutable owned variable word: String. The idiomatic signature of such a function would be fn(&str) -> String, or perhaps fn(&str) -> Option<String> if you want rejection of empty strings (or other input validation) to be part of it.

2 Likes

Thank you very much.

Here the result after some refactoring

fn is_vowel(c: char) -> bool {
    matches!(c.to_ascii_lowercase(), 'a' | 'e' | 'i' | 'o' | 'u')
}

fn pig_latin(word: &str) -> Option<String> {
    let mut chars = word.chars();
    match chars.next() {
        Some(c) => {
            if is_vowel(c) {
                let mut result = word.to_string();
                result.push_str("-hay");
                Some(result)
            } else {
                let mut result = String::new();
                result.extend(chars);
                Some(format!("{result}-{c}ay"))
            }
        }
        None => None,
    }
}

fn main() {
    let word = String::from("Здравствуйте");

    match pig_latin(&word) {
        Some(translated_word) => println!(
            "The word converted into pig-latin is \"{}\"",
            translated_word
        ),
        None => println!("The string is empty"),
    }
}

Still feel free to give any advice!

You could make good use of .as_str() of the Chars iterator here, I believe. Then you wouldn’t need the intermediate result string at all in the second case of the if. The first case could use format!, too, eliminating the intermediate result string there, too.

Also, if you encounter None => None, cases, you could check if you could use ? operator instead.

Here’s a possible implementation to compare to that’s using these suggestions:

Click here to expand the implementation

Make sure to try it on your own first, though :wink:

fn pig_latin(word: &str) -> Option<String> {
    let mut chars = word.chars();
    let first = chars.next()?;
    Some(if is_vowel(first) {
        format!("{word}-hay")
    } else {
        format!("{rest}-{first}ay", rest = chars.as_str())
    })
}

I'll offer a disagreeing argument here: you might want to be able to reuse an allocation when converting between strings, which may make a difference for performance. It wouldn't be wrong (in my opinion) to have a function along the lines of fn to_pig_latin(s: String) -> String or fn convert_pig_latin(s: &mut String).

All these approaches have their own merits - it's worth thinking about which might be the best fit for the use case.

1 Like

I assume you disagree not exactly with the quoted part but with the suggested “idiomatic signature” I gave.

I’d say, I’ll stick to my suggestion as the idiomatic default, but I’m not trying to imply that other signatures can’t also be proper & useful. I’d say that generally, it’s a good thing to learn that inputs like the String here, should be borrowed, if the function generally transforms the input into a completely new piece of data.

For pig-latin and Strings, there could be the consideration that on one of the two cases you only append to the end, and thus (at least that case) can potentially benefit from allocation re-use. On the other hand, I’d also say that capturing this aspect might realistically be premature optimization.

One important aspect of my suggested signature is also that by working with a fn(&str) -> String, you can learn that &str is a common argument type to consider, whereas suggesting fn(String) -> String is teaching less new things.

Speaking of alternative approaches with their own merits, one could even consider further alternatives like a never-allocating fn(&str) -> impl Display (though that would be more convenient to implement if something like this existed in std).

1 Like

This can be shortened:

    match chars.next() {
        Some(c) => {
            if is_vowel(c) {
                let mut result = word.to_string();
                result.push_str("-hay");
                Some(result)
            } else {
                let mut result = String::new();
                result.extend(chars);
                Some(format!("{result}-{c}ay"))
            }
        }
        None => None,
    }

if let Some(c) = chars.next() { ... }.

And this match returns some value, but you don't assign it anywhere, it just gets dropped.

1 Like

That's not correct. The match expression is the return value of the whole function.

It can be shortened of course, but not with if let unless you add a else { None}, but I'd rather suggest using map or using ?-operator as more reasonable options for shortening this.

1 Like

I refered to the original post:

fn main() {
    let mut word = String::from("Здравствуйте");

    let mut chars = word.chars();
    match chars.next() {
        ...
    }
}