Kritik erwünscht zu meinem Französisch-Verbentrainer

[nooby]

Hallo zusammen

Ich habe mir für die Schule einen kleinen Französisch-Verbentrainer mit Python geschrieben.
Die Verben werden im XML Format gespeichert.
Falls ein paar Python Profis Verbesserungsvorschläge oder Verschönerungen haben immer her damit!

Code: Alles auswählen

import random
import pickle
import xml.dom.minidom
from xml.dom.minidom import parse
from colorama import init
from colorama import Fore, Back, Style

init()
random.seed()

rightAnswers = 0
wrongAnswers = 0
totalAnswers = 0

randomPersonDict = {0:"je", 1:"tu", 2:"il", 3:"nous", 4:"vous", 5:"ils"}
randomTenseDict = {0:"présent", 1:"imparfait", 2:"futur"}
statisticsPath = "statisticsVerbs.bin"

def saveStatistics(filename):
    global rightAnswers
    global wrongAnswers
    global totalAnswers
    save = pickle.Pickler(open(filename, "wb"),2)
    save.dump(rightAnswers)
    save.dump(wrongAnswers)
    save.dump(totalAnswers)

def loadStatistics(filename):
    global rightAnswers
    global wrongAnswers
    global totalAnswers
    save = pickle.Unpickler(open(filename, "rb"))
    rightAnswers = save.load()
    wrongAnswers = save.load()
    totalAnswers = save.load()

def getVerbs(filename):
    DOMTree = xml.dom.minidom.parse(filename)
    verbList = DOMTree.documentElement
    verbs = verbList.getElementsByTagName("verb")
    return verbs

def getRandomData(verbs):
    randomVerb = random.randint(0, len(verbs)-1)
    randomPerson = randomPersonDict[random.randint(0,5)]
    randomTense = randomTenseDict[random.randint(0,2)]
    return (randomVerb,randomPerson,randomTense)

def getAnswer(randomVerb,randomTense,randomPerson):
    rightAnswer = verbs[randomVerb].getElementsByTagName(randomTense)[0]
    rightAnswer = rightAnswer.getElementsByTagName(randomPerson)
    rightAnswer = rightAnswer[0].firstChild.nodeValue
    return rightAnswer

def checkAndPrintAnswer(inputAnswer,rightAnswer):
    global rightAnswers
    global wrongAnswers
    global totalAnswers
    if inputAnswer.strip() == rightAnswer:
        rightAnswers += 1
        totalAnswers += 1
        print(Fore.GREEN+"Richtig!")
        print()
    else:
        wrongAnswers += 1
        totalAnswers += 1
        print(Fore.CYAN+"Falsch.")
        print(Fore.CYAN+"Richtige Antwort wäre:")
        print(rightAnswer)
        print()

def printVerbData(randomVerb,randomTense,randomPerson):
    print(Fore.RED+"Verb:", verbs[randomVerb].getAttribute("deutsch"))
    print(Fore.RED+"Zeit:", randomTense)
    print(Fore.RED+"Person:", randomPerson)

def printStatistics():
    global rightAnswers
    global wrongAnswers
    global totalAnswers
    print()
    print()
    print(Fore.RED+"-------------------------Statistics-------------------------")
    print(Fore.RED+"            Richtige Antworten:", rightAnswers)
    print(Fore.RED+"            Falsche Antworten:", wrongAnswers)
    print(Fore.RED+"            Totale Anzahl Antworten:", totalAnswers)
    print(Fore.RED+"            "+str((100*rightAnswers)/totalAnswers) + "% Richtige Antworten.")


try:
    loadStatistics(statisticsPath)
except IOError:
    pass
for i in range(25):
    verbs = getVerbs("verbes.xml")
    randomVerb,randomPerson,randomTense = getRandomData(verbs)
    printVerbData(randomVerb,randomPerson,randomTense)
    rightAnswer = getAnswer(randomVerb,randomTense,randomPerson)
    inputAnswer = input()
    checkAndPrintAnswer(inputAnswer,rightAnswer)

printStatistics()
saveStatistics(statisticsPath)

[Sirius3]

@nooby: allen Code solltest Du in Funktionen packen und nichts auf Modulebene ausführen. »random.seed« ist unnötig. Die Dicts sind eigentlich beides Listen. Variablennamen sollten nicht den Datentyp enthalten. »global« solltest Du nicht verwenden, alles was in einer Funktion gebraucht wird, muss sie über Parameter betreten und über Rückgabewerte verlassen. Statt 'minidom' gibt es in Python 'ElementTree' das eine viel einfacheres und schöneres Interface hat. Um ein zufälliges Element aus einer Liste zu bekommen, gibt es random.choice. »randomVerb« ist kein Verb sondern nur eine Zahl. »totalAnswers« ist überflüssig, weil die Zahl jederzeit einfach über rightAnswers+wrongAnswers ausgerechnet werden kann.

[Hyperion]

Zudem solltest Du PEP8 mehr beachten. Deine Funktionsnamen sind eher Java-Style - in Python sollten diese klein und mit Unterstrichten getrennt sein, also ``save_statistics`` statt ``saveStatistics``. Das gilt auch für Deine anderen Namen.

Bist Du sicher, dasss Du wirklich ``Pickler`` nutzen willst? Für den Standard-Use-case gibt es (afaik bei allen Serialisierungsmodulen in der Standardlib) doch die Convenience-Funktionen ``load`` und ``dump``.

Klappt es wirklich, drei Mal hintereinander die Laden bzw. Speichern-Methode auf dem selben Pickler-Objekt aufzurufen und dabei wirklich verschiedene Werte zu bekommen? Ich habe ``pickle`` zwar nie verwendet, aber es würde mich doch stark wundern, wenn man das API von ``pickle`` so benutzen muss!

Generell könnte man fragen, ob ein binäres Format für diesen Anwendungsfall wirklich sinnvoll ist.

Strings sollte man nicht per ``+`` zusammensetzen. Benutze dafür die ``.format``-Methode von Strings oder die ``%`` Syntax. Da Du diese Escape-Sequenzen aber eh nur *innerhalb* von ``print`` verwendest, kannst Du diese dort ebenfalls als Parameter voranstellen, analog zu dem, wie Du es bei den variablen Parametern machst.

Edit: Wusste ich doch, dass PEP8 (bezüglich der Namenswahl) Dir schon *mehrfach* genannt worden ist:
http://www.python-forum.de/viewtopic.ph ... 63#p218263
http://www.python-forum.de/viewtopic.ph ... 91#p218191
Also ab jetzt bitte beachten

[nooby]

@Sirius und Hyperion: Vielen Dank für eure Anmerkungen.
@Hyperion Bezüglich der Namenswahl hast du natürlich recht. Der Mensch ist ein Gewohnheitstier, ich muss mir da mehr Mühe geben..
Bezüglich ``Pickler`` ein Auszug aus dem Galileo Openbook (Ja, ich weiss keine Gute Referenz )

Das Gegenstück zu Pickler ist Unpickler, um aus der übergebenen Datei die ursprünglichen Daten wiederherzustellen. Unpickler-Instanzen besitzen eine parameterlose Methode namens load, die jeweils das nächste Objekt aus der Datei liest.

Was ist den der Nachteil, wenn ich das ganze mit ``Pickler`` und im Binärformat mache?

Hab den Rest des Codes mal angepasst.
Ist es jetzt mehr Python-Like?

Code: Alles auswählen

import random
import pickle
import xml.dom.minidom
from xml.dom.minidom import parse
from colorama import init
from colorama import Fore, Back, Style


def save_statistics(filename, num_of_right_answers, num_of_wrong_answers):
    save = pickle.Pickler(open(filename, "wb"), 2)
    save.dump(num_of_right_answers)
    save.dump(num_of_wrong_answers)


def load_statistics(filename, num_of_right_answers, num_of_wrong_answers):
    save = pickle.Unpickler(open(filename, "rb"))
    num_of_right_answers = save.load()
    num_of_wrong_answers = save.load()
    return (num_of_right_answers, num_of_wrong_answers)


def get_verbs(filename):
    DOMTree = xml.dom.minidom.parse(filename)
    verb_list = DOMTree.documentElement
    verbs = verb_list.getElementsByTagName("verb")
    return verbs


def get_random_data(verbs, random_persons, random_tenses):
    random_verb = random.choice(verbs)
    random_person = random_persons[random.randint(0, 5)]
    random_tense = random_tenses[random.randint(0, 2)]
    return (random_verb, random_person, random_tense)


def get_answer(random_verb, random_tense, random_person):
    right_answer = random_verb.getElementsByTagName(random_tense)[0]
    right_answer = right_answer.getElementsByTagName(random_person)
    right_answer = right_answer[0].firstChild.nodeValue
    return right_answer


def check_and_print_answer(input_answer, right_answer, num_of_right_answers, num_of_wrong_answers):
    if input_answer.strip() == right_answer:
        num_of_right_answers += 1
        print(Fore.GREEN+"Richtig!")
        print()
    else:
        num_of_wrong_answers += 1
        print(Fore.CYAN+"Falsch.")
        print(Fore.CYAN+"Richtige Antwort wäre:")
        print(right_answer)
        print()
    return(num_of_right_answers, num_of_wrong_answers)


def print_verb_data(random_verb, random_tense, random_person):
    print(Fore.RED+"Verb: {0}".format(random_verb.getAttribute("deutsch")))
    print(Fore.RED+"Zeit: {0}".format(random_tense))
    print(Fore.RED+"Person: {0}".format(random_person))


def print_statistics(num_of_right_answers, num_of_wrong_answers):
    total_answers = num_of_right_answers+num_of_wrong_answers
    print()
    print()
    print(Fore.RED+"-------------------------Statistics-------------------------")
    print(Fore.RED+"            Richtige Antworten: {0}".format(num_of_right_answers))
    print(Fore.RED+"            Falsche Antworten: {0}".format(num_of_wrong_answers))
    print(Fore.RED+"            Totale Anzahl Antworten: {0}".format(total_answers))
    print(Fore.RED+"".join(["            ", str((100*num_of_right_answers)/total_answers) + "% Richtige Antworten."]))


def main():
    init()
    num_of_right_answers = 0
    num_of_wrong_answers = 0
    random_persons = ["je", "tu", "il", "nous", "vous", "ils"]
    random_tenses = ["présent", "imparfait", "futur"]
    statistics_path = "statisticsVerbs.bin"

    try:
        num_of_right_answers, num_of_wrong_answers = load_statistics(statistics_path, num_of_right_answers, num_of_wrong_answers)
    except IOError:
        pass
    for i in range(25):
        verbs = get_verbs("verbes.xml")
        random_verb, random_person, random_tense = get_random_data(verbs, random_persons, random_tenses)
        print_verb_data(random_verb, random_person, random_tense)
        right_answer = get_answer(random_verb, random_tense, random_person)
        input_answer = input()
        num_of_right_answers, num_of_wrong_answers = check_and_print_answer(input_answer, right_answer, num_of_right_answers, num_of_wrong_answers)
    print_statistics(num_of_right_answers, num_of_wrong_answers)
    save_statistics(statistics_path, num_of_right_answers, num_of_wrong_answers)


if __name__ == "__main__":
    main()

//Edit: Noch eine Frage. Kann ich die String Verknüpfungen so im ``print`` stehen lassen, oder ist die Variante mit ``.join`` besser oder gibt es eine viel elegantere Lösung?
//Edit2: Python style guide checker gibt mir an, nach jeder Funktion zwei Leerzeilen einzubauen, wird das tatsächlich so gemacht?

[Sirius3]

@nooby: die Dateien von load_statistics und save_statistics werden nicht wieder geschlossen. Das läßt sich am einfachsten per with-Konstruktion machen. Du benutzt immer noch minidom. Ein Umstieg auf ElementTree lohnt sich. Bei get_random_data sind die Namen der Parameter 2 und 3 falsch und Du benutzt aus irgendeinem Grund nur in Zeile 30 random.choice. Wie sieht denn Deine xml-Datei aus? Du kodierst doch nicht Variablen in Tag-Namen?
Hyperion hat Dich doch schon an .format verwiesen, wo hast Du also dieses schreckliche join her?
Ich sehe nicht was an den Variablen in Zeile 78 und 79 'random' sein sollte, am Besten schreibst Du Dir diese Listen aber auch direkt in Deine XML-Datei hinein, die übrigens nicht bei jedem Schleifendurchgang neu geladen werden muß.

[nooby]

Den Umstieg auf ElementTree programmier ich noch.
Die anderen Änderungen habe ich bereits umgesetzt.

Code: Alles auswählen

import random
import pickle
import xml.dom.minidom
from xml.dom.minidom import parse
from colorama import init
from colorama import Fore, Back, Style


def save_statistics(filename, num_of_right_answers, num_of_wrong_answers):
    with open(filename, "wb") as save:
        save = pickle.Pickler(save, 2)
        save.dump(num_of_right_answers)
        save.dump(num_of_wrong_answers)


def load_statistics(filename, num_of_right_answers, num_of_wrong_answers):
    with open(filename, "rb") as save:
        save = pickle.Unpickler(save)
        num_of_right_answers = save.load()
        num_of_wrong_answers = save.load()
    return (num_of_right_answers, num_of_wrong_answers)


def get_verbs(filename):
    DOMTree = xml.dom.minidom.parse(filename)
    verb_list = DOMTree.documentElement
    verbs = verb_list.getElementsByTagName("verb")
    return verbs


def get_random_data(verbs, random_persons, random_tenses):
    random_verb = random.choice(verbs)
    random_persons = random.choice(random_persons)
    random_tenses = random.choice(random_tenses)
    return (random_verb, random_persons, random_tenses)


def get_answer(random_verb, random_tense, random_person):
    right_answer = random_verb.getElementsByTagName(random_tense)[0]
    right_answer = right_answer.getElementsByTagName(random_person)
    right_answer = right_answer[0].firstChild.nodeValue
    return right_answer


def check_and_print_answer(input_answer, right_answer, num_of_right_answers, num_of_wrong_answers):
    if input_answer.strip() == right_answer:
        num_of_right_answers += 1
        print(Fore.GREEN+"Richtig!")
        print()
    else:
        num_of_wrong_answers += 1
        print(Fore.CYAN+"Falsch.")
        print(Fore.CYAN+"Richtige Antwort wäre:")
        print(right_answer)
        print()
    return(num_of_right_answers, num_of_wrong_answers)


def print_verb_data(random_verb, random_tense, random_person):
    print(Fore.RED+"Verb: {0}".format(random_verb.getAttribute("deutsch")))
    print(Fore.RED+"Zeit: {0}".format(random_tense))
    print(Fore.RED+"Person: {0}".format(random_person))


def print_statistics(num_of_right_answers, num_of_wrong_answers):
    total_answers = num_of_right_answers+num_of_wrong_answers
    print()
    print()
    print("{0}-------------------------Statistics-------------------------".format(Fore.RED))
    print("{0}            Richtige Antworten: {1}".format(Fore.RED, num_of_right_answers))
    print("{0}            Falsche Antworten: {1}".format(Fore.RED, num_of_wrong_answers))
    print("{0}            Totale Anzahl Antworten: {1}".format(Fore.RED, total_answers))
    print("{0}            {1} % Richtige Antworten.".format(Fore.RED, str((100*num_of_right_answers)/total_answers)))


def main():
    init()
    num_of_right_answers = 0
    num_of_wrong_answers = 0
    random_persons = ["je", "tu", "il", "nous", "vous", "ils"]
    random_tenses = ["présent", "imparfait", "futur"]
    statistics_path = "statisticsVerbs.bin"

    try:
        num_of_right_answers, num_of_wrong_answers = load_statistics(statistics_path, num_of_right_answers, num_of_wrong_answers)
    except IOError:
        pass

    verbs = get_verbs("verbes.xml")
    for i in range(25):
        random_verb, random_person, random_tense = get_random_data(verbs, random_persons, random_tenses)
        print_verb_data(random_verb, random_person, random_tense)
        right_answer = get_answer(random_verb, random_tense, random_person)
        input_answer = input()
        num_of_right_answers, num_of_wrong_answers = check_and_print_answer(input_answer, right_answer, num_of_right_answers, num_of_wrong_answers)
    print_statistics(num_of_right_answers, num_of_wrong_answers)
    save_statistics(statistics_path, num_of_right_answers, num_of_wrong_answers)


if __name__ == "__main__":
    main()

Sirius was meinst du genau mit

Du kodierst doch nicht Variablen in Tag-Namen?

Meine xml-Datei sieht wie folgt aus:

Code: Alles auswählen

<?xml version="1.0" encoding="utf-8"?>
<verbList>
	<verb deutsch="gehen">
		<infinitive>aller</infinitive>
		<présent>
			<indicatif>
				<je>vais</je>
				<tu>vas</tu>
				<il>va</il>
				<nous>allons</nous>
				<vous>allez</vous>
				<ils>vont</ils>
			</indicatif>
		</présent>
		<imparfait>
			<indicatif>
				<je>allais</je>
				<tu>allais</tu>
				<il>allait</il>
				<nous>allions</nous>
				<vous>alliez</vous>
				<ils>allaient</ils>
			</indicatif>
		</imparfait>
		<futur>
			<indicatif>
				<je>irai</je>
				<tu>iras</tu>
				<il>ira</il>
				<nous>irons</nous>
				<vous>irez</vous>
				<ils>iront</ils>
			</indicatif>
		</futur>
	</verb>
</verbList>

Wie würded ihr mir vorschlagen die beiden Listen (Personen,Zeiten) in die xml zu packen?

[Hyperion]

Ganz kurz noch mal generell etwas: Bei so langem Code mit (relativ) wenigen Änderungen lohnt es sich, diesen bei gist.github.com zu posten. Da hast Du sogar eine Versionskontrolle dabie, d.h. Du kannst für daselbe Gist mehrere Versionen Schritt für Schritt hinzufügen

Hier im Forum wird es schnell unübersichtlich, wenn man bei jedem zweiten Posting 100 Code-Zeilen durchscrollen muss

[BlackJack]

@nooby: `pickle` ist ein Format das Python-spezifisch ist. Es gibt deutlich universellere Formate um einfache Daten und Datenstrukturen zu speichern die nur aus den Grunddatentypen von Python bestehen. JSON zum Beispiel, oder in diesem Fall sogar eine CSV-Datei oder gar eine ”normale” Textdatei mit den zwei Zahlen für die Statistik. Für JSON oder CSV gibt es jeweils ein Modul in der Standardbibliothek.

Das „right” bei den richtigen Antworten würde ich vom Sprachgefühl eher als „correct” bezeichnen.

Ist das XML-Format von Dir entworfen? Warum ist das deutsche Wort in einem Attribut gespeichert, alles andere aber als Text in Elementen?

@Hyperion: Das Problem bei Gists ist das bisher soweit ich das sehe die Leute immer einen direkten Link zum Gist gepostet haben, also zur aktuellsten Version, und nicht explizit zu der Version auf die sich der Beitrag bezieht. Zum Zeitpunkt der Veröffentlichung des Beitrags ist das ja noch das selbe, aber am Ende hat man halt ein Thema mit mehreren Beiträgen mit immer dem selben Link und man kann nicht mehr (einfach) nachvollziehen auf welchen Code sich die Antworten mit den Verbesserungsvorschlägen denn eigentlich beziehen. Das ist blöd für Leute die das Thema erst später finden/lesen.

[nooby]

@BlackJack: Ja, was du sagst klingt logisch. Das ``Pickle`` ist stark übertrieben für das Speichern von zwei Zahlen..
Ja, das XML-Format ist von mir.
Da es für jedes Verb ja nur eine Übersetzung gibt, dachte ich, dass kann man gut als Attribut speichern.
Wäre es anders schöner?
Und wie soll ich in Zukunft den Code Posten?
Als Gists odr im Forum Selber?
Gibt es eine Möglichkeit den Code zu "spoilern"?
Also so, dass er manuell aufgeklappt werden muss, dass würde das Scrollen vereinfachen...

[BlackJack]

@nooby: Es gibt ja auch nur einen Infinitiv und jedes Subelement in <indicatif> hat auch nur einen Wert. Da hast Du Dich nicht entschieden Attribute zu verwenden.

Ebenfalls aus dem Rahmen fällt der Attributname, der im Gegensatz zu den ganzen Tagnamen nicht auf französisch ist.

Die übliche ”Trennlinie” zwischen Tags und Attributen ist ob es sich um ”Daten” oder um ”Metadaten” handelt. Wobei das nicht immer so eindeutig trennbar ist, es also Fälle gibt, die beides sein könnten, oder wo zwei Programmierer anderer Auffassung sind in welche Kategorie etwas gehört. Wenn man nicht genau weiss was es ist, nimmt man eher ein Element.

[nooby]

Okay
Vielen Dank für eure vielen Anregungen und Verbesserungsvorschläge.
Ich hoffe ich kann später mal anderen Einsteigern etwas zurückgeben