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
letterandeng_letteringet_letter()... \$\endgroup\$