25
\$\begingroup\$

I wrote a python script that converts English to Entean language from an anime called "The Devil is a part timer". I am sloppy at coding and know that the script can be made better.

In the anime, the language in Ente Isla is called Entean, which is basically most of the consonants in the English alphabet switched around. Vowels and the consonants 'L', 'N', 'Q' keep their original position. ABCDEFGHIJKLMNOPQRSTUVWXYZ is changed to AZYXEWVTISRLPNOMQKJHUGFDCB. For example, 'human' is 'tupan', and 'world' becomes 'foklx'.

Here is the code:

alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".lower()
switched = "AZYXEWVTISRLPNOMQKJHUGFDCB".lower()

usr_txt = input("Enter Englsih text:")

print(usr_txt)

def get_letter(eng_letter):
    if alphabet.find(letter):
        if eng_letter.isupper():
            alpha_place = alphabet.find(letter.lower())
            return(switched[alpha_place].upper())
        else:
            alpha_place = alphabet.find(letter.lower())
            return(switched[alpha_place])
    else:
        return(eng_letter)


ent_word = ""
ent_output = ""


for word in usr_txt.split(" "): 
    ent_word = ""
    for letter in word:
        ent_letter = get_letter(letter)
        ent_word += ent_letter
    ent_output += " " + ent_word


print(ent_output.strip())

If you interested the updated code can be found at https://github.com/Logmytech/English-to-Entean

\$\endgroup\$
  • 3
    \$\begingroup\$ Question titles should reflect what the code does, so I edited your title to reflect that. \$\endgroup\$ – Barry Nov 25 '15 at 16:47
  • 1
    \$\begingroup\$ Your code is broken, as you intermix letter and eng_letter in get_letter()... \$\endgroup\$ – holroy Nov 25 '15 at 18:17
  • \$\begingroup\$ i have created an updated version of the script according to the suggestion in the accepted answer codereview.stackexchange.com/questions/112016/… \$\endgroup\$ – user3620828 Nov 27 '15 at 10:58
  • \$\begingroup\$ I notice that you have switched your accepted answer to a code-only answer. You should probably consider what the community thinks about that practice. \$\endgroup\$ – 200_success Mar 15 '16 at 13:58
  • \$\begingroup\$ @200_success I thought that you could select multiple answers when I saw the tick mark on multiple answers. I was not aware that it switches the answer.Thanks for pointing that out. \$\endgroup\$ – user3620828 Mar 16 '16 at 8:45
16
\$\begingroup\$

The most idiomatic Python solution would be to use the str.translate() function, which solves exactly this problem.

I don't see any reason to split the input into words, then join the results back together. Just translate the entire string at once (where space maps to itself).

\$\endgroup\$
17
\$\begingroup\$

Use Dictionaries

We are basically just mapping from one letter to the next. The most naturally way to do that is to actually use a map:

to_entean = {...}

The easiest way to build up the dictionary is to use zip():

to_entean = {eng: ent
    for eng, ent in zip(string.ascii_uppercase, 'AZYXEWVTISRLPNOMQKJHUGFDCB')
    }

Now to_entean['A'] == 'A' and to_entean['B'] == 'Z', etc. This simplifies your get_letter function to:

def get_letter(letter):
    ent = to_entean.get(letter.upper(), letter)
    if not letter.isupper():
        ent = ent.lower()
    return ent

String Concatentation

String concatenation is slow. Also it reads terribly. What we want to do is call get_letter for every letter in word and then join them all together. That's what join() is for:

ent_word = ''.join(get_letter(letter) for letter in word)

Why split?

Since get_letter() handles spaces already, we don't have to split into words. Just transform the whole thing:

print(''.join(get_letter(letter) for letter in usr_txt))

Which, if you prefer, can also be spelled:

print(''.join(map(get_letter, usr_txt)))

Typo

You have a typo here:

usr_txt = input("Enter Englsih text:")
                       ^^^^^^^
\$\endgroup\$
6
\$\begingroup\$

Barry and 200_success has made some algorithmic suggestsion, which I verify the speed of later on compared to your original version and two modified versions of my own. But before tackling those version, lets review your code:

  • Avoid having top level code – In your script you start with some constants, followed by asking for text at top level, then you define a function, get_letter, before handling the text you asked for. This is not clean. If you introduce a function to translate an entire text, and a main function you could make this code modular.

    You would then be able to import your module from another script, i.e. a script downloading the anime, and you could call your to_entean() from your module. To still allow running this as a script you use the if __name__ == '__main__': main() at the end of your file. See code example below

  • Store result of function, when used repeatedly – For each letter you execute the alphabet.find() three times, that it two times too much. Do it once, and be over with it.
  • Bug 1: You intermix eng_letter and letter in get_letter() – This is a fail, which you possibly introduced when copy-pasting code into Code Review... They should still be the same
  • Bug 2: Punctuation characters are wrongly translated – When using find it returns -1 when not finding the letter, which is a legal index in Python, so it happily returns the character in that position: b
  • Please use full variable names – Stuff like ent_word, eng_letter or usr_txt should be spelled out in most cases, like entean_word, english_letter or user_text. It makes it easier for everybody in the long run
  • Naming is mostly good - If you expand the names, your naming is mostly good. I would however make the constants to be UPPER_SNAKE_CASE to clearly identify them as constants
  • Make the full alphabet as an constant – In your code you do a lot of lower() and upper(). Especially when setting a constant, this is wasteful. Within get_letter() it is more understandable, but if you double the constants with both lower- and uppercase alphabet you don't need to call either any place
  • Add vertical spacing in compact blocks – Your get_letter() is quite condensed, and hard to read, and with all the return statements intermixed it is kind of hard to see what is really happening. My rule is to add blank lines before if, elif, else, for and while, and sometimes in front of logical code blocks. Also note that functions should have two blank lines in front of them
  • Splitting, adding and stripping strings is a little expensive – Most of the time your code is used doing this. Know your options, and choose the one most suited for you. When refactoring your code I keep one version with the splitting, and one version joining like Barry did. See time difference on this two.

Code refactored

When refactoring according to my comments, I end up with the following code:

FULL_ENGLISH_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
FULL_ENTEAN_ALPHABET = "AZYXEWVTISRLPNOMQKJHUGFDCBazyxewvtisrlpnomqkjhugfdcb"


def get_letter(letter):
    """Return english letter translated to entean."""
    alpha_place = FULL_ENGLISH_ALPHABET.find(letter)

    return FULL_ENTEAN_ALPHABET[alpha_place] if alpha_place >= 0 else letter


def to_entean(text):
    """Return text translated from english to entean."""

    entean_output = ""

    for word in text.split(" "): 
        entean_word = ""

        for letter in word:
            entean_letter = get_letter(letter)
            entean_word += entean_letter

        entean_output += " " + entean_word

    return entean_output.strip()


def main(text=None, with_output=False):

    if with_output:
        print("\nmain")

    if text is None:
        text = input("Enter English text: ")

    if with_output:    
        print(text)
    entean_text = to_entean(text)
    if with_output:
        print(entean_text)

The default parameters of main() was used to enhance my testing below, so it could be written simpler. If written without respect to the other versions, I would most likely also strip away FULL_ from the constants.

Notice how I also simplified the entire get_letter() into a single return as I found the letter once, and extended the alphabet to both upper- and lowercase characters.

The alternate version of the to_entean() using join is as follows:

def to_entean_join(text):
    """Return text translated from english to entean."""

    return ''.join(get_letter(letter) for letter in text)

Performance review

Using variants of the main() method above, I tested both your original, mine versions and 200_success version. I tested within IPython 2, but I assume the same differences will keep up even for Python 3. Do note that it it a little futile to performance test this code, but it could be useful for seeing how changes in code affect performance as well.

The test I ran was:

In [1]: for test_main in (main_org, main_barry, main, main_v2, main_200_success):
  ...:     print('{:<16}: '.format(test_main.__name__), end='') 
  ...:     %timeit test_main("Hello world! Welcome humans to the Entean world. This text is written in Entean. Cool?")
  ...:

Which gave the following output:

main_org        : 10000 loops, best of 3: 93.2 µs per loop
main_barry      : 10000 loops, best of 3: 70.7 µs per loop
main            : 10000 loops, best of 3: 57.6 µs per loop
main_v2         : 10000 loops, best of 3: 52.7 µs per loop
main_200_success: The slowest run took 6.24 times longer than the fastest. This could mean that an intermediate result is being cached 
1000000 loops, best of 3: 649 ns per loop

As can be seen, the clear winner performance wise is the version by 200_success, which does make sense as that is a specialised version using a dedicated translation method.

Another interesting fact is that changing from splitting, adding and stripping (in main) to joining (in main_v2) is almost a 10% reduction in time. Using dict's (in main_barry) and upper() and isupper(), is faster than original code, but slower than find(). In both my and Barry's join version I used the one without map(), if using map() the times improve by 4-6 µs.

A note on testing the performance related to initialisation cost of the various solution, as both Barry's and 200_success's version has a small initialisation costs (not included in above timings). But this is in the rate of 60-70 µs, so if doing loads of translations it can be justified using more time for initialisation.

The main lesson to learned through all of this is two-folded:

  • Using a dedicated resource, like str_translate(), can quite often reduce execution time drastically
  • Cleaning up your code, and avoiding unneccessary operations, can also have a major influence on execution time (in this case almost 50% faster). Not to mention it easier to maintain and come back to a later stage
\$\endgroup\$
  • \$\begingroup\$ The assumption that the performance of str.translate() doesn't change much between versions is actually invalid: there is a dramatic difference between Python 3.4 and 3.5. \$\endgroup\$ – 200_success Dec 16 '15 at 9:32
  • \$\begingroup\$ @200_success It went faster in 3.5, so str.translate is even faster then. Do you know if translate ever have been slower than alternatives which would break my recommendation? \$\endgroup\$ – holroy Dec 16 '15 at 11:36
1
\$\begingroup\$

Here's your full program re-implemented using 200_success's suggestion of using str.translate:

from string import ascii_letters as alphabet

switched = 'azyxewvtisrlpnomqkjhugfdcb'
switched += switched.upper()

entean_translation = str.maketrans(alphabet, switched)

user_text = input("Enter Englsih text:")

print(user_text)
print(user_text.translate(entean_translation))
\$\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.