6
\$\begingroup\$

I'm using a selfmade python program for learning new words in new languages. Originally I made this program for a university course but now I'm just trying to imrove it and make the code "prettier" and perhaps some small functionality improving. :)

The basic principle is that the program reads from a file the foreign word and the native word, saves them into two lists ([foreign_words] and [native_words]). If I remember the word correct, it will remove the word from both lists and ask a random new word from the lists. If I fail to remember it correctly, it will not remove it, and it will copy the words so that it'll have to ask the word twice more. The actual code is written in my native language but I'll try to translate most of it. Also the actual program is quite long so I will try to include only the relevant parts of it.

The actual files are in format:

foreign_word:native_word, another_word 

and the words will be saved to their respective lists. This below is how I read the files. Just for reference. It works and I don't think I really need to change anything there.

for line in open(file, "r", encoding="UTF-8"):
    if ":" in line:
        (foreign, native) = line.rstrip("\n").split(":")
        if "," in foreign:
            foreign_words += [foreign.split(",")]
        else:
            foreign_words += [foreign]
        if "," in native:
            native_words += [native.split(",")]
        else:
        native_words += [native]
    self.__foreign_words = foreign_words
    self.__native_words = native_words
    x = random.randint(0, (len(self.__foreign_words) - 1))
    self.__index.set(x)

Now for the part in my program that I wish to improve. I feel like it's really not too effective code even though it mostly does what it's supposed to. I use isinstance(correct_word, str) to check if the word is a single word (not separated by a comma in the file) or multiple words (separated by commas in the file). But I'm struggling to find a proper way to check if the answer is correct in the case that there are multiple options (separated by commas), so that either or any of the answers would suffice. Currently it just works if I type the whole word with the commas included or if it's not separated by the commas in the first place.

How could I compare the answer to the single word (string), or if it's a list of two or three words, check if at least one of them is correct? I tried using:

if answer in correct_word:
    #deleting the words from the lists and congratulating

but that didn't really do exactly what I wanted it to.

I think most of the relevant code should be here:

L = len(self.__foreign_words)
x = random.randint(0, L - 1)
#quits the program if the list length is 0
if L > 0: 
    if language == "F":
        correct_word = self.__native_words[x]
    elif language == "N":
        correct_word = self.__foreign_words[x]

    if isinstance(correct_word, str):
        correct_word = correct_word
    else:
        correct_word = ", ".join(correct_word)
    self.__question.set(correct_word)
    # Above I only set the question to be in the form of "word, another word" or "word"
    # instead of ['word', 'another word'] or "word"

    # this here I wish to improve
    if answer == correct_word:
        #here's also code that congratulates on correct answer (not included)
        del self.__foreign_words[x], self.__native_words[x]
    else:
        # telling you here how bad you are for not remembering the word (not included)
        y = self.__wrong_answers.get() + 1
        self.__wrong_answers.set(y)
        self.__foreign_words.append(self.__foreign_words[x])
        self.__native_words.append(self.__native_words[x])

    #checks if the list is still not empty after deleting the previous ones
    if L - 1 > 0:
        x = random.randint(0, L - 1)
        self.__index.set(x)
        if language == "F":
            new_word = self.__foreign_words[x]
            if not isinstance(new_word, str):
                new_word = ", ".join(self.__foreign_words[x])

        elif language == "N":
            new_word = self.__native_words[x]
            if not isinstance(new_word, str):
                new_word = ", ".join(self.__native_words[x])

        self.__question.set(new_word)

I might be a bit rusty with my code but that's why I'm posting here, to see if you guys could help me figure out way(s) to improve it! :) Hope the code isn't too messy.

\$\endgroup\$
2
\$\begingroup\$

In your file reading part:

Use the with .. as construct

Use

with open(file, "r", encoding="UTF-8") as words_file:
    for line in words_file:

This will ensure that the file will be closed at the end.

split is more awesome than you think

>>> "abc".split(",")
["abc"]

So there is no need to check whether there is a "," in there. Note that both this and your original code will not strip the spaces after ":" and ",", if you enter for example:

foreign_word: native_word1, native_word2

So you might have to do something like this at the end:

foreign = [word.strip() for word in foreign]

You can also directly assign the read foreign_words to the class, no need for the intermediary list:

self.__foreign_words.append(foreign.split(","))
self.__native_words.append(native.split(","))

list.append() is also faster than list += other_list

From the code you posted I don't see any reason to have this in the file reading part:

x = random.randint(0, (len(self.__foreign_words) - 1))
self.__index.set(x)

Because in the second part of the code you immediately set x as well.


In your second part:

if language == "F":
    correct_word = self.__native_words[x]
elif language == "N":
    correct_word = self.__foreign_words[x]

Can be simplified to

# In the class add this member
self.dictionary = {"F": self.__native_words, "N": self.__foreign_words
...
# Then you can just use later
correct_word = self.dictionary[language][x]

Instead of putting everything in an if block to make sure there are words to process, you could do:

assert(L>0)
# or
if L==0:
    exit() # or continue if you are in a loop
# Rest of code

join is more awesome than you think

>>> ", ".join(["abc"])
'abc'

So no need to check for isinstance(words,str), just use:

self.__question.set(", ".join(correct_word))

But I would not do this, because if you leave your correct_word as a list of correct words (possibly containing only one word, which is fine), then checking for the right answer becomes what you want:

if answer in correct_words:
    # Do stuff

When checking whether the list is still not empty, it is better to set L to the new length:

L = len(self.__foreign_words)
if L > 0:
    # Do stuff

Instead of

if L - 1 > 0:
    # Do Stuff

This way you can wrap this in a while loop more easily and repeat until the user quits or has finished all words. Your current code seems to only support asking for two words in a row.

\$\endgroup\$
  • \$\begingroup\$ That solved a lot of the problems that I had in my mind! Thank you! :) However, I was under the impression that the "if answer in correct_words:" would accept empty lines as well as substrings like accepting "someth" when the correct word is "something", does it not work like that for lists or was I simply wrong from the beginning? Testing it now, with most of your suggestions in use it does seem to work exactly as I want it to. \$\endgroup\$ – vivas Jul 1 '16 at 13:05
  • \$\begingroup\$ answer in ["foo", "bar"] will only give True if answer is either "foo" or "bar". It is similar for answer in ["foo"]. This relies on the changes I proposed (always using the result of split(","), thus storing a list even if there is only one correct word). \$\endgroup\$ – Graipher Jul 1 '16 at 13:31
  • \$\begingroup\$ "f" in "foo, bar" will return True, as you noticed. The in keyword does different things on strings and lists. \$\endgroup\$ – Graipher Jul 1 '16 at 13:32

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.