16
\$\begingroup\$

This is a program for converting English to the pseudo-language Pig Latin. Not the strictest form of Pig Latin but it most likely will accurately output an argument in Pig Latin. I got the idea from a list of ideas on Github. Is there any way I can improve it concisely?

def pig_latin(message):
    consonants = ['b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y',
                  'z']
    new_message = ''

    for word in message.split():
        ay = ['a', 'y']
        listed_word = list(word)
        word_copy = word
        moved_consonants = ''

        for letter in listed_word.copy():
            if letter.lower() == 'y':
                if letter in word_copy[0]:
                    moved_consonants += letter
                    listed_word.remove(letter)
                else:
                    break
            elif letter.lower() in consonants:
                moved_consonants += letter
                listed_word.remove(letter)
            else:
                break

        listed_word.append('-' + moved_consonants + ''.join(ay))
        new_message += ''.join(listed_word) + ' '

    return new_message


print(pig_latin('Can you read pig latin?'))
\$\endgroup\$
  • 8
    \$\begingroup\$ To the close voter (/ presumably downvoter), they said they got the idea from a github list, not the implementation. I don't see an authorship issue here. \$\endgroup\$ – Carcigenicate Sep 4 at 20:19
18
\$\begingroup\$

First, at the top you list all the consonants out. There are two things that can be improved here:

  • Since you only use it to check whether or not something is a consonant, it should be a set. It's much more efficient to to a membership lookup on a set than it is to do one on a list. Just replace the [] with {}.

    consonants = {'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y', 'z'}
    
  • Second, there's a slightly less painful way to generate those letters. Python's string module contains a ascii_lowercase built-in that holds 'abcdefghijklmnopqrstuvwxyz'. You can use that along with a set of vowels to limit what letters need to be hard-coded:

    import string as s
    
    vowels = {'a', 'e', 'i', 'o', 'u'}
    consonants = set(s.ascii_lowercase) - vowels  # Consonants are the set of letters, minus the vowels
    

    I personally prefer this way.

You could also just change your algorithm to use vowels directly.


Just to clear something up,

word_copy = word

does not create a copy of word. This just creates a second name for the word string. For Strings this doesn't matter because Strings are immutable, but with a mutable object, this will bite you:

my_list = []
list_copy = my_list  # Does not actually create a copy!
my_list.append(1)
print(my_list, list_copy)  # prints [1] [1]

Notice how both lists were added to. This happens because there's actually only one list. Both names are referring to the same list.

For the sake of clarity, I'd rename it to say what it's purpose is. Offhand though, I can't see the need for word_copy at all! It would make sense if it was being used as an accumulator for a loop or something, but the only time its ever used is at word_copy[0], and since you never reassign word, you could just do word[0]. I'd simply get rid of word_copy.

Along the same lines, I'd reconsider ay. The name you've given it is exactly as descriptive as the string it contains, and is only ever used in one place. At the very least, I'd rename it to something meaningful:

pig_latin_suffix = ['a', 'y']

I'll also note that there's no reason to use a list of Strings here instead of a multi-character String. They behave the same in this case:

" ".join(['a', 'y'])
'a y'

" ".join("ay")
'a y'

Strings are iterable just like lists are.


I think pig_latin is too big. It's doing two main jobs: breaking the message into words, and processing the words. I would make the processing step its own function:

def process_word(word):
    ay = ['a', 'y']
    listed_word = list(word)
    word_copy = word
    moved_consonants = ''

    for letter in listed_word.copy():
        if letter.lower() == 'y':
            if letter in word_copy[0]:
                moved_consonants += letter
                listed_word.remove(letter)
            else:
                break
        elif letter.lower() in consonants:
            moved_consonants += letter
            listed_word.remove(letter)
        else:
            break

    listed_word.append('-' + moved_consonants + ''.join(ay))

    return ''.join(listed_word)

def pig_latin(message):
    new_message = ''

    for word in message.split():
        processed_word = process_word(word)

        new_message += processed_word + ' '

    return new_message

process_word could arguably be broken down further. This is already much better though. The immediate benefit is now you can test individual words, and not have to worry about how the rest of the code will react:

print(process_word("Can"))  # Prints 'an-Cay'
\$\endgroup\$
  • \$\begingroup\$ print(process_word("Can?")) "an?-Cay" probably isn't what OP wanted. \$\endgroup\$ – Griffin Sep 5 at 12:43
  • 2
    \$\begingroup\$ @Griffin That's what their's prints. I was just rolling with it. \$\endgroup\$ – Carcigenicate Sep 5 at 13:41
  • \$\begingroup\$ for what its worth if you do need to create a genuine copy the way to do that would be: from copy import deepcopy; word_copy = deepcopy(word). That way you're actually creating a copy and not just a reference to the original \$\endgroup\$ – TheSaint321 Sep 5 at 20:43
  • \$\begingroup\$ @TheSaint321 I can't think of a circumstance where you would ever need to copy a String though. I wouldn't be surprised if deepcopy just immediately returns Strings passed to it. \$\endgroup\$ – Carcigenicate Sep 5 at 22:09
  • 1
    \$\begingroup\$ yup thought about it for another second you're right. There's no point in deep copying a string. Its still a pretty useful tool nevertheless for more complicated objects \$\endgroup\$ – TheSaint321 Sep 6 at 1:47
12
\$\begingroup\$

As this is a simple text transformation, the regular-expression module re is your friend.

Processing letters one at a time is h-a-r-d. It would be simpler to process things one word at a time, as suggested by @Carcigenicate.

The re.sub(pattern, repl, text, ...) function is interesting in that it allows you to specify a string or a function for the replacement. As a first approximation:

import re

def pig_latin(text):

    def transform(match):
        first, rest = match.group(1, 2)
        if first.lower() in "aeiou":
            return first + rest + 'way'
        else:
            return rest + first + 'ay'

    return re.sub(r'(\w)(\w*)', transform, text)

print(pig_latin("Can you read pig latin?"))

anCay ouyay eadray igpay atinlay?

Here, the regex is extracting entire words with two capturing groups. The first group is simply the first letter \w, an the second group is the rest of the letters (if any) in the word \w*. The match is passed to the transform function, from which groups #1 and #2 are extracted and assigned to first and rest. If the first letter is a vowel, the replacement text is generated one way; if not it is generated a second way. The returned value is used as the replacement in the re.sub(...) call.

The transform function can easily be modified to include the -, if desired, as well as additional special handling of y, if required.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.