piglatin exercise in rust

Question

This is my attempt to make the exercise suggested at the end of this chapter of rust's official tutorial

pub fn to_pig_latin(phrase: &String) -> String {
    let mut result = String::new();

    for word in phrase.split(" ") {
        let pig_latin_word = word;

        let first_char = match pig_latin_word.chars().nth(0){
            None => panic!("can't get first char in word: {}", phrase),
            Some(c) => c,
        };

        if is_vowel(first_char) {
            result += &(pig_latin_word.to_owned() + "-hay ");
        } else {
            result += &(pig_latin_word[1..].to_owned() + "-" + &first_char.to_string() + "ay ");
        }
    }

    result[0..(result.len()-1)].to_string()
}

fn is_vowel(c :char)-> bool{
    c.to_string() == String::from("a") ||
    c.to_string() == String::from("e") ||
    c.to_string() == String::from("i") ||
    c.to_string() == String::from("o") ||
    c.to_string() == String::from("u")
}

Edge cases such as empty input can be ignored.

I find this very ugly, but to be fair every rust program looks ugly to me.

I don't understand why I need to add to_owned() when I use pig_latin_word , I'm not writing to it seems, actually I did let pig_latin_word = word; exactly because I didn't want ownership problems with word (that is part of phrase).

So far rust's manual memory management feels like just following compiler's help messages until it compiles.

Answer 1

Not a bad start. Some ways it could be improved:

Use &str for String Slices

Currently, you cannot call to_pig_latin("apple"), because the input is a &String. It requires to_pig_latin(&String::from("apple")).

You don’t need to modify or move the input, so the signature should be

pub fn to_pig_latin(phrase: &str) -> String

Now it will understand all stringy types, including a String, a &str or a string literal.

There are Some Bugs

Try this with an empty input string, one with all spaces, or one starting with capital A, E, I, O or U. In fact:

Write Test Cases

Maybe you did, but there are none included here. A basic start:

pub fn main() {
    assert_eq!(to_pig_latin( "a b c d e f g h i j k l m n o p q r s t u v w x y z"),
               "a-hay -bay -cay -day e-hay -fay -gay -hay i-hay -jay -kay -lay -may -nay o-hay -pay -qay -ray -say -tay u-hay -vay -way -xay -yay -zay" );
    assert_eq!(to_pig_latin( "Australian babies complain dingoes eat flavorful girls hungrily in jumping kangaroo lairs munching no other poorer quoditian refreshment so teach utterly verily when xpounding youths zoology"),
               "Australian-hay abies-bay omplain-cay ingoes-day eat-hay lavorful-fay irls-gay ungrily-hay in-hay umping-jay angaroo-kay airs-lay unching-may o-nay other-hay oorer-pay uoditian-qay efreshment-ray o-say each-tay utterly-hay erily-vay hen-way pounding-xay ouths-yay oology-zay");
    assert_eq!(to_pig_latin(""), "");
    assert_eq!(to_pig_latin("   "), "");
}

Don’t Make Expensive Copies You Don’t Need

The biggest problem with the efficiency of this program is that it creates a lot of temporary strings, does round-trip conversions from the native UTF-8 format to 32-bit char and back, copies all but the last character of the result string into a new String object, and so on. Each of these needs to create an object, perform an allocation, and copy some data in linear time.

A good example is the implementation of is_vowel:

fn is_vowel(c :char)-> bool{
    c.to_string() == String::from("a") ||
    c.to_string() == String::from("e") ||
    c.to_string() == String::from("i") ||
    c.to_string() == String::from("o") ||
    c.to_string() == String::from("u")
}

This creates two String objects at runtime and compares them. You could just have compared

(c == 'a') || (c == 'e') || (c == 'i') || (c == 'o') || (c == 'u') ||
(c == 'A') || (c == 'E') || (c == 'I') || (c == 'O') || (c == 'U')

Or written the if is_vowel(first_char) test as a match expression:

        match first_char {
            'a' | 'e' | 'i' | 'o' | 'u' | 'A' | 'E' | 'I' | 'O' | 'U' => {
                unimplemented!(); // You want to write these yourself.
            }
            'b' | 'c' | 'd' | 'f' | 'g' | 'h' | 'j' | 'k' | 'l' | 'm' |
            'n' | 'p' | 'q' | 'r' | 's' | 't' | 'v' | 'w' | 'x' | 'y' |
            'z' => {
                unimplemented!(); // You want to write these yourself.
            }
            'B' | 'C' | 'D' | 'F' | 'G' | 'H' | 'J' | 'K' | 'L' | 'M' |
            'N' | 'P' | 'Q' | 'R' | 'S' | 'T' | 'V' | 'W' | 'X' | 'Y' |
            'Z' => {
                unimplemented!(); // You want to write these yourself.
            }
            _ => {
                unimplemented!(); // You want to write these yourself.
            }
        }

If you really, truly do need a comparison like this, at least compare to a const or static value or a literal, instead of creating a new temporary.

Similarly, instead of copying all but one char of result to a new String, you would be better off truncating result, or using a different approach, such as:

if !result.is_empty() {
    result.push(' ');
}

or:

let separator : &str = if accumulator.is_empty() {""} else {" "};

Another of these, you noticed yourself and asked for feedback on:

result += &(pig_latin_word.to_owned() + "-hay ");

The reason this needed .to_owned() is that you can’t add string slices—only a slice to a String (so .to_string() or String::from would also have worked). There’s no space at the end of a slice to add anything to, and no way to resize it. So you end up creating a temporary String on the right hand side of +=, then converting that to a slice. Not only does this look ugly, it ends up copying all the bytes that you append, twice.

One way around this would be to write:

result += word;
result += "-hay";

(You don’t need pig_latin_word, since you just set it to word and never modify it, but this copy is harmless.) However, there’s a nicer alternative. Even though this more fluent style looks as if it would need to make a temporary copy of result, in fact, rustc is able to optimize away the extra copy when you write:

result = result + word + "-hay";

So this is a zero-cost abstraction. The reason this works is that, in Rust (unlike C++ or Java), adding something to a string consumes the string on the left side of the +. (Therefore, if you do want to add something to a String and also use the original again, you want something like let hello_world = hello.clone() + ", world!";.)

Work on Slices Instead

Whenever possible, you want to be working with slices of the underlying array of bytes, which can be created, resized or passed around in constant time. This is a bit tricky to do here, because a Rust String is, internally, not an array of char like in many other languages. It’s really an array of u8 holding bytes encoded in UTF-8, so you do not have random access to each char, only sequential. Therefore, concatenating string slices is a zero-cost abstraction, but iterating over each char in a string is not: it has to convert between UTF-8 and UCS-4.

The interface you actually want to do this efficiently is not taught in the Rust Book: str::char_indices. So here’s a hint:

    let mut indices = word.char_indices();
    if let Some((0, first_char)) = indices.next() {
        let mid = match indices.next() {
            None => word.len(),
            Some((i, _)) => i,
        };
        let (first_slice, rest_of_word) = word.split_at(mid);
        /* Now first_slice is a slice containing the first letter of the word,
         * and rest_of_word is a (possibly-empty) slice containing the rest of the
         * word.  As before, first_char is the first Unicode character of the
         * word as a 32-bit char.  You decide what to do with them here.
         */
    } else {
        unreachable!("A word was not valid UTF-8");
    }

Consider Refactoring with a Helper Function

Currently, you have a for loop that iterates over the words in the input as string slices. This is a good approach! But there are alternatives that you’ve seen if you’ve done functional programming before, but not in the Rust Book so far.

You can start by factoring out the body of the loop into a helper function, which can be nested inside to_pig_latin:

fn helper(accumulator: String, word: &str) -> String

What this should do is append the Pig-Latin translation of the word slice to the accumulator string, separated by a space, and return the updated accumulator. (You can move accumulator with let mut result = accumulator; to update it in place, or implement the function without that.) Doing it this way avoids creating any String other than accumulator, which can be moved instead of copied. This can simplify your for loop, but the real benefit of this is that there’s a very elegant abstraction for iterating over a sequence and passing each item to a function like this, then returning the final value.

The body of the to_pig_latin function can become simply

phrase.split_whitespace()
      .fold(String::new(), helper)

Plus of course the definition of fn helper. I personally strongly prefer this style over a for loop.