8
\$\begingroup\$

To train my Python skills I have made a vocabulary trainer which runs on a terminal:

import random
import os
import sys

FILE_PATH = "./words.txt"

class Entry:
    def __init__(self, french, english):
        self.french = french
        self.english = english

entries = []

if os.path.isfile(FILE_PATH):
    words = open(FILE_PATH, "r")

    for line in words:
        words = line.split(",")
        entries.append(Entry(words[0].strip(), words[1].strip()))


def insert():
    while True:
        french = raw_input("French word: ")

        if french == "#":
            return 

        english = raw_input("English word: ")

        if english == "#":
            return 

        entries.append(Entry(french, english))
        backup_wordpairs()

def query():
    total = 0
    right = 0
    wrong = 0

    while True:
        i = random.randint(0, len(entries) - 1)
        french = raw_input("French translation of " + entries[i].english + ": ")

        # TODO: Add a statistics which is displayed before leaving (x of y correct. Equal z percent).
        if french == "#":
            percentage = (right  * 100) / total
            print("You answered " + str(right) + " question out of " + str(total) + " correct.")
            print("Percentage: " + str(percentage) + " %")
            return 

        total = total + 1

        if french.strip() == entries[i].french.strip():
            print("Correct.")
            right = right + 1
        else:
            print("Wrong!")
            wrong = wrong + 1

def printall():    
    if len(entries) == 0:
        print("No words stored.")
        return

    for i in range(len(entries)):
        print(entries[i].french + " : " + entries[i].english)

def backup_wordpairs():
    woerter = open(FILE_PATH, 'w')

    for wort_paar in entries:
        woerter.write(wort_paar.french + "," + wort_paar.english + "\n")

    woerter.close()

def reset_list():
    global entries

    entries = []

    if os.path.isfile(FILE_PATH):
        os.remove(FILE_PATH)

while True:
    command = raw_input("Please enter a command: ")

    if command == "add":
        insert()
    elif command == "test":
        query()
    elif command == "list":
        printall()
    elif command == "end":
        break
    elif command == "reset":
        reset_list()
    else:
        print("No known command.")

print(" ------- Vocabulary becomes terminated. ----------  ")
sys.exit()

My GitHub repository (including a README-file with user-instructions and a screenshot).

The application runs fine but I would appreciate feedback from more experienced Python-developers. What would you have done differently and why?

\$\endgroup\$
  • \$\begingroup\$ This can't be python-3.x, since you used raw_input(). I have retagged the question as python-2.x. \$\endgroup\$ – 200_success Jun 27 '18 at 5:08
  • \$\begingroup\$ @200_success Have used Eclipse with Python-plugin. Rather sure if have configured 3.6 in the project properties. Anyway: Thanks for the hint and the correction. \$\endgroup\$ – michael.zech Jun 27 '18 at 5:38
10
\$\begingroup\$

Currently your code is all over the place. You have global variables (which should be named in ALL_CAPS according to Python's official style-guide, PEP8), functions which modify these globals, then some actual code in between the functions and at the end some code that actually executes the code, also in the global namespace (preventing you from importing any of the functions defined here in another script).

In addition I would also change:

  1. Use random.choice(entries) instead of random.randint(0, len(entries)).
  2. Make Entry a collections.namedtuple (which allows using tuple unpacking).
  3. Since you say you are using Python 3.x, use the new f-strings introduced in Python 3.6.
  4. Where this is not possible, use str.format instead of building the strings using addition and manual calls to str.
  5. Make main take the file path (defaulting to the global constant) so it is more extendable.
  6. Making sure to close all opened files again using with ... as.
  7. Use if __name__ = "__main__" guard to allow importing.
  8. Call strip on the manually entered words, removing the need to do that every time you compare them later.
  9. Remove the wrong variable, it is unused (and can be easily calculated as total - right).
  10. raw_input is not called that anymore in Python 3. It is now called input (and the input from Python 2 is gone).
import random
import os
import sys
from collections import namedtuple

FILE_PATH = "./words.txt"
Entry = namedtuple("Entry", ["french", "english"])


def insert(entries):
    while True:
        french = input("French word: ").strip()
        if french == "#":
            return
        english = input("English word: ").strip()
        if english == "#":
            return
        entries.append(Entry(french, english))


def query(entries):
    total, right = 0, 0
    while True:
        word_pair = random.choice(entries)
        french = input(f"French translation of {word_pair.english}:")

        if french == "#":
            percentage = (right * 100) / total
            print(f"You answered {right} question out of {total} correct.")
            print(f"Percentage: {percentage} %")
            return
        total += 1
        if french.strip() == word_pair.french:
            print("Correct.")
            right += 1
        else:
            print("Wrong!")


def printall(entries):
    if len(entries) == 0:
        print("No words stored.")
        return
    for word_pair in entries:
        print("{} : {}".format(*word_pair))


def backup_wordpairs(entries, file_path):
    with open(file_path, 'w') as words:
        for word_pair in entries:
            words.write("{},{}\n".format(*word_pair))


def main(file_path=FILE_PATH):
    if os.path.isfile(file_path):
        with open(file_path) as words:
            entries = [Entry(*map(str.strip, line.split(",")))
                       for line in words]
    else:
        print("File does not exist")
        sys.exit()

    while True:
        command = input("Please enter a command: ")

        if command == "add":
            insert(entries)
            backup_wordpairs(entries, file_path)
        elif command == "test":
            query(entries)
        elif command == "list":
            printall(entries)
        elif command == "end":
            break
        elif command == "reset":
            entries.clear()
            if os.path.isfile(file_path):
                os.remove(file_path)
        else:
            print("No known command.")

    print(" ------- Vocabulary becomes terminated. ----------  ")
    sys.exit()

if __name_ == "__main__":
    main()

As you can see now, almost all functions take entries as the first argument and modify it somehow. This should tell you that you could easily make this a class.

\$\endgroup\$
  • \$\begingroup\$ You have a lot of code duplication and logical dependencies in your code. For example, in function insert() you repeat code of getting input and checking for value '#' (for french and english words). Names of your functions don't tell anything about what they do. For example, when I read call of function insert() I ask a question: "Insert what? From where we get what we will insert?" The same is for function query(): "Query what?". Also you have a problem with code readability, because you mixed pure calculations and I/O operations together. \$\endgroup\$ – Stexxe Jun 26 '18 at 20:45
  • \$\begingroup\$ @Stexxe This is a first step, taking what the OP has and making it better. If you have further improvements, feel free to add your own answer. \$\endgroup\$ – Graipher Jun 26 '18 at 20:46
  • \$\begingroup\$ As I know function map() is deprecated. Also, you have three "while True"; Is it possible to make one function for such repeated I/O operations and call it thrice? If-tree for commands in main() function could be expressed in more readable table form. Do you think having parameter in main() function is expectable? \$\endgroup\$ – Stexxe Jun 26 '18 at 20:56
  • \$\begingroup\$ Ok. I just though this is your version of requestor's program. \$\endgroup\$ – Stexxe Jun 26 '18 at 20:57
  • \$\begingroup\$ @Stexxe I would like to see a reference for map being officially deprecated now (all I can find is GvR saying it should die, 15 years ago). Having a parameter in main is not the best, but since it has a default it is fine if it is not so discoverable IMO. In my own code I would probably add arparse for command line arguments where this would be the first to be added. \$\endgroup\$ – Graipher Jun 27 '18 at 5:02
3
\$\begingroup\$

Code Organization

For a program of this size, free-floating code should be eliminated. Interleaving free-floating code between class and function definitions is particularly messy:

class Entry:
    ...

entries = []

if os.path.isfile(FILE_PATH):
    ...

def insert():
    ....

The same goes for your while True loop: it should be packaged inside a main() function.

Global variables (namely entries) should be eliminated.

For this program, I would recommend making your program object-oriented, because so many of your functions deal with entries. Specifically, I would split the code into two layers: a database layer and a UI layer. The database (the Vocabulary class in my example below) deals with parsing and writing the file, as well as manipulating vocabulary entries. The UI (the Trainer class) implements the commands (add, test, list, reset, end). The function names should ideally match the corresponding command names.

Properly structured programs should not need to call sys.exit() to exit normally. Execution just naturally ends.

Please don't mix languages when naming variables (e.g. woerter and wort_paar). If the code is mostly in English, then just stick to English names.

Design

The design of your code is too rigid and limited: it only does English-to-French translations. Those two languages are hard-coded into the strings and identifiers. Why not handle French-to-English and more? For very little additional effort, you could easily generalize the program:

  • The words.txt file is in CSV format. It could have any number of columns, and does not need to be limited to two languages. The file format could be self-documenting, with the first row containing the language names. (To read and write CSV files, don't split() and concatenate the content yourself; use the csv module.)
  • The vocabulary trainer should be given a source language and target language as parameters.

Implementation details

Files should always be open()ed in the context of a with block. That way, the file will automatically be closed when the block exits, even if it exits with an exception.

In the quiz loop, there is no need to keep track of the number of wrong answers, since it can be deduced from total - right. Instead of a while True: loop, I would use for total in itertools.count():.

Concatenating strings is tedious:

print("You answered " + str(right) + " question out of " + str(total) + " correct.")

There are many ways to compose strings in Python. One of the better ways, which is compatible with Python 2 and Python 3, is str.format().

Suggested solution

import csv
from itertools import count
import os
import random
from sys import stdout

class Vocabulary:
    def __init__(self, filename, languages):
        self.filename = filename
        if os.path.isfile(filename):
            with open(filename) as f:
                reader = csv.DictReader(f)
                self.entries = list(reader)
                self.languages = reader.fieldnames
        else:
            self.entries = []
            self.languages = languages

    def clear(self):
        self.entries = []

    def add(self, *entries):
        self.entries.extend(*entries)

    def save(self):
        with open(self.filename, 'w') as f:
            writer = csv.DictWriter(f, self.languages)
            writer.writeheader()
            for entry in self.entries:
                writer.writerow(entry)

    def random_entry(self):
        return random.choice(self.entries)


class Trainer:
    COMMANDS = ['add', 'test', 'list', 'reset']

    def __init__(self, vocabulary, source_language, target_language):
        self.vocabulary = vocabulary
        self.source_language = source_language
        self.target_language = target_language

    def add(self):
        def prompt_entries():
            while True:
                entry = {}
                for language in self.vocabulary.languages:
                    entry[language] = raw_input('{} word: '.format(language))
                    if entry[language] == '#':
                        return
                yield entry
        self.vocabulary.add(prompt_entries())
        self.vocabulary.save()

    def test(self):
        right = 0
        for total in count():
            entry = self.vocabulary.random_entry()
            q, a = entry[self.source_language], entry[self.target_language]
            ans = raw_input('{} translation of {}: '.format(self.target_language, q))
            if ans == '#':
                break
            elif ans == a:
                print('Correct.')
                right += 1
            else:
                print('Wrong!')
        print('You answered {} questions out of {} correctly.'.format(right, total))
        print('Percentage: {} %'.format(100 * right / total))

    def list(self):
        if not self.vocabulary.entries:
            print("No words stored.")
        else:
            writer = csv.DictWriter(stdout, self.vocabulary.languages, delimiter=':')
            for entry in self.vocabulary.entries:
                writer.writerow(entry)

    def reset(self):
        self.vocabulary.clear()
        self.vocabulary.save()


def main():
    t = Trainer(
        Vocabulary('words.txt', ['English', 'French']),
        source_language='English',
        target_language='French'
    )
    choices = ', '.join(t.COMMANDS + ['end'])
    while True:
        command = raw_input('Please enter a command ({}): '.format(choices))
        if command == 'end':
            break
        elif command not in t.COMMANDS:
            print('Unknown command.')
        else:
            getattr(t, command)()
    print(' ------- Vocabulary quiz terminated. ----------  ')

if __name__ == '__main__':
    main()
\$\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.