Telefonbuch

[Mugen]

Hallo, ich bin's mal wieder.
Momentan lerne ich mit dem Buch "Objektorientierte Programmierung mit Python 3" von Michael Weigend (wie schneidet das bei euch eigentlich so ab - besser als die Bücher vom Galileo-Verlag?) und nach meinem "Taschenrechner-Projekt" habe ich mich mal an etwas "größeres" gewagt: eine Art Telefonbuch.
Ich hab mich dabei strikt an die Aufgabenstellung aus dem Buch gehalten und deshalb gäbe es wohl noch mehr mögliche Funktionen (die Möglichkeit, Personen aus dem Telefonbuch zu streichen etc.), aber ich wollte erst mal fragen, ob das noch pythonischer geht.

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

PS: Darf ich den Lösungscode aus dem Buch eigentlich hier hochladen? Ich will das nur wissen, weil der Autor eine globale Variable benutzt hat (die hier ja so verteufelt wird) und ich ohne ausgekommen bin.

[webspider]

Ich will das nur wissen, weil der Autor eine globale Variable benutzt hat (die hier ja so verteufelt wird) und ich ohne ausgekommen bin.

Interessant. Was soll dann Zeile 4 sein?

[Mugen]

Na ja, ich meinte jetzt ein global-Statement in einer Funktion.
Ließe sich das denn noch anders lösen?

[nomnom]

Na ja, ich meinte jetzt ein global-Statement in einer Funktion.
Ließe sich das denn noch anders lösen?

Wenn man es nicht anders lösen könnte, dürften wir die globalen Variablen nicht verteufeln. Warum nutzt du die Objektorientierung nicht aus?

[Hyperion]

Ließe sich das denn noch anders lösen?

Ja klar, indem Du das Dictionary-Objekt einfach an die Funktionen übergibst Nehmen wir das Beispiel ``save_new_number``. Die Signatur der Funktion sähe dann eben so aus:

Code: Alles auswählen

def save_new_number(phone_book):
    # usw.

Das kannst Du dann aus Deinem Menü mit dem Objekt als Parameter aufrufen.

Einige Anmerkungen zum Code:

- Es ist imho ungewöhnlich, dass eine ``main``-Funktion ganz oben in der datei steht. IdR. erwarte ich die unten direkt über dem ``if __name__``-Hook.

- Wieso merkst Du Dir die Menüeinträge nicht auch in Deinem Dispatcher-Dict? z.B. so:

Code: Alles auswählen

    choices = {
        'S': (search_number, (S)uche nach Telefonnummer),
        'N': (save_new_number, "(N)eue Nummer eintragen"),
        'A': (print_numbers, "(A)lle Nummern ausgeben"),
        'E': (sys.exit, "(E)nde")
    }

Damit hast Du alles in einer Datenstruktur und musst beim Hinzukommen / einer Änderung eines Menüpunktes nicht eine ``print``-Zeile hinzufügen Wenn es Dir auf die Reihenfolge ankommt, dann kannst Du z.B. ein ``OrderedDict`` benutzen - denn bei einem normalen Dictionary werden die Menüpunkte natürlich nicht in der gegebenen Reoihenfolge ausgegeben. Alternativ verzichtest Du auf das Dictionary und nimmst generell ein Tupel, oder legst Dir ein Tupel der Keys für die Reihenfolge separat an.

- Beim Dispatching kannst Du Dir das Überschreiben von ``choice`` sparen:

Code: Alles auswählen

# Deine Lösung
        try:
            choice = choices[choice]
            choice()
        except KeyError:
            print("Auswahl nicht erkannt")
            print()
            continue
# imho hübscher:
        try:
            choices[choice]()
        except KeyError:
            print("Auswahl nicht erkannt", end="\n\n")

- Generell mischst Du die Verwendung von ``print()`` für eine Leerzeile und das Anhängen eines "\n". Das würde ich vereinheitlichen.

- ``search_number`` kann so doch nicht klappen.

Code: Alles auswählen

        print(list(telefonbuch.values())[0])

Mir diesem Code bekommst Du niemals die Nummer der gesuchten Person
Genau an dieser Stelle kann man sich auch streiten, ob man mit einem ``if something in dict``-Konstrukt sicher stellt, dass ein Name im Telefonbuch enthalten ist, oder es mittels ``try... except KeyError`` löst, wie Du das beim Dispatchen oben tust.

- Zeile 54 kann man kürzen:

Code: Alles auswählen

# bei dir
for i in tel.keys():
# kompakter
for name in tel:

- Namen wie ``i`` sind auch nicht besonders toll. Wieso nimmst Du an der Stelle nicht ``name`` als Namen? Genau das repräsentiert ja der Schlüssel in Deinem Telefonbuch!

- Wieso schreibst Du kein leeres Dict in die Datei bei der Fehlerbehandlung in ``print_numbers``? Taucht denn da gar kein Fehler beim Laden auf in Zeile 51:

Code: Alles auswählen

        with open("tel.txt", "rb") as tel:
            tel = pickle.load(tel)

Aus einer leeren Datei kann ``pickle`` ja eigentlich nur ein ``None``-Object generieren - das wäre für den weiteren ABlauf aber fatal.

Generell sieht das alles gar nicht so verkehrt aus. Aber: Bist Du Dir darüber im klaren, dass Du immer auf Dateiebene arbeitest? Bei jedem Eintrag in Dein Telefonbuch speicherst Du das Telefonbuch-Dict und bei jeder Anzeige lädst Du das Objekt wieder in den Speicher. Ist das sinnvoll? Beim Neuanlegen ist das vielleicht sogar eine gute Idee, damit man Änderungen nicht immer separat speichern muss (per zudätzlichem Menüpunkt). Aber beim Anzeigen der Datensätze, musst Du das doch nicht neu laden?! Du hast die Daten ja im Speicher Zumal Du beim Suchen da "inkonsequent" bist und tatsächlich in Deinem Objekt suchst.

Zudem hast Du den Dateinamen ``tel.txt`` somit als "magic number" fest in Deinem Code in jeder Funktion. Imho ist es da sinnvoller, die Dateioperationen von den eigentlichen Datenoperationen zu separieren und zudem den Dateinamen als Parameter an diese IO-Funktionen zu übergeben. Wenn Du ihn nicht per Kommandozeilenparameter übergeben magst, kannst Du den tatsächlich auf Modulebene festlegen:

Code: Alles auswählen

PHONE_BOOK_FILENAME = "tel.txt"

(Wobei ".txt" nicht so glücklich ist bei einer ``pickle``-Datei - die ist ja binär!)

[Mugen]

So, erst mal Entschuldigung, dass ich so spät antworte.

Ich habe soweit die Vorschläge von Hyperion umgesetzt, aber ganz zufrieden bin ich (und ihr wahrscheinlich auch) damit nicht.

http://paste.pocoo.org/show/548457/
Fix: http://paste.pocoo.org/show/548471/ (aktuell)

@if-Kaskaden von Zeile 52-57: Dort wusste ich nicht, wie man den Funktionen im Dictionary beim Aufruf Argumente übergibt. Ist die Lösung in Ordnung, oder wird dadurch das dict obsolet?

Im Moment krieg ich das Anfügen von neuen Daten auch nicht hin. Ich hab schon versucht als Modus ``"ab+"`` zu wählen, aber da wird bei mir gar nichts angefügt.

Edit: Die "unterdrückten" Exceptions (Zeile 21-22, Zeile 30-31) habe ich nur eingefügt, damit es bei einer leeren ``tel.txt`` keinen Programmabbruch gibt. Sicherlich auch nicht optimal?

[Hyperion]

Ich habe soweit die Vorschläge von Hyperion umgesetzt, aber ganz zufrieden bin ich (und ihr wahrscheinlich auch) damit nicht.

Oh weia... das sieht eher verschlimmbessert aus, sorry!

@if-Kaskaden von Zeile 52-57: Dort wusste ich nicht, wie man den Funktionen im Dictionary beim Aufruf Argumente übergibt. Ist die Lösung in Ordnung, oder wird dadurch das dict obsolet?

Die sind überflüssig und ja, die machen Dir ja den vorteil des Dicts kaputt! Dein Problem ist, dass Du verschiedene Objekte an die Funktionen weiterreichst. Dabei willst Du doch nur ein und dasselbe an alle weiterreichen, nämlich Dein Telefonbuch-Dictionary! Wieso gibt es dafür zweimal den Namen ``tel`` und einmal ``telbook``? Zumal Du da immer noch Bockmist baust: Wieso legst Du eine leere Datei an? Wieso legst Du keine Datei mit einem leeren Dictionary an? Das sind doch zwei Paar Schuh! Und der ``EOFError`` kommt doch bestimmt dann, wenn man aus einer leeren Datei etwas "pickeln" will, oder?

Im ``else``-teil steht doch der Aufruf schon - Du musst ja nur noch den Parameter einsetzen.

Ok, ``sys.exit`` nimmt ja kein Dictionary entgegen... da könnte man mittels ``lambda`` einen Workaround basteln:

Code: Alles auswählen

'E': (lambda _: sys.exit(), "(E)nde")}

Edit: Die "unterdrückten" Exceptions (Zeile 21-22, Zeile 30-31) habe ich nur eingefügt, damit es bei einer leeren ``tel.txt`` keinen Programmabbruch gibt. Sicherlich auch nicht optimal?

Ja, die waren mir eh unklar, aber daran sieht man ja, dass Du einen fetten Fehler bezüglich Deiner Datenstruktur drin hast

Der Name ``save_new_number`` gefällt mir so auch nicht; denn darin werden ja zwei Dinge erledigt: 1.) Ein neuer Datensatz in das Dict eingefügt und 2.) das Dict gespeichert. Wieso nicht zwei Funktionen daraus bauen?

Und wieso heißt der Parameter bei zwei Funktionen ``tel`` und einmal ``telbook``, obwohl immer das gleiche Objekt übergeben werden soll?

[Mugen]

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

So besser?

Ansonsten hätte ich noch eine Frage: Wie kann ich prüfen, ob die Datei ein Dictionary enthält, welches bereits mit Daten gefüllt ist? Dann sollte im append-binary-Modus gespeichert werden, weil ansonsten ja die anderen Daten futsch wären.

Edit: Das return-Statement in Zeile 22 war übrigens nur zum Testen.

[nomnom]

Code: Alles auswählen

In [1]: bool({})
Out[1]: False

In [2]: bool({1:23})
Out[2]: True

So vielleicht? Und wenn du `pickle` benutzt wird afaik der `append` Modus nicht funktionieren.

[Hyperion]

So besser?

Imho ja.

Aber:

- In Zeile 10 willst Du einen ``IOError`` abfangen - oder nicht? Niemals einfach so ein except benutzen. Damit fängst Du alle Exceptions ab - und das willst Du nicht.

- Der einzelne Aufruf in Zeile 51 ist unnötig.

- Ich würde die Funktion nicht ``make_dict`` nennen - es geht Dir ja um das Telfeonbuch. Und das generierst Du ja auch nicht (immer), sondern lädst es aus einer Datei. Wieso also nicht ``get_telbook``? Ggf sogar ``load_telbook``.

- Es fehlen noch ein Shebang und ein Encoding-Cookie

- Es fehlt die komplette Doku

Ich lasse immer zwei Zeilen zwischen Funktionen frei; das erhöht imho die Leserlichkeit. Speziell, weil man ja ab und an einzelne Zeilen auch innerhalb von Funktionen leer lässt.

[Mugen]

Und wenn du `pickle` benutzt wird afaik der `append` Modus nicht funktionieren.

Ja, es scheint so. Trotzdem Danke.
@Hyperion: Danke für die Hilfe. Und morgen auf zum OOP-Kapitel.