8
\$\begingroup\$

I want to improve my code's performance and running time, looking for write my loops better.

For example, I have a dictionary that contains words as keys, and their translation in Spanish as values.

{
    'Hello' : 'Hola',
    'Goodbye' : 'Adios',
    'Cheese' : 'Queso'
}

I also have a given English sentence, and I want to iterate over any word in my dict and replace it with the Spanish translation. For this scenario I consider that up to one word could be exist in the given sentence.

I wrote a basic code that do that, but I am not sure that it is best practice:

words_list = {
        'Hello' : 'Hola',
        'Goodbye' : 'Adios',
        'Cheese' : 'Queso'
    }
sentence = "Hello, I want to talk Spanish"

for english_word in words_list.keys():
    if english_word in sentence:
        sentence = sentence.replace(english_word, words_list[english_word])
        break

print sentence

How can I write it better?

\$\endgroup\$
  • 1
    \$\begingroup\$ should it be case insensitive? \$\endgroup\$ – RomanPerekhrest Aug 29 '17 at 14:50
12
\$\begingroup\$

Problems

  • not properly handling words, just substrings. Currently, you are using str.replace() that would replace substrings in a string, not complete words, which means that it would behave incorrectly when your replacement is a part of another word - e.g. "Cheese" would be replaced from "Cheesecake" becoming "Quesocake"
  • just a single substring is replaced. you have that break statement which basically stops all the replacements after the first one occurred
  • handling words in different cases. You are also not properly handling the titled or upper-cased words
\$\endgroup\$
4
\$\begingroup\$
for english_word in words_list.keys():

is a bad approach, you should instead understand that your dictionary is for looking things up, not for iterating over in this case. Each entry in it keys to another value and you want to use it that way.

I would instead set up a list of words that have been stripped of whitespace and syntax, and compare as lower case. Then I would use those to ".get()" from the dictionary, in this case you would have to use dict keys that are lower case as well, but this makes behavior much more consistent.

a quick code snippet on how to use .get()

my_dict.get(english_word, english_word)

If the entry is in the dictionary this will return the value, otherwise it will return the key.

\$\endgroup\$
3
\$\begingroup\$

Wait. You're iterating over the dictionary, rather than the string? I realize computers aren't the same as humans, but at least some of the intuition transfers. If you were translating a sentence, would you look through a dictionary and check each word to see if it's in the sentence, or look through the sentence and check each word to see whether it's in the dictionary?

You should first split the string on spaces, then deal with punctuation. You should end up with a list of words, and you can then iterate over the list and translate them one by one. Or you can just do a list comprehension. The basic code would be:

translated_word_list = [my_dict.get(english_word) for english_word in original_word_list]

There would then be various modifications depending on how you want to deal with various issues. You'll have to decide how you want to handle capitalization; Erich's suggestion of setting everything in the dictionary and in the string to lower case is one option, but depending on the use case you might not want, to take one example, to translate "Captain Hook" as "capitán gancho", and you might want to preserve capitalization of the first word in the sentence. You should also decide how you want to deal with words that aren't in the dictionary. Once you've finished translating the words, you can then paste them back into a single string.

Also, on the object level, you do realize that this is a very poor way of translating, and are just asking for academic purposes, right? This approach would do stuff like translate "I do not speak Spanish" as "Yo hago no hablar Espanol".

\$\endgroup\$
  • \$\begingroup\$ using get without a default in this case would KeyError. \$\endgroup\$ – Erich Aug 30 '17 at 5:09
  • \$\begingroup\$ You mean in general, or when attempted when a key isn't in the dictionary? My understanding is that in the latter case, it returns None. As I said, this is the basic code, and one would have to decide how to handle missing keys. Your method of returning the original word is good in many cases, but there would be cases where one might want to do something else or in addition. That's something one should be making a deliberate choice about for specific, thought out reasons. Just getting code to not return an exception is rarely a good goal to be aiming for. \$\endgroup\$ – Acccumulation Aug 31 '17 at 2:31

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.