Übungsprojekt meinAdressbuch

[ltownatze]

Ich habe vor ca 2 Wochen angefangen Python zu lehrnen. Habe noch keine Programmiererfahrung.
In Anlehnung an diese Aufgabe

Schreiben Sie Ihr
eigenes kommandozeilenbasiertes Adressbuch-Programm, mit dem Sie Ihre Kontaktadressen wie Freunde, Familie, Kollegen, und die zugehörigen Informationen wie E-Mail-Adresse und/oder Tele-
fonnummer hinzufügen, verändern, löschen oder durchsuchen können. Die Daten müssen für den späteren Zugriff gespeichert werden.

aus dem Buch "Byte of Python" habe ich dieses Programm geschrieben. paste.pocoo.org.
Es kann ein Dictionary aus einer .json-Datei, lesen, verändern und wieder in die .json-Datei schreiben.

Ich möchte noch eine Suchfunktion, die möglichkeit mehrere Telefunnummern und E-Mail-Adressen unter einem Eintrag zu speichern und ein paar sinnvolle Option einbauen.

Würde mich über Kritik und Feedback freuen.

[martin101986]

Hallo,

anstatt das Dictionary für jede Aktion im Adressbuch aus der Datei zu laden und anschließend wieder zu speichern, tätte ich einmal das Adressbuch laden und in einer Variable ablegen und beim beenden des Programmes wieder in einer Datei speichern.

Da du keinen Objektorientieren Ansatz gewählt hast würde ich das Adressbuch jeden Funktionsaufruf als Variable mitgeben und dieses in der Funktion bearbeiten.

Lass den Code ab Zeile 45 von der Modulebene verschwinden und verwende diesen in einer main-Funktion.

Module kannst du ausführbar machem mit einem:

Code: Alles auswählen

 if __name__ == '__main__':
    main()

Anstatt der if/elif Abfragen kannst du eine Dictionary verwenden, dass die entsprechenden Funktionen aufruft:

Code: Alles auswählen

opt = {"show":show_entry,
           "add":add_entry,
           "remove":del_entry}

d[arg[0]](arg)

Grüße Martin[/code]

edit: Code ausgebessert

[ltownatze]

Danke für deine Mühe

Module kannst du ausführbar machem mit einem: [...]

Ich hatte es ne ganze Zeit lang so gemacht, da es aber nicht vorgesehen ist die Datei als Modul zu laden kam es mir überflüssig vor, daher habe ichs entfernt. Versteh ich hier etwas Falsch!?

Anstatt der if/elif Abfragen kannst du eine Dictionary verwenden, dass die entsprechenden Funktionen aufruft

Das hört sich gut an. Ich dachte mir schon, das es auch ohne if/elif-Massaker möglich ist, hatte das aber erstmal aufgeschoben.

[cofi]

Nein das verstehst du schon richtig, aber für den Fall, dass jemand das Modul doch importieren will, ist es gut das so zu kapseln. Fuer die Entwicklung ist das auch eine nette Angelegenheit, da du nur das Modul neuladen musst und in derselben Python-Sitzung weiterarbeiten kannst.

[BlackJack]

@ltownatze: Die Funktionen könnte man aber später durchaus weiterverwenden. Zum Beispiel wenn man eine GUI für das Adressbuch schreibt, bleiben die Funktionen zum laden/speichern/hinzufügen/entfernen etc. ja gleich.

Ausserdem gibt's noch andere Gründe warum ein Modul importiert werden könnte. Zum Beispiel von Programmen die eine HTML-Dokumentation aus den Docstrings erstellen oder Codeanalyseprogramme wie `pylint`.

[ltownatze]

Ich habe jetzt

Code: Alles auswählen

if __name__ == "__main__":
    main()

wieder ans Ende des Codes geschrieben. Seltsamer weise bekomme ich jetzt 2 unterschiedlich SyntaxError, je nachdem wie ich die Datei ausführe.
Aus einer Shell:

$ ./meinAdressbuch.py
File "./meinAdressbuch.py", line 53
if __name__ == '__main__':
^
SyntaxError: invalid syntax

Hier zeigt der Pfeil auf den ':' am Ende der Zeile

Aus DrPython mittels 'Run Script':

$ ./meinAdressbuch.py
File "./meinAdressbuch.py", line 53
if __name__ == '__main__':
^
SyntaxError: invalid syntax

Ich muss mich korrigieren:
Hier zeigt der Pfeil auf das zweite '='.

Ich habe noch ein Backup, aus dem ich die if-Abfrage kopiert habe, dort funktioniert es. Wie kommt sowas?

Update: Habe den Fehler gefunden. Es lag an der Zeile davor:

Code: Alles auswählen

cmd[args[0](args[1])

Wenn ich die auskommentiere gehts. Weiß zwar noch nicht wo da der Fehler ist aber das werd ich noch rausfinden. Also vorerst bitte
keine Tipps in der Richtung. Möchte das gerne alleine schaffen

Update2: Fehler gefunden.. bei den ganzen Klammern kann man schonmal den Überblick verlieren...
Ich beende jetz mal mein Selbstgespräch

[ltownatze]

Neue Version: paste.pocoo.org
Habe versucht eure Vorschläge einzuarbeiten.
Ist mir das gelungen?

[BlackJack]

@ltownatze: Es reicht nicht Methoden einfach zu referenzieren, man muss sie auch *aufrufen*. Also ``fp.close()`` statt ``fp.close``.

Die Argumentübergabe ist IMHO schlecht gelöst. Die API sollte für Menschen sein und nicht den Zwängen einer möglichst einfachen Übergabe von Kommandozeilenargumenten folgen. Wenn da ein Argument `name` heisst, erwartet kaum jemand dass man da eine Liste mit einem oder zwei Elementen übergeben muss, sondern dass an diesen Namen wirklich ein Name und das auch direkt gebunden werden soll. Schau Dir mal in der Doku zu Funktionen an wie man beim Aufruf mit einem ``*`` eine Sequenz von Argumenten "entpacken" kann.

Bei `cleanup()` denke ich an eine Funktion die Daten bereinigt, kürzt, Doubletten entfernt, oder ähnliches, aber nicht eine die einfach nur die Daten speichert.

Der Name der Adressbuchdatei sollte nur *einmal* im ganzen Programm stehen. Wenn man den ändern möchte muss man dann nicht das ganze Programm absuchen und hoffen, dass man alle vorkommen erwischt. Der Dateiname wäre auch ein Kandidat für eine Option.

[ltownatze]

Es reicht nicht Methoden einfach zu referenzieren, man muss sie auch *aufrufen*. Also ``fp.close()`` statt ``fp.close``.

Dämlicher Flüchtigkeitsfehler.. danke für den Hinweis

Schau Dir mal in der Doku zu Funktionen an wie man beim Aufruf mit einem ``*`` eine Sequenz von Argumenten "entpacken" kann.

Das 'name' ist noch ein Relikt aus der alten Version..
Werd mir das mal anschauen..

Bei `cleanup()` denke ich an eine Funktion die Daten bereinigt, kürzt, Doubletten entfernt, oder ähnliches, aber nicht eine die einfach nur die Daten speichert.

Die Funktion heißt 'cleanup', weil ich damit rechne, das dort noch mehr rein kommt um vor dem Beenden aufzuräumen.. bin mit dem Namen aber auch nicht 100% zufrieden...

Der Dateiname wäre auch ein Kandidat für eine Option.

Das ist ne gute Idee. Werd ich umsetzen

[ltownatze]

Für alle die es Interessiert hier die vorerst fertige Version von 'meinAdressbuch'
paste.pocoo.org

Ich arbeite zur Zeit an einem GUI mit GtkBuilder und Glade3, hab da noch ein paar Probleme zu lösen aber das wird schon.
Sobald ich das ganze für vorzeigbar halte werde ich das hier posten und in der Zwischenzeit könnt ihr die ein oder andere Frage von mir im Gtk/Gnome-Forum lesen und hoffentlich beantworten

[BlackJack]

@ltownatze: ``global`` sollte man nicht verwenden. So sind eine Menge Funktionen von dieser einen, globalen Variable abhängig und sind schlechter wiederzuverwenden. Stell Dir zum Beispiel mal eine Anwendung vor, die das Modul verwendet und mit mehreren Adressbüchern gleichzeitig arbeiten möchte. Da müsste man ständig aufpassen, dass die globale Variable vor jedem Aufruf den richtigen Wert hat.

Die Namen sind für meinen Geschmack teilweise zu kurz. Das `ab` für `address_book` steht sollte man nicht raten müssen. `abfile` ist irreführend weil an den Namen gar keine Datei sondern ein Datei*name* gebunden wird.

Man sollte versuchen Wiederholungen zu vermeiden. Man könnte für den `OptionParser` die Benutzerinformation einmal etwas ausführlicher schreiben, statt im Fehlerfall dann immer wieder Teile davon auszugeben, teilweise auch mit Wiederholung von Texten im Quelltext. `OptionParser` kennt die Methoden `print_help()` und `print_usage()`, sowie `error()`.

Du schliesst die JSON-Datei nach dem Einlesen nicht. Da fehlt der *Aufruf* von `close()`.

Warum wird bei den Funktionen, welche die Kommandos implementieren eine variable Anzahl von Argumenten entgegengenommen, wo die Funktionen doch *grundsätzlich* mit *genau zwei* Argumenten aufgerufen werden? Das erste was dann immer gemacht wird, ist das *eine* Argument an Index 0 anzusprechen. Eine völlig sinnlose Verschachtelung.

Wobei die Funktionen sowieso lesbarer wären wenn die Argumente nicht als Liste reinkommen würden, sondern in der Funktionsignatur ordentliche Namen bekämen, und nicht erst *in* der Funktion. Der ``*`` sollte beim Aufruf stehen und nicht in der Funktionsignatur.

[ltownatze]

``global`` sollte man nicht verwenden.

Mir kam das auch wie ein Workaround vor.. kannst du mir evtl. eine alternative Vorschlagen?

Du schliesst die JSON-Datei nach dem Einlesen nicht. Da fehlt der *Aufruf* von `close()`.

Der Fehler (vergessen der '()' beim Funktionsaufruf) passiert mir immer wieder.. :/

Warum wird bei den Funktionen, welche die Kommandos implementieren eine variable Anzahl von Argumenten entgegengenommen, wo die Funktionen doch *grundsätzlich* mit *genau zwei* Argumenten aufgerufen werden?

Das stimmt so nicht. Die Funktion 'add_entry' benötigt 2, alle anderen nur 1 Argument.

Wobei die Funktionen sowieso lesbarer wären wenn die Argumente nicht als Liste reinkommen würden

Schau Dir mal in der Doku zu Funktionen an wie man beim Aufruf mit einem ``*`` eine Sequenz von Argumenten "entpacken" kann.

Ich dachte ich hätte damit deinen Vorschlag umgesetzt (Siehe deinen Beitrag vom 17.Apr, 14:22)
Wenn ich mir das ganze jetzt aber nochmal durchlese glaube ich das ich etwas falsch verstanden habe..
Werde mir das am Wochenende nochmal angucken und Versuchen das umzusetzen und bei der gelegenheit auch gleich die Namen aktualisieren. (Habe in der schon erwähnten GUI-Version wesentlich bessere verwendet)

PS.: Freut mich, das du dir die Zeit nimmst meinen Code zu lesen und zu kritisieren! (Hoffe das klingt nicht zu geschleimt..!?)

[BlackJack]

@ltownatze: Zur Zahl der Argumente: Die werden alle nur mit zweien aufgerufen. Das zweite ist eine Liste die ein einziges Argument enthält. nämlich wieder eine Liste, und *die* hat dann in allen bis auf einen Fall auch wieder nur ein Element. Bei `add_entry()` sind es zwei Elemente in der innersten Liste. Die Funktion selbst bekommt aber nur zwei Argumente.

[Hyperion]

Ich persönlich finde das default mäßige Anhängen der Endung ".json" in Zeile 22 ja auch suboptimal. Wenn ich mit einer Shell arbeite, dann nutze ich idR Tab-Completion und habe damit den kompletten Datennamen (+ ggf. Pfad). Insofern würde ich es dem Anwender überlassen, den richtigen und kompletten Dateinamen anzugeben.

[ltownatze]

Code: Alles auswählen

cmd[args[0]](ab, *args[1:])

def show_entry(ab, name):
        print name, ':', ab.get(name, 'not found, try `meinAdressbuch show | grep -i <NAME>`')

Habe ich das jetzt richtig verstanden?

Wenn ich das so löse, bekomme ich ein neues Problem:
Es funktioniert nicht, wenn man 'show' ohne weitere Argumente aufruft. Also so:

Code: Alles auswählen

$ ./meinAdressbuch.py show

Das selbe wird bei allen anderen Funktionen wohl auch passieren..
Mach ich was falsch oder ist dieser Lösungsansatz ungeeignet?

[BlackJack]

@ltownatze: Da könnte man mit Defaultargumenten arbeiten.

Code: Alles auswählen

def show_entry(ab, name=None):
        if name is not None:
            print name, ':', ab.get(name, 'not found, try `meinAdressbuch show | grep -i <NAME>`')
        else:
            pass # Irgendwas anderes machen.

[ltownatze]

Nicht ganz ohne Stolz präsentiere ich eine komplett überarbeitete Version von meinAdressbuch

Wie immer freue ich mich auf kontruktive Kritik.

Edit: Ich spiele mit dem Gedanken Docstrings einzubauen.. haltet ihr das bei einem so kleinen Script/Modul/Programm[?] für Sinnvoll? Welchen Mehrwert hätte das für den Anwender/Programmierer?

[BlackJack]

@ltownatze: Ich sehe nicht so ganz den Mehrwert das in dieser Form in eine Klasse zu stecken. Ausser einmal ein Exemplar zu erzeugen, dass dann die Kommandozeilenargumente auswertet, kann man damit ja nicht viel machen.

Die Klasse vermischt zuviel. Die Benutzerinteraktion ist da mit drin, und man kann auch kein Adressbuch erzeugen, ohne das versucht wird etwas aus einer Datei zu laden.

[ltownatze]

Ich sehe nicht so ganz den Mehrwert das in dieser Form in eine Klasse zu stecken.

Das schien mir die beste möglichkeit zu sein 'global' zu vermeiden ohne 'addressbook_filename' an jede Funktion übergeben zu müssen.
Meinen Recherchen und Experimenten zu Folge ist die Verwendung von 'self' nur innerhalb einer Klasse möglich.
Die möglichkeit über 'self' auf Referenzen aus dem Namensraum von 'MeinAdressbuch' zuzugreifen hat auserdem den positiven Nebeneffekt das ich problemlos auf 'parser.get_usage()' zugreifen kann.

Glaubst du ich sollte von der Klassen-Idee abstand nehmen?

[Hyperion]

Glaubst du ich sollte von der Klassen-Idee abstand nehmen?

Ja! Zumindest von der, wie Du es jetzt machst.

Im Grunde wrappst Du nur Dictionary-Zugriffe und baust Speicher- und Lade-Mechanismen direkt in die Klasse ein. Du kannst doch genauso gut Funktionen schreiben, die ein Adressbuch-Dictionary ausgeben, sortieren usw. Das Laden und Speichern würde ich auch eher aus der Klasse rausziehen. Ist doch toll eine Funktion zu haben, die solch eine Klasse anlegen kann.

Prinzipiell würde ich erst einmal alles ohne Klassen versuchen. Solltest Du auf die Idee kommen, verschachtelte Datenstrukturen einzubauen, oder eine schönere Sortiermethode anzugehen, kann man später immer noch eine Art "Adresse"-Klasse bauen und die Objekte in einer Liste oder einem Dictionary halten.

Das Argument mit dem global habe ich nicht ganz verstanden. Den Dateinamen brauchst Du doch nur zum Laden und Speichern. Da müßtest Du ihn also an zwei Funktionen übergeben - imho nicht aufwendig Aber dennoch braucht es da keine Klasse, um ein global zu vermeiden.