Programm zur Rechnungsverwaltung

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
remboven
User
Beiträge: 7
Registriert: Samstag 1. Januar 2011, 16:02

Hallo zusammen.
Ich habe in den letzten Tagen mal ein kleines Programm zur Rechnungsverwaltung für einen Einmannbetrieb geschrieben.
Mit dem Programm kann man eine Kundendatei und (mithilfe von Latex) Rechnungen erstellen. Außerdem kann man mit dem Programm seine Rechnungen verwalten. Ich weiß, dass noch nicht alles perfekt ist, vieles könnte man wahrscheinlich noch flexibler und kürzer gestalten. Ich wollte euch das Programm trotzdem schon einmal zeigen und um Anregungen und Kritik bitten: http://paste.pocoo.org/show/313312/
BlackJack

@remboven: Importe gehören normalerweise an den Anfang eines Moduls. In jeder Funktion Module zu importieren ist unübersichtlich. Man kann dann nicht mal eben schnell schauen von welchen anderen Modulen das Modul abhängt.

Programmlogik und Benutzerinteraktion ist überhaupt nicht getrennt. Man könnte von dem Programm so zum Beispiel nichts wiederverwenden, wenn man eine GUI für die Rechnungsverwaltung schreiben möchte.

``return`` und ``except`` sind keine Funktionen, die sollte man also auch nicht wie welche hinschreiben. Also ohne Klammern.

`select_customer()` ist deutlich zu kompliziert. Ein Dictionary mit fortlaufenden Zahlen als Schlüssel ist eigentlich eine Liste. `keys` wird nicht verwendet, also könnte man die erste Zeile einfach vor der Schleife überlesen, statt mit dem manuell hochgezählten `x` zu arbeiten. Wenn man einen Index neben den eigentlichen Elementen eines "Iterable" benötigt, bietet sich die `enumerate()`-Funktion an.

Dateien die man öffnet, sollte man auch explizit wieder schliessen, oder die ``with``-Anweisung dafür verwenden.

Die `select_customer()`-Funktion könnte so aussehen (ungetestet und immer noch Logik und Benutzerinteraktion gemischt):

Code: Alles auswählen

def select_customer(filename):
    with open(filename) as reader:
        next(reader)    # Skip keys.
        customers = list(reader)
    for i, customer in customers:
        print('[%d] %s' % (i, customer)
    return customers[int(input("Welchen Kunden möchten Sie auswählen? "))]
Du scheinst das Programm nicht getestet zu haben, denn in `add_entry()` wird eine Funktion `reader()` aufgerufen, die nirgends definiert ist!?

Endlosrekursion um das Hauptprogramm erneut aufzurufen ist keine gute Idee, weil man damit ein Speicherleck hat und das Programm mit einer Aussnahme aussteigt, wenn das Rekursionslimit erreicht ist, oder bei anderen Implementierungen als CPython vielleicht sogar hart abstürzt, wenn der Stapelspeicher "voll" ist und zum Beispiel anfängt den Datenbereich des Programms zu überschreiben.

Die erste Zuweisung an `travel_cost` in `create_bill()` wird nirgends verwendet. Die Unterscheidung ob `duration` gleich 1 ist, kannst Du Dir sparen. Beim ``else``-Teil würde nämlich das selbe heraus kommen. Beim zweiten mal ist der ``if``-Teil überflüssig, denn die Zeile für die erste Stunde hängt ja gar nicht von der `duration` ab, die kommt in beiden Zweigen vor. Und damit kann sie ohne Bedingung davor gezogen werden. Die zweite Zeile hängt davon ab ob `duration` grösser als 1 ist.

Das Runden auf zwei Stellen mit `round()` funktioniert so nicht zuverlässig und macht bei Python 3.x vielleicht auch nicht das, was Du erwartest. Da sollte man eventuell mit dem `decimal`-Modul arbeiten.

Die ganzen `str()`-Aufrufe beim Formatieren der Zahlen sind überflüssig. Der ``%``-Operator liefert hier ja schon ein Objekt vom Typ `str`.

Die vielen `bill.replace()`-Aufrufe sollte man durch ein Template-System ersetzen. `string.Template` aus der Standardbibliothek bietet sich an, wenn man keine externe Abhängigkeit haben möchte.

`mark_as_payed()` dürfte Probleme bekommen, wenn es für einen Kunden mehr als eine Rechnung gibt.
remboven
User
Beiträge: 7
Registriert: Samstag 1. Januar 2011, 16:02

@blackjack: Vielen Dank erst einmal für die ausführliche Rückmeldung. Ich habe mal so viel wie gerade möglich davon umgesetzt. Einiges war mir neu, anderes nur dumme Fehler von mir.
Ich schreibe einmal chronologisch auf, was ich geändert habe. Zu manchen deiner Punkte habe ich auch noch Fragen.

Ich muss zugeben, dass ich nicht weiß, wie ich Programmlogik und Benutzerinteraktion voneinander trenne, habe ich bisher noch nie gemacht. Kannst du mir da vielleicht ein Beispiel oder einen Link geben?

Die return und except statetments(?) habe ich jetzt geändert.

select_customer habe ich auch geändert. Es ist ein wenig länger als deine Variante, für mich aber deutlich verständlicher. Was mir nicht klar ist, ist wie ich hier die Datei schließe. Muss man das bei csv Dateien überhaupt tun? In der Dokumentation habe ich dazu nichts gesehen.

Die fehlende Funktion in add_entry() lag da noch vom Beginn des Programmierens rum. Diese Funktion hatte ich tatsächlich lange nicht mehr getestet, mit der neuen return_keys()-Funktion klappt es nun aber.

Hast du einen Vorschlag wie ich meine Endlosrekursion im Hauptprogramm ersetzen könnte ohne das bei einem Fehler gleich das ganze Programm abstürzt?

travel_cost wird nun sinnvoller genutzt. Die duration checks habe ich jetzt auch entfernt. Du hattest komplett recht, ist einfacher so.

Das decimal-Modul muss ich mir noch mal angucken, das mir jetzt auf die Schnelle noch nicht so eindeutig.

Die str()-Aufrufe sind nun auch entfernt.

Das meine bill.replace()-Aufrufe nicht gerade schön sind dachte ich mir schon, wusste aber nicht, wie ich besser vorgehen sollte. Würdest du mir empfehlen string.Template zu benutzen, oder sollte ich mir ein anderes Templatesystem angucken? Die externen Abhängigkeiten sind mir nicht soo wichtig.

In mark_as_payed() wurde die Rechnungsnummer nicht der Name verglichen um genau diesem Problem zu entgehen. Die Variabel war wohl nur etwas ungünstig benannt, ich habe das jetzt mal geändert.

Hier ist nun die neue Version:
http://paste.pocoo.org/show/313426/
Benutzeravatar
cofi
Python-Forum Veteran
Beiträge: 4432
Registriert: Sonntag 30. März 2008, 04:16
Wohnort: RGFybXN0YWR0

remboven hat geschrieben:Hast du einen Vorschlag wie ich meine Endlosrekursion im Hauptprogramm ersetzen könnte ohne das bei einem Fehler gleich das ganze Programm abstürzt?
http://paste.pocoo.org/show/313442/
Das `continue` in der `except` Suite duerfte dabei ueberfluessig sein, aber ich finde es dokumentiert ganz gut den Zweck.

Als Template-Engine kann ich Jinja2 sehr empfehlen, wenn dir die Abhaengigkeiten egal sind. Aber unabhaengig davon sollten Formatstrings bzw. `string.Template` schon genuegen.
Und statt

Code: Alles auswählen

master = open("test.tex", "r")
bill = master.read()
master.close()
das hier, wie BlackJack schon angedeutet hatte:

Code: Alles auswählen

with open("test.tex", "r") as master:
    bill = master.read()
BlackJack

@remboven: Beispiel oder Link zum Trennen von Programmlogik und Benutzerinteraktion kann ich Dir nicht geben, das ist aber im Grunde auch viel zu einfach, als dass man dafür ein extra Tutorial oder so schreiben müsste. Die Teile die etwas für den Benutzer ausgeben oder Eingaben von ihm entgegennehmen, also in diesem Falle `print()` und `input()` sollten nicht in den gleichen Funktionen stecken, welche die Arbeit verrichten. Überleg Dir einfach was das Programm an Funktionalität benötigt und schreibe dafür Funktionen. Und dann Funktionen, die mit dem Benutzer interagieren und die Funktionen mit der Programmlogik aufrufen. Dann kann man die Funktionen einfacher automatisiert testen und wiederverwenden. Zum Beispiel wenn man eine GUI dafür schreiben möchte, oder vielleicht eine Importfunktion für Rechnungen in einem anderen Format. Der wiederholte Ausdruck einer bereits versandten Rechnung wäre sicher auch interessant, falls jemand nicht bezahlt oder sich meldet, dass er die Rechnung verlegt hat, oder so. Zumindest der wiederholte Rechnungsversandt oder dann auch Mahnungen sollten ja zum Beispiel ohne weitere Eingaben des Benutzers funktionieren, würden aber einen guten Teil von `create_bill()` benötigen.

Genau, ``return`` und ``except`` sind "Statements", oder auf deutsch auch "Anweisungen".

Schliessen von Dateien sollte man bei jeder Art von Datei tun. Für das `open()` sind CSV-Dateien ja nichts anderes als andere Dateien auch. Dateien werden mit der `close()`-Methode geschlossen, das sollte auch in der Dokumentation stehen. Oder eben mit der ``with``-Anweisung. Dann wird die Datei geschlossen, wenn der Programmfluss den ``with``-Block verlässt -- und zwar egal aus welchem Grund. Wenn man das mit `close()` macht, muss man das mit ``try``/``finally`` machen, damit die Datei auch in jedem Fall geschlossen wird.

Die "magischen" Methoden mit den beiden führenden und nachfolgenden Unterstrichen sollte man übrigens nicht direkt aufrufen. Wenn es zu der Methode eine Funktion gibt, ist die zu bevorzugen. Also ``next(iterator)`` statt ``iterator.__next__()``.

Man sollte wenn möglich Wiederholungen von Daten und Code vermeiden. Zum Beispiel müsstest Du das Programm an mehreren Stellen ändern, wenn Du die Dateinamen ändern möchtest. Bei `add_entry()` könnte man den konkreten Dateinamen zum Beispiel durch ein Argument ersetzen.

Die Endlosrekursion kann man doch ganz einfach durch eine Endlosschleife ersetzen. Einfach ``while True`` um den Inhalt der Funktion und das ``main()`` aus dem ``except``-Block entfernen.

Ich würde wahrscheinlich ein anderes Template-System als `string.Template` verwenden, aber hauptsächlich auch nur, weil ich für andere Aufgaben schon etwas externes verwende. Für diese Aufgabe solle `string.Template` aber ausreichend sein.
remboven
User
Beiträge: 7
Registriert: Samstag 1. Januar 2011, 16:02

So, ich habe noch mal möglichst viele eurer Kritikpunkte umgesetzt:
http://paste.pocoo.org/show/314869/
Bei select_customer und return_customer macht das Programm jetzt ja sehr ähnlich Dinge, ich habe aber versucht Benutzereingabe und Programmlogik zu trennen. Ist das so sinnvoll?
Wie kann ich der with Anweisung sagen, dass der delimiter in meiner csv-Datei ein Tab ist?

Code: Alles auswählen

 with csv.reader(open(csvFile), delimiter="\t") as newFile
funktioniert nicht.
Habt ihr noch andere Anmerkungen/Tipps?
Benutzeravatar
cofi
Python-Forum Veteran
Beiträge: 4432
Registriert: Sonntag 30. März 2008, 04:16
Wohnort: RGFybXN0YWR0

remboven hat geschrieben:

Code: Alles auswählen

 with csv.reader(open(csvFile), delimiter="\t") as newFile
funktioniert nicht.
Natuerlich nicht. Die Fehlermeldung sagt aber auch, dass der Fehler woanders liegt: `csv.reader` liefert eben keinen Contextmanager zurueck, genauer, hat kein `__exit__` Attribut.

So sollte es aussehen:

Code: Alles auswählen

with open(csv_fname) as in_file:
    reader = csv.reader(in_file, delimiter='\t')
remboven
User
Beiträge: 7
Registriert: Samstag 1. Januar 2011, 16:02

@confi: Danke für die schnelle Antwort. Dass es ungefähr so aussehen muss habe ich mir schon gedacht, habe es aber nicht zum Laufen bekommen. So wie du es geschrieben hast klappt es jetzt aber sehr gut (und macht auch Sinn)
Korrigierte Version: http://paste.pocoo.org/show/314895/
Weitere Ratschläge erwünscht ;)
quant
User
Beiträge: 34
Registriert: Freitag 29. Mai 2009, 12:06

naja du könntest das jetzt noch alles in eine klasse verpacken ;)
Benutzeravatar
cofi
Python-Forum Veteran
Beiträge: 4432
Registriert: Sonntag 30. März 2008, 04:16
Wohnort: RGFybXN0YWR0

Und eine unnoetige Klasse ist besser, weil?

Unschoen:

Code: Alles auswählen

# Change the sums to the German format (, instead of .) and change the number to two decimals
travel_cost =("%.2f" % travel_cost).replace(".", ",")
Warum sich unnoetig eingrenzen? Dazu gibt es `locale.format`

Code: Alles auswählen

locale.setlocale(LC_ALL, '') # am anfang
travel_cost =locale.format("%.2f", travel_cost)
Falls die locale wie bei mir nicht 'de_DE.*' sein sollte kann man das immernoch festlegen"

Code: Alles auswählen

>>> locale.setlocale(locale.LC_ALL, 'de_DE.UTF-8')
'de_DE.UTF-8'
>>> locale.format('%.2f', 0.2)
'0,20'
Sehe ich das richtig, dass in `return_customer` eine unnoetige Liste gebaut wird, nur um dann einen Eintrag auszugeben?
Wie waere es mit dem hier:

Code: Alles auswählen

def return_customer(csv_file, customer):
    with open(csvFile) as in_file:
        entry_reader = csv.DictReader(in_file, delimiter="\t")
        customers = []
        for i, row in enumerate(entry_reader):
            if i == customer:
                return row
Wo ich

Code: Alles auswählen

tax = netto * 0.19
    tax = round(tax, 2)
sehe. Weil es hier um Geld geht solltest du besser auf das decimal Modul zurueckgreifen, statt zu hoffen dass die Floats einigermassen passen.
remboven
User
Beiträge: 7
Registriert: Samstag 1. Januar 2011, 16:02

Danke für den den Tipp mit den Locals, das war mir noch nicht bekannt, ist ja aber sehr praktisch :)
return_customer habe ich auch überarbeitet und das Decimal Modul habe ich jetzt (hoffentlich) auch verstanden:
http://paste.pocoo.org/show/315329/
BlackJack

@remboven: `select_costumer()` und `return_costumer()` sind auch so noch sehr unschön weil unnötig oft die Datei eingelesen wird. Lies die Datei *einmal* ein und frage den Benutzer dann welchen Eintrag davon er haben möchte. Im Grunde wie vorher, nur halt nicht in einer Funktion.
remboven
User
Beiträge: 7
Registriert: Samstag 1. Januar 2011, 16:02

Ich habe den Code jetzt mal mit pylint gecheckt und an einigen Stellen ein wenig verändert. Ich verstehe, warum die Zeilen nicht über 80 Zeichen haben sollten. Sollte man das dann mit langen strings so machen wie ich in add_entry() oder gibt es da schönere Varianten?
@BlackJack: Ich habe jetzt mal versucht die Datei nur einmal einzulesen, die return_customer() Funktion liefert allerdings nur noch None zurück. Könnte das daran liegen, dass die Datei quasi "leergelesen" wird? Oder habe ich da einen Denkfehler?
Hier mal die optimierte (aber nicht mehr funktionierende) Version:
http://paste.pocoo.org/show/316665/
Benutzeravatar
cofi
Python-Forum Veteran
Beiträge: 4432
Registriert: Sonntag 30. März 2008, 04:16
Wohnort: RGFybXN0YWR0

remboven hat geschrieben:Ich verstehe, warum die Zeilen nicht über 80 Zeichen haben sollten. Sollte man das dann mit langen strings so machen wie ich in add_entry() oder gibt es da schönere Varianten
Die Variante hat einen grossen Nachteil: Das hat mehr Whitespace als man eigentlich will. Gegenmittel: textwrap.dedent
Andere Moeglichkeiten sind natuerlich Konkatenation per `+` die implizite Konkatenation von Stringliteralen ("a" "b" = "ab") und natuerlich die Strings auszulagern.

Aber das ist sehr von der Situation abhaengig.

Zu deinem Dateiproblem: Man kann nur einmal ueber die Datei iterieren, dann ist man am Ende. An den Anfang (oder andere Stellen) kommt man mit `file.seek`. Aber BlackJack meinte, dass du die Datei _einlesen_ sollst, nicht das Dateiobjekt herumreichen. Letzteres hat naemlich viele subtile Probleme, z.b. wer schliesst die Datei?
remboven
User
Beiträge: 7
Registriert: Samstag 1. Januar 2011, 16:02

Ah, okay jetzt habe ich es glaube ich verstanden. So funktioniert es jetzt auch.
Den zu langen Zeilen widme ich mich dann später unter Verwendung der genannten Möglichkeiten noch mal. So wichtig ist das ja auch nicht.
http://paste.pocoo.org/show/316716/
Antworten