Ist mein Programmierstil "pythonisch" genug?

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
dansch
User
Beiträge: 6
Registriert: Dienstag 4. September 2007, 21:19

Hallo,

ich beschäftige mich erst seit kurzem mit Python und arbeite mich gerade durch die Bücher "Core Python" und "wxPython in Action" durch. Hobbymäßig programmier ich schon eine Weile und habe Erfahrungen in Java und Perl.

Was ich bisher von Python gesehen habe, begeistert mich. Beim Durcharbeiten des Buches CorePython wurde eine Aufgabe gestellt, die ich mit dem beigefügten Quelltext gelöst habe.

Meine Frage ist nun:

Ist mein Quelltext "pythonisch" genug?

Oder sieht man meine Perl-Wurzeln? Auch über Tipps, was man anders oder eleganter machen könnte sind willkommen.

Aufgabe:
Schreibe ein simples Programm mit einem Menu mit den folgenden Punkten:

a) Dateinamen erfragen, nach Dateiinhalt zeilenweise fragen, mit Zeile mit . beendet Eingabe, speichern der Datei

b) Dateinamen erfragen, Inhalt der Datei anzeigen

c) Dateinamen erfragen, Inhalt editieren lassen, fragen, ob Änderungen gespeichert werden sollen, Sicherungskopie anlegen

Meine Lösung:
Originalform: Ausgelagert
Überarbeitete Form: Ausgelagert (inkl. Eurer Vorschläge)

Der Quelltext ist zwar lang. Aber so kann man vielleicht typische Fehler eher erkennen.

Freue mich jetzt schon über Eure Kritik.

Grüße,
Daniel

Edit (Leonidas): Code ausgelagert.
Edit(dansch): geänderten Code hinzugefügt
Zuletzt geändert von dansch am Freitag 7. September 2007, 16:19, insgesamt 2-mal geändert.
"Just because I don't care - doesn't mean I don't understand."
Homer J. Simpson
Leonidas
Python-Forum Veteran
Beiträge: 16025
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Hallo dansch, willkommen im Forum,

ich fange erst mal mit der Kritik an, du musst dir einfach denken, dass alles was ich nicht kritisiere so ziemlich in Ordnung ist.

Fangen wir mal oben an: ``os.linesep`` zuzuweisen macht in der Regel wenig bis gar keinen Sinn, da man es nur selten braucht und es dann an einen nichtssagenden Namen wie ``ls`` zu binden ist keine schlaue Idee. Man kann mit ``from os import linesep`` den Namen importieren, aber auch das halte ich für eher unnütz weil man sich so nur das Tippen von ``os.`` spart was man dann aber wieder mit einem Namen bezahlt von dem man nicht sofort sieht wo er her kommt.

Du benennst deine Funktionen in ``camelCase``, der Python-Styleguide schlägt aber ``names_with_uderscores`` vor.

Einen Namen ``file`` zu benennen, weil er so das Builtin ``file`` überschriebt, was zwar nicht kritisch ist, aber in einigen Fällen durchaus für Verwirrung sorgen kann. Also besser einen anderen Namen finden.

Die Prüfung auf Vorhandensein kann man so machen, aber man könnte sie auch in die Exception-Behandlung rein packen.

Das ``fobj.writelines(["%s%s" % (x, ls) for x in content])`` könnteman auch als ``fobj.writelines("%s%s" % (x, os.linesep) for x in content)`` schreiben, was aus der LC einen Generator macht, der etwas Speichersparender ist. Insgesamt brauchst du ``linesep`` nicht, denn ``\n`` wird immer "richtig" abgespeichert.

Dann solltest du noch den Code auf Modulebene in eine ``main()``-Funktion auslagern, die du dann via ``if __name__ == '__main__'\n main()`` aufrufst.

Wenn du willst, kann ich dir mal ein solches Programm schrieben, wie ich es gemacht hätte, damit du es vergleichen kannst.

Eine detailiertere Analyse wird warscheinlich gleich von BlackJack kommen ;)
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
Benutzeravatar
gerold
Python-Forum Veteran
Beiträge: 5555
Registriert: Samstag 28. Februar 2004, 22:04
Wohnort: Oberhofen im Inntal (Tirol)
Kontaktdaten:

dansch hat geschrieben:Beim Durcharbeiten des Buches CorePython wurde eine Aufgabe gestellt, die ich mit dem beigefügten Quelltext gelöst habe.
Hallo dansch!

Willkommen im Python-Forum!

Keiner ist perfekt vom Himmel gefallen. Leonidas hat dir ja schon ein paar wichtige Tipps gegeben. Aber dafür, dass du noch nicht alles wissen kannst weil du ja erst mit Python anfängst, sieht dein Quellcode gar nicht mal so schlecht aus. 8)

mfg
Gerold
:-)
http://halvar.at | Kleiner Bascom AVR Kurs
Wissen hat eine wunderbare Eigenschaft: Es verdoppelt sich, wenn man es teilt.
lunar

Zusätzlich zu Leonidas Ausführungen:

Du schließt Dateiobjekt entweder gar nicht (z.B. Z. 21ff oder 48ff), oder inkorrekt (Z. 114ff). Schlägt writelines fehl, bleibt das Dateiobjekt offen.
Wenn du Python 2.5 verwendest, solltest du dir das with-Statement anschauen. Ansonsten musst du mit verschachtelten try-except-finally Statements arbeiten.

Statt die Backup-Datei zu öffnen und den Inhalt der Datei händisch zu schreiben, könntest du copy aus dem shutil Modul verwenden, was das Backup zu einem Einzeiler machen würde.

Dann solltest du beim Anzeigen des Menüs eine Fehlermeldung auswerfen, wenn eine ungültige Auswahl getroffen wurde. Gegenwärtig ignorierst du falsche Eingabe stillschweigend (Z. 139). Besser so:

Code: Alles auswählen

try:
     CMDs[choice]()
except KeyError:
     print 'Ungültige Auswahl'
Zumindest aber der Aufruf der Methode keys() (Z. 139) auf dem dict-Objekt ist unnötig, der Operator "in" kann auch direkt auf das dict angewandt werden, um auf die Existenz eines Schlüssels zu testen.

Außerdem könntest du dir die relativ unnötige Konvertierung in int sparen, wenn du die Auswahl-Indizes direkt als String im dict ablegen würdest. Ansonsten könntest du das dict nämlich auch getrost durch ein Tupel ersetzen...

Dann könnte man die Fehlermeldungen auch vereinheitlichen, in dem nur eine ungültige Eingabe gemeldet wird, und nicht extra noch ein Fehler, dass die Eingabe eine Nummer sein muss:

Code: Alles auswählen

try:
    CMDs[raw_input("What's your choice: ")).lower()]()
    break
except KeyError:
    print 'invalid input'
Ansonsten Zustimmung, der Quelltext sieht schon ganz ordentlich aus...
EyDu
User
Beiträge: 4881
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Zwei Dinge habe ich noch die ich für etwas unglücklich halte:

1) Zur Realisierung der Programmlogik verwendest du sehr oft "return" und "break". Mit diesen Sprügen werden die Funktionen unter Umständen schon sehr früh verlassen und der Programmfluss ist teilweise nur schwer nachzuvollziehen. Daher würde ich zu einem if-else-Konstrukt tendieren, ist aber wohl teilweise auch Geschmackssache.

Code: Alles auswählen

def readTextFile():
    file = raw_input("Please enter a filename to read from: ")
    if not os.path.exists(file):
        print err, " ", file, " doesn't exists."
    else:
        try:
            fobj = open(file, 'r')
            print fobj.read()
        except IOError, e:
            print err, " I/O-Error: ", e
Auch die while-Schleifen ließen sich leserlicher gesalten, indem man die Bedinung auch an die entsprechende Stelle schreibt und dies nicht hinter einem "break" verbirgt. Hier mal am Beispiel eines Codeausschnittes der Funktion "makeTextFile":

Code: Alles auswählen

line = None
while line != ".":
        line = raw_input(">> ")
        content.append(line)
2) Das ist ein kurzer Punkt: Die Methode "makeTextFile" ist viel zu lang (86 Zeilen). Ich bin mir ziehmlich sicher, dass man diese auch noch sinnvoll aufteilen kann.
dansch
User
Beiträge: 6
Registriert: Dienstag 4. September 2007, 21:19

Hallo,

danke für Euer Feedback. Einige Änderungen habe ich bereits in das Skript eingearbeitet. Das geänderte Skript findet Ihr als Link unter dem Originalskript in meinem Ausgangspostings.

Je mehr ich mich an Python gewöhne, muss ich sagen, es gefällt mir immer besser. Werde wohl nicht mehr zu Perl zurückfallen. 8)

@Leonidas:
Danke für das Auslagern. Habe mein überarbeitetes Skript auch ausgelagert. Ob ich mich an this_is_a_function() gewöhnen kann, weiß ich noch nicht. Sowohl im CorePython- als auch dem wxPython-Buch wird camelCase für Funktionsnamen benutzt.

Anstelle von os.linesep reicht also auch die Verwendung von '\n'?

Das Angebot, dass Du auch so ein Programm schreibst und postet, würde ich gerne annehmen. :-)

@Gerold
Danke für das Lob.

@lunar
Ich habe jetzt einige fobj.close() hinzugefügt. Ich gehe aber davon aus, dass wenn ich das Programm mit sys.exit() verlasse, das dann Python für mich alle Filehandles schließt. Damit sollte das finally wahrscheinlich überflüssig sein. Aber soweit habe ich mich in dem Buch CorePython noch nicht eingelesen. ;-)

Das es für das Kopieren ein Modul gibt, hätte ich mir denken können. Wollte das Problem aber mit Bordmitteln lösen und um ein Gefühl für Python zu bekommen.

Der Tipp mit CMDs[raw_input("What's your choice: ")).lower()]() finde ich super. Habe ich auch gleich eingebaut.

@EyDu
Habe jetzt alle return und break eleminiert. :-) Ist etwas übersichtlicher geworden. Die Fehlerbehandlung wurde in eine eigene Funktion fail(msg) ausgelagert.

Die Funktion editTextFile() habe ich auch etwas gestrafft. U.a. wurde der Backupteil in eine eigene Funktion ausgelagert. Wahrscheinlich ist aber immer noch etwas Optimierungsbedarf.
"Just because I don't care - doesn't mean I don't understand."
Homer J. Simpson
EyDu
User
Beiträge: 4881
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Sieht doch eigentlich schon recht vernüftig aus, aber bei deinen Änderungen ist noch eine Kleinigkeit hinzugekommen. Der folgende Code (von dir) mach nicht ganz das was du erwartest:

Code: Alles auswählen

try:
    fobj = open(textfile, "w")
    fobj.writelines("%s%s" % (x, os.linesep ) for x in content)
    fobj.close()
except IOError, e:
    fail("I/O-Error %s" % e)
Sollte hier bei "wirtelines" ein Fehler auftreten, so wird die Datei nicht richtig geschlossen, da das "close" bei einer Ausnahme übersprungen wird. Was du möchtest entspricht wohl eher dem Folgendem:

Code: Alles auswählen

try:
    fobj = open(textfile, "w")
except IOError, e:
    fail("I/O-Error: could not open file %s" % e)
else:
    try:
        fobj.writelines("%s%s" % (x, os.linesep ) for x in content)
    except IOError, e:
        fail("I/O-Error: could not wirte all lines %s" % e)
        fobj.close()
So wird die Datei immer korrekt geschlossen, oder wenn sich nicht geöffnet wird, das Schließen übersprungen.

Je nach dem ob du das Script nur zum Python üben verwendest, oder mehr Wert auf die Programmierung legst, wäre eine mögliche Erweiterung noch die Trennung der Logik und der Ausgabe der Meldungen. Wenn man das vernünftig macht, kann man das Modul schön auslagern und dann Erweiterungen für beispielsweise wxPython schreiben, ohne dass die Logik angefasst werden müsste.

Edit: e's eingefügt.
Zuletzt geändert von EyDu am Samstag 8. September 2007, 11:49, insgesamt 2-mal geändert.
schlangenbeschwörer
User
Beiträge: 419
Registriert: Sonntag 3. September 2006, 15:11
Wohnort: in den weiten von NRW
Kontaktdaten:

Da fehlen jetzt aber die ", e"s hinter den "IOError"s.
BlackJack

Nun geb ich auch noch meinen Senf dazu.

Das übliche: Zeilen die länger als 80 Zeichen sind. Namenskonvention wurde ja schon angesprochen.

Doctstrings werden per Konvention in dreifache *doppelte* Anführungszeichen gesetzt. `pydoc` hatte da sogar mal ein Problem mit, wenn es nur einfache waren.

Das `seek()` in der Backup-Funktion würde ich mir eventuell sparen und es einfach als Voraussetzung annehmen.

Wenn Du schon nicht das `shutil`-Modul aus der Standardbibliothek zum kopieren verwenden willst, könntest Du wenigstens mit der `writelines()`-Methode die ``for``-Schleife einsparen:

Code: Alles auswählen

        fobj_backup.writelines(fobj)
Allerdings wäre kopieren in grösseren Blöcken, z.B. 8 oder 16 KiB sicher effizienter:

Code: Alles auswählen

        for block in iter(lambda: fobj.read(8129), ''):
            fobj_backup.write(block)
Auf das Vorhandensein der Datei in `readTextFile()` kannst Du verzichten, der Fall wird auch von der Ausnahmebehandlung abgedeckt.

Bei der Schleife zum einlesen des Textes vom Benutzer würde ich jetzt genau das Gegenteil von EyDu empfehlen: Das ist eine DO...UNTIL-Schleife und das Python-Idiom dafür ist eine "Endlosschleife" mit einem ``break``.

Wenn die Datei in `editTextFile()` sowieso komplett in den Speicher geladen wird, kann man sich die Backup-Funktion sparen bzw. dort das lesen der alten Datei. Jetzt wird diese Datei ja zweimal gelesen. Damit würde man auch die `seek()`\s los.

So kurze Namen wie `ec`, wo beim ersten auftreten als Kommentar ``edit counter`` dabei steht, kann man auch gleich `edit_counter` nennen. Dann spart man sich den Kommentar und weiss auch gleich an *allen* Stellen was gemeint ist, ohne den Kommentar zu suchen.

Das Menü kann man etwas übersichtlicher im Quelltext formatieren. Zeichenketten die nur durch "whitespace", also Leerzeichen und Zeilenumbrüche, getrennt sind, werden vom Compiler beim übersetzen zu einer Zeichenkette zusammengesetzt:

Code: Alles auswählen

    while True:
        print ('(1) read a text textfile\n'
               '(2) make a text textfile\n'
               '(3) edit a text textfile\n'
               '(4) quit')
Ausprobiert hast Du das Programm anscheinend nicht: Der Name `ls` in `editTextFile()` wird nirgends definiert und bei `makeBackupFile()` wird die Datei *in* der Schleife geschlossen in die beim nächsten Schleifendurchlauf wieder geschrieben werden soll. Das sollte beides mit einer Ausnahme quittiert werden.

In der `main()` wird der Name `e` in der Ausnahmebehandlung gar nicht benötigt.
lunar

@dansch

Man sollte Dateiobjekt immer explizit schließen. Und was shutil angeht: Module, die in der Standardbibliothek sind, sind "Bordmittel" und natürlich zu verwenden! Wer sie nicht verwendet, ist selbst schuld :roll:

Btw, die Aufgabe, eine Datei zu bearbeiten, hat mich auch mal gereizt. Meine Lösung unter Ausschöpfung aller Möglichkeiten, die das readline-Modul so bietet: http://paste.pocoo.org/show/3513
Dazu gehört Autovervollständigen von Dateinamen, sowie das direkte Editieren von Zeilen.
dansch
User
Beiträge: 6
Registriert: Dienstag 4. September 2007, 21:19

Danke für Eure Tipps.

Es hat mir geholfen, dass Schreiben von Python-Code mehr zu verstehen. :-)
"Just because I don't care - doesn't mean I don't understand."
Homer J. Simpson
Antworten