6
\$\begingroup\$

Yes, I know this exercise has been posted before, but I'm posting it again because I want feedback on why certain parts of my code aren't idiomatic. I'm new to rust, and my code works, but as I said, I'd like to have parts pointed out that could be implemented another way, and why my way is "wrong". Thx in advance!

I haven't given naming that much of a thought, many variables could probably be named better. I could also extract a lot of parts into their own functions, however I'm more gunning for advice on which kind of patterns are applicable to my solution, rather than how I could modularize my code more by simple extraction. I'm also not looking for "This is the best solution for this problem", I'm looking for "this part is 'wrong/unsafe/stupid' because of X, here is how to do it better".

pub fn pig_latinize(text: String) -> String {
    let allowed_specials = ['.', ',', '?', '!'];
    text.split_whitespace()
        .map(|word_with_special_char| {
            let annoying_character = allowed_specials
                .iter()
                .any(|special| word_with_special_char.ends_with(special.to_string().as_str()));
            let mut suffix = "";
            let word = if annoying_character {
                let (word_without_special_char, special_char) = word_with_special_char
                    .split_at(word_with_special_char.len() - 1);
                suffix = special_char;
                word_without_special_char
            } else {
                word_with_special_char
            }
            .to_string();

            pig_latin_single_word(word) + suffix
        })
        .collect::<Vec<_>>()
        .join(" ")
}

fn pig_latin_single_word(word: String) -> String {
    let mut word = word.clone();
    let vowels = "aeiou".to_string();
    let first_letter = word.remove(0);
    let is_uppercase = first_letter.is_uppercase();
    let mut lower_first_letter = first_letter.to_lowercase().to_string();
    if vowels.contains(lower_first_letter.as_str()) {
        word.insert(0, first_letter.clone());
        lower_first_letter = "h".to_string();
    }
    word.push_str(format!("-{}ay", lower_first_letter).as_str());
    if is_uppercase {
        let first_letter = word.remove(0);
        word.insert(0, first_letter.to_string().to_uppercase().remove(0));
    }
    word
}

Tests:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_pig_latin() {
        let input = "This is a very complicated sentence. It is actually more than one sentence.".to_string();
        let expected = "His-tay is-hay a-hay ery-vay omplicated-cay entence-say. It-hay is-hay actually-hay ore-may han-tay one-hay entence-say.".to_string();
        
        let actual = pig_latinize(input);
        
        assert_eq!(actual, expected);
    }
    
    #[test]
    fn test_pig_latin_multiple_words() {
        let input = "This is a very complicated sentence".to_string();
        let expected = "His-tay is-hay a-hay ery-vay omplicated-cay entence-say".to_string();
        
        let actual = pig_latinize(input);
        
        assert_eq!(actual, expected);
    }
    
    #[test]
    fn test_pig_latin_small1() {
        let input = "This".to_string();
        let expected = "His-tay".to_string();
        
        let actual = pig_latin_single_word(input);
        
        assert_eq!(actual, expected);
    }
    
    #[test]
    fn test_pig_latin_small2() {
        let input = "this".to_string();
        let expected = "his-tay".to_string();
        
        let actual = pig_latin_single_word(input);
        
        assert_eq!(actual, expected);
    }
    
    #[test]
    fn test_pig_latin_small3() {
        let input = "one".to_string();
        let expected = "one-hay".to_string();
        
        let actual = pig_latin_single_word(input);
        
        assert_eq!(actual, expected);
    }
}
```
\$\endgroup\$
0

2 Answers 2

Reset to default
6
\$\begingroup\$

The two things that jump out at me are:

Use &str Instead of String for Input Parameters

All other stringy types should convert to this efficiently. If you use String, you’ll often need to clone. Since cloning is expensive, Rust deliberately requires you to do it explicitly, and there will be a lot of .to_string() calls around. These are code smells.

If you were editing a String in place, or could move instead of copying, there would be a reason to prefer text: mut String to text: &str. As it is, though, you’re only forcing yourself to make extra copies of your test strings on the heap. Instead of,

let input = "this".to_string();
let expected = "his-tay".to_string();
        
let actual = pig_latin_single_word(input);
        
assert_eq!(actual, expected);

You should be able to write something like:

assert_eq!(&pig_latin_single_word("this"), "his-tay");

Use Helper Functions

The implementation of the closure within .map looks idiomatic in its structure, but it’s long enough that you might want to move it to a nested helper function.

You likely want to split the input string into slices of consecutive alphabetic or non-alphabetic characters, and call pig_latin_single_word only on the alphabetic chunks, without making a deep copy of them. You can then take advantage of the fact that every alphabetic chunk is followed by a non-alphabetic chunk, and vice versa, because of how you’re splitting them.

Consider Building From Slices

A Pig-Latin string will be composed entirely of slices of the original string, plus a few const strings such as "ay" That is, if your original text is "this!", the Pig-Latin output ("his-tay!") will be &text[1..4], followed by "-". followed by &text[0..1], followed by "ay", followed by &text[4..5]. Each of these slices either has the same lifetime as text, or 'static lifetime.

One approach you might therefore take is to write a while loop, which starts with an empty String and builds the output string by appending each chunk in order. This is similar to how you would write an efficient Pig-Latin function in C++.

You might also build an iterator or Vec<&str> over the chunks of the Pig-Latin string. Write a function that turns an input chunk into one to four output chunks, and call it with .flat_map(). Once you have the iterator over output chunks, you could then .fold() over the output chunks, appending each one to a String initialized to String::new(). You don’t need to get that complicated to solve this problem, but it’s a handy technique to have in your toolbox. It comes in especially handy when you’re passing the chunks to a buffered I/O function.

\$\endgroup\$
5
\$\begingroup\$

I'll make some simple function design remarks.

Of course it is good to split the sentence into words and other characters, and have a function that translate the words into Pig Latin. So I like that function split between pig_latinize and pig_latin_single_word (why not pig_latinize_single_word though?)

Generally the list of "special characters" are not part of a word. If I'd have to create Pig Latin for a sentence I'd just select those pieces in a sentence that constitute a word (say \w+) and leave the other characters as they were. It is possible to do this by iterating over regex matches using match_indices - the non-matching parts are relatively easy to find using the index & size of the previous match and the current index.

If there is a specific set of characters that are to be supported then it is a good idea to document that and use a guard clause to make sure that the function fails fast instead getting into a GIGO situation (garbage in, garbage out). Of course regex is of great help here too. For user input it is probably best to rely on additional checks (e.g. for string encoding), but those do not have to be in this particular function.

I'd make sure that the tests are well described. Currently the test name are not very descriptive - and generally tests need to be documented anyway.

Generally the code is well formatted and relatively easy to read. Capitalized letters are handled neatly.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.