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
Adressbuch V1.0
- Benutze Leerzeichen statt Tabs (PEP8 schlägt 4 vor)
- Fange Ausnahmen nicht mit einem reinen except ab
- Benutze
- 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
- Fange Ausnahmen nicht mit einem reinen except ab
- Benutze
Code: Alles auswählen
if __name__ == '__main__':
main()- 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
Kannst du mir den Codeschnippel erklären den du mir gepostet hast?
Nur ganz schnell:
- bei C oder Dummy Varianten von Modulen verwendet man gern sowas: da kann man hinterher leicht austauschen.
- Z.65-93. statt vielen if's macht man lieber ein dictionary indem man die Funktionen speichert, also:
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
- bei C oder Dummy Varianten von Modulen verwendet man gern sowas:
Code: Alles auswählen
import cPickle as Pickle- 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']()- 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
http://docs.python.org/library/__main__.htmlpyMarkus hat geschrieben:Kannst du mir den Codeschnippel erklären den du mir gepostet hast?
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 adressbuchIch 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
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.
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__"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.
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
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
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
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)Ich hoffe nicht, denn dann dient das hier alles niemandem.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.
-
Hyperion
- Moderator
- Beiträge: 7478
- Registriert: Freitag 4. August 2006, 14:56
- Wohnort: Hamburg
- Kontaktdaten:
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")
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")
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 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.
Code: Alles auswählen
if __name__ == '__main__':
main()MFG HerrHagen