Code Review for Chapter 8 Pig Latin

[cyberphine]

Any comments would be appreciative. Thanks for looking!

fn main() {
    assert_eq!(pig_latin("apple".to_string()), "apple-hay".to_string());
    assert_eq!(pig_latin("banana".to_string()), "anana-bay".to_string());
    assert_eq!(pig_latin("Здравствуйте".to_string()), "дравствуйте-Зay".to_string());
}

// Pig Latin
fn pig_latin(phrase: String)-> String {
    struct FirstLetter {
        first_letter: char,
    }

    impl FirstLetter {
        // Get a vowel function.
        fn is_vowel(&self)-> bool {
            match self.first_letter {
                'A' | 'a' | 'E' | 'e' | 'I' | 'i' | 'O' | 'o' | 'U' | 'u' => {
                    true
                },
                _ => false,
            }
        }

        fn new(first_letter: char)-> FirstLetter {
            FirstLetter {
                first_letter
            }
        }
    }

    let mut phrase_chars = phrase.chars();

    let first_letter = match phrase_chars.next() {
        Some(char) => char,
        None => panic!("Empty String?")
    };

    let first_letter = FirstLetter::new(first_letter);

    let mut pig_latin = String::new();

    // apple  : a
    // banana :
    if first_letter.is_vowel() {
        pig_latin.push(first_letter.first_letter);
    }

    // apple  : apple
    // banana : anana
    while let Some(char) = phrase_chars.next() {
        pig_latin.push(char);
    };

    // apple  : apple-
    // banana : anana-
    pig_latin.push('-');

    // apple  : apple-b
    // banana : anana-h
    if !first_letter.is_vowel() {
        pig_latin.push(first_letter.first_letter);
    } else {
        pig_latin.push('h');
    }

    // apple  : apple-bay
    // banana : anana-hy
    pig_latin.push_str(&"ay");

    pig_latin
}

[wb4]

I'm putting my comments inline since this forum doesn't show line numbers when quoting code (or at least I don't how to make it do so).

fn main() {
    // Don't need to call .to_string() on the 2nd arg.
    // A String can be compared to a &str.
    assert_eq!(pig_latin("apple".to_string()), "apple-hay".to_string());
    assert_eq!(pig_latin("banana".to_string()), "anana-bay".to_string());
    assert_eq!(pig_latin("Здравствуйте".to_string()), "дравствуйте-Зay".to_string());
}

// Pig Latin
// It's more idiomatic to pass string data as a &str instead of a String.
// I.e.: phrase: &str
fn pig_latin(phrase: String)-> String {
    // Defining this struct is an awful lot of code just to be able to
    // call .is_vowel().  Why not just write is_vowel() as a stand-alone
    // function that takes a char argument?
    struct FirstLetter {
        first_letter: char,
    }

    impl FirstLetter {
        // Get a vowel function.
        fn is_vowel(&self)-> bool {
            // This can be more succinctly written as:
            // "aeiou".contains(self.first_letter.to_ascii_lowercase())
            match self.first_letter {
                'A' | 'a' | 'E' | 'e' | 'I' | 'i' | 'O' | 'o' | 'U' | 'u' => {
                    true
                },
                _ => false,
            }
        }

        fn new(first_letter: char)-> FirstLetter {
            FirstLetter {
                first_letter
            }
        }
    }

    let mut phrase_chars = phrase.chars();

    // This can be more succinctly written as:
    // let first_letter = phrase_chars.next().expect("Empty String?");
    let first_letter = match phrase_chars.next() {
        Some(char) => char,
        None => panic!("Empty String?")
    };

    let first_letter = FirstLetter::new(first_letter);

    let mut pig_latin = String::new();

    // apple  : a
    // banana :
    if first_letter.is_vowel() {
        pig_latin.push(first_letter.first_letter);
    }

    // apple  : apple
    // banana : anana
    // This can be more succinctly written as:
    // pig_latin.extend(phrase_chars);
    while let Some(char) = phrase_chars.next() {
        pig_latin.push(char);
    };

    // apple  : apple-
    // banana : anana-
    pig_latin.push('-');

    // apple  : apple-b
    // banana : anana-h
    // The code pig_latin.push(...) is duplicated here.  We can remove that
    // duplication by putting the condition inside the call to push(), like so:
    //    pig_latin.push(if first_letter.is_vowel() {
    //        'h'
    //    } else {
    //        first_letter.first_letter
    //    });
    if !first_letter.is_vowel() {
        pig_latin.push(first_letter.first_letter);
    } else {
        pig_latin.push('h');
    }

    // apple  : apple-bay
    // banana : anana-hy
    // Don't need the & here.
    pig_latin.push_str(&"ay");

    pig_latin
}

Edited to put tip to remove the & from &"ay".
Edited to fix typo.

[cyberphine]

Thank you for the succinct comments! The struct code was indeed overkill and me toying around a bit.

[drmason13]

The struct was an interesting way to remember what the first character was for later.

I'd consider making the first_letter_is_vowel(c: char) return Some(char) if it is a consonant and None otherwise, (and probably rename it too!)

Then you get the true, false information and the first letter to remember for later.

let ay_prefix = first_letter_is_vowel(phrase_chars);
...
if let Some(c) = ay_prefix {
    pig_latin.push(c);
} else {
...

Or, bearing in mind wb4's advice...

let ay_prefix = first_letter_is_vowel(phrase_chars);
...
pig_latin.push(ay_prefix.unwrap_or('h'));

[cyberphine]

Thank you! I am just now getting into the .unwrap/.unwrap_or chains in the book. I see why that's more beneficial in this case.

[scottmcm]

@drmason13

I'd consider making the first_letter_is_vowel(c: char) return Some(char) if it is a consonant and None otherwise, (and probably rename it too!)

Then you get the true, false information and the first letter to remember for later.

This is a good pattern to remember -- keep the data in the type, rather than having to remember what various bools mean.

You could also consider things like -> Result<char, char>, where you get Ok(c) when c is a vowel or Err(c) when c isn't a vowel.

Or one could go further and make your own enum so you can if let Vowel(c) = ... and such. Whether that'd be worth it depends on how broadly one would need such a thing, though. It's overkill for something done only once.

[cyberphine]

Thank you! I am seeing that this is the more idiomatic pattern in Rust.