Adressbuch V1.0

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
pyMarkus
User
Beiträge: 9
Registriert: Montag 20. April 2009, 19:05
Kontaktdaten:

Montag 20. April 2009, 19:15

Hallo zusammen!
Ich habe vor einer Woche begonnen Python-Programmierung zu erlernen. Zum Start hab ich mir "Dive into Python" angeeignet und am Ende des Buches empfiehlt der Autor ein Adressbuch zu programmieren. An diese Aufgabe hab ich mich gesetzt und heute Nachmittag bin ich zur Version 1.0 gekommen. Zu diesem Zeitpunkt dachte ich ist der richtige Moment mich an die Community zu wenden und mir rechtzeitig Tipps und Tricks zu holen, bevor ich mir schlechte Dinge einpräge.

Den Code findet ihr unter http://paste.pocoo.org/show/113397/

Bin für jegliche Kritik offen! Besonders für schlechte, da lernt man am meisten =)

Gruß
Markus
derdon
User
Beiträge: 1316
Registriert: Freitag 24. Oktober 2008, 14:32

Montag 20. April 2009, 19:30

- Benutze Leerzeichen statt Tabs (PEP8 schlägt 4 vor)
- Fange Ausnahmen nicht mit einem reinen except ab
- Benutze

Code: Alles auswählen

if __name__ == '__main__':
    main()
- Verwende keine unnötige Rekursion
- Es ist aus usability-Sicht gesehen bequemer, ein Skript mit Argumenten und Optionen aufzurufen als nach dem Aufruf mit lästigen raw_input Befehlen konfrontiert zu werden
- Vermeide print-statements in Methoden
- Sind die Klassen wirklich notwendig?

Fazit: Das Skript lässt sich von der Größe auf weniger als die Hälfte reduzieren ohne dass Funktionalitäten verlören gingen.

PS: Der gute Stil kommt von alleine, wenn du von den erfahrenen Programmierern hier im Forum (oder anderswo) abguckst :wink:
pyMarkus
User
Beiträge: 9
Registriert: Montag 20. April 2009, 19:05
Kontaktdaten:

Montag 20. April 2009, 19:37

Kannst du mir den Codeschnippel erklären den du mir gepostet hast?
Benutzeravatar
HerrHagen
User
Beiträge: 430
Registriert: Freitag 6. Juni 2008, 19:07

Montag 20. April 2009, 19:42

Nur ganz schnell:
- bei C oder Dummy Varianten von Modulen verwendet man gern sowas:

Code: Alles auswählen

import cPickle as Pickle
da kann man hinterher leicht austauschen.
- Z.65-93. statt vielen if's macht man lieber ein dictionary indem man die Funktionen speichert, also:

Code: Alles auswählen

funcs = {'a':spam, 'b':eggs}
funcs['a']()
so werden die ensprechenden Funktionen z.B. nur an einer Stelle aufgerufen und try/except wird einfacher
- Einrückung immer mit 4 Leerzeichen!
- Lass lieber Kommentare dieses Stils (#Kontakt aus Liste entfernen) weg und schreib vernüftige doc-Strings, so sind Hinweise zur Benutzung auch von außen zu erreichen


MFG HerrHagen
derdon
User
Beiträge: 1316
Registriert: Freitag 24. Oktober 2008, 14:32

Montag 20. April 2009, 19:50

pyMarkus hat geschrieben:Kannst du mir den Codeschnippel erklären den du mir gepostet hast?
http://docs.python.org/library/__main__.html

So wird garantiert, dass der Code nur ausgeführt wird, wenn das Skript *direkt* aufgerufen wurde (also z.B. aus dem Terminal) und nicht, wenn es als Modul importiert wurde. Du willst doch auch keine Ausgabe auf dem Terminal haben, nachdem du

Code: Alles auswählen

import adressbuch
eingegeben hast.
EyDu
User
Beiträge: 4871
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Montag 20. April 2009, 19:54

Ich habe dir da mal einiges zusammengetragen:

Du vermischt überal Logik mit Anzeige. Wenn du jetzt von deiner textbasierten Anwendung zu einer mit GUI wechseln wollen würdest, dann müsstest du den ganzen Code noch einmal schreiben.

Einzelne Werte als Attribute zu Klassen hinzuzufügen ist meistens eine recht schlechte Idee (z.B. Kontakt.name und Kontakt.email). Diese sind dann nämlich genau einmal für die Klasse vorhanden und unterscheiden sich nicht von Instanz zu Instanz.

Die "auskunft"-Methode in Kontakt sollte besser "__str__" heißen und einen String zurückgeben und nicht schreiben.

Die Name-Klasse ist vollkommen überflüssig.

Warum ist das Adressbuch keine Klasse?

Die Verzweigungen in der optionen-Funktion könntest du durch ein (Default-)Dictionary besser lösen. Selbiges gilt für adressbuch.

Du rufst die adressbuch-Funktion immer wieder rekursiv auf. In Python würdest du das eher iterativ lösen.

Du solltest dich zwischen Englisch und Deutsch entscheiden.

aufnahme_add und aufnahme machen genau gar nichts. Würden sie die erzeugten Objekte zurückgeben, so könntest du dir diesen misen Hack über die Klassenatribute sparen. Auch das Erzeugen der Objekte machst du sehr seltsam, was ist, wenn ein Fehler bei der Eingabe auftritt?

Die suchen-Funktion sollte einfach probieren den Wert aus dem Dictionary zu holen und einen Fehler durch eine Exception abfangen. Vorher zu prüfen lohnt sich nicht.

Die Dateinamen an Namen zu binden ist überflüssig, die kannst du auch direkt beim öffnen hinschreiben. Oder noch besser: übergeben der Dateinamen als Parameter.

Dateien solltest du mit "open" öffenen un nicht mit "file".

Code gehört nicht auf Modulebene, den solltst du durch ein

Code: Alles auswählen

if __name__ == "__main__"
schützen, dann kannst du es auch importieren.

Du musst "ab" nicht vorher definieren, mit "ab = cPickle.load(f)" wird es eh gleich wider überschrieben (Zeile 161 und 162).

Wenn du eine Datei öffnest, auch zum Lesen, dann solltest du sie auch wieder schließen.

Einrückung mit vier Leerzeichen und nicht mit Tabs.

Algemein:
- Arbeite mit Rückabewerten (return) und Parameter.
- Gewöhne dir aussägekräftigere Namen an
- Wirf doch noch einmal einen Blick ins offizielle Python-Tutorial
- Der Programmablauf ist total verwirrend, in jeder Funktion wird die Folgefunktion aufgerufen, dass solltest du zentraler regeln. Du hast ein exzellentes Beispiel für Spaghetti-Code geliefert ;-)


Jetzt hast du hoffentlich genug Anregung. Da ist noch eine Menge Arbeit nötig. Ich empfehle alles neu zu schreiben und vorher das Programm etwas zu planen.
Das Leben ist wie ein Tennisball.
pyMarkus
User
Beiträge: 9
Registriert: Montag 20. April 2009, 19:05
Kontaktdaten:

Montag 20. April 2009, 22:18

Zu so später Stunde möchte ich noch das neue Grundgerüst vorstellen. Entspricht das der Kritik die ich bekommen habe oder bin ich immer noch auf dem Falschen Dampfer.
Der Optionen"Block" wird komplett durch sys.argv ersetzt werden, sodass man direkt im Adressbuch landet wenn man den Skript startet.
Das Adressbuch wurde nun zur Klasse mit seinen gesamten Methoden.

http://paste.pocoo.org/show/113450/

Anschließend noch eine Frage:
Würde sich jemand als "Privatlehrer" bereit erklären, damit ich wegen kleineren Fragen PN schicken kann, oder sogar ICQ.

Gruß
Markus
derdon
User
Beiträge: 1316
Registriert: Freitag 24. Oktober 2008, 14:32

Montag 20. April 2009, 22:24

Wenn du die Funktion main aufrufst, dann muss sie auch im Namespace definiert sein. Weil du nicht weißt, wie man auf einen NameError zu reagieren hat, empfehle ich dir das Tutorial (auf Version achten).
Dauerbaustelle
User
Beiträge: 996
Registriert: Mittwoch 9. Januar 2008, 13:48

Dienstag 21. April 2009, 00:14

Code: Alles auswählen

class Adressbuch(Kontakt):
    '''Methoden fuer das Adressbuch
        Nimmt die Kontaktdaten der Klasse "Kontakt" auf
        damit die Methoden Variablen erhalten'''

    def __init__(self, name, email):
        Adressbuch.__init__(self, name, email)
Weißt du, was du machst, wenn du in der Initialisierungsfunktion einer Klasse die Initialisierungsform der gleichen Klasse aufrufst? O___o
pyMarkus hat geschrieben:Anschließend noch eine Frage:
Würde sich jemand als "Privatlehrer" bereit erklären, damit ich wegen kleineren Fragen PN schicken kann, oder sogar ICQ.
Ich hoffe nicht, denn dann dient das hier alles niemandem.
Benutzeravatar
Hyperion
Moderator
Beiträge: 7472
Registriert: Freitag 4. August 2006, 14:56
Wohnort: Hamburg
Kontaktdaten:

Dienstag 21. April 2009, 07:42

Du machst bei Deinem Neu-Design imho einen entscheidenen Fehler:
Du hast Dir zu wenig Gedanken zur Datenmodeliierung gemacht.

Wie ich das sehe, ist Dein Addressbuch mehr oder minder eine Neuimplementierung einer Liste. Da fragt man sich einfach, wieso das in eine Klasse verpackt werden muss.

Ich würde das erst einmal ohne Klasse versuchen. (Für Kontakt mag es durchaus sinnvoll sein, nicht aber für "Addressbuch")
EyDu
User
Beiträge: 4871
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Dienstag 21. April 2009, 09:42

Ich finde den neuen Entwurf gar nicht so schlecht, auch das Adressbuch als Klasse halte ich sinnvoll. Im Moment wirkt es zwar mehr wie eine Neuentwicklung einer Liste, aber wer weiß, was noch für Funktionen hinzu kommen sollen.

Das Adressbuch sollte aber nicht von Kontakt ergeben, das macht keinen Sinn. Vererbung musst du dir als eine IST-EIN-Beziehung vorstellen. Und ein Adressbuch ist eindeutig kein Kontakt. Ein Adressbuch besitzt keinen Namen (ok, manche vielleicht schon^^) und sicher keine E-Mail-Adresse.

Das "if __name__ == '__main__':" solltest du noch ans Ende der Modul setzen, dort ist es üblich. Und dann noch einmal der Hinweis auf die Vermischung von deutschen und englischen Namen.

Als Privatlehrer wird sich wahrscheinlich niemand bereit erklären. Wie Dauerbaustelle schon meinte, sollen alle etwas von der Hilfe haben. Außer du bietest eine gewisse "Aufwandsentschädigung" ;-)
Das Leben ist wie ein Tennisball.
Benutzeravatar
HerrHagen
User
Beiträge: 430
Registriert: Freitag 6. Juni 2008, 19:07

Dienstag 21. April 2009, 17:30

Code: Alles auswählen

if __name__ == '__main__':
    main()
Das ganze scheitert, weil du keine Funktion main hast, die aufgerufen werden kann. Der Ausdruck ist nicht irgendeine wirre Floskel, sondern wird wie jedes andere Statement ganz normal ausgwertet. __name__ ist '__main__' wenn du die Datei direkt startet. Wenn du die Datei als Modul importierst, werden die Anweisungen innerhalb des If-Blocks nicht ausgeführt. So kannst du beispielsweise beim direkten starten der Datei einen Code ausführen, der die Funktionsweise des Moduls erläutert oder irgendwelche Tests (z.B. so) ausführt, zugleich aber das Modul normal importierbar machen (d.h. heißt es wird kein Testcode, etc. ausgeührt).

MFG HerrHagen
Antworten