Kritik zu Script und Speichern von Instanzen, OOP

[Hartmannsgruber]

Servus Leute,

bitte Kritik zu den kleinen Script, es ist nur ein Script zum Üben.
Mich würde interessieren ob ich Fehler mache und ob ich irgendwas andersmachen sollte

Code: Alles auswählen

#! /usr/bin/python3
###Klassendefinition###
class Kalender(object):
    '''
    Instanzen dieses Objekts nehmen Instanzen der Klasse Termin auf und
    speichern sie in einer Liste
    '''
    def __init__(self):
        self.__kalenderliste = []

    def getListe(self):
        return self.__kalenderliste

    def termineintragen(self, termin):
        self.__kalenderliste.append(termin)

    def termineanzeigen(self):
        for eintrag in self.__kalenderliste:
            print(eintrag)

        
class Termin(object):
    '''
    Instanzen dieser Klasse repräsentieren einen Termin
    '''
    def __init__(self, datum, uhrzeit, ueberschrift, beschreibung):
        self.__datum = str(datum)
        self.__uhrzeit = str(uhrzeit)
        self.__ueberschrift = ueberschrift
        self.__beschreibung = beschreibung


    def __repr__(self):
        return ("{0:} {1:} {2:} {3:}".format(self.__datum,
                                             self.__uhrzeit,
                                             self.__ueberschrift,
                                             self.__beschreibung)
                                            )


### Hauptprogramm ###
if __name__ == "__main__":
    k = Kalender()
    t = Termin("11.11.2014", "11:11", "Hellau", "Hellau rufen")
    t2 = Termin("12.12.2014", "12:30", "Arbeiten", "Mittagessen kochen")
    
    k.termineintragen(t)
    k.termineintragen(t2)
    k.termineanzeigen()

Zusätzlich habe ich jetzt noch eine Frage:
Wie speichere ich nun solche Termine ab, sollte ich Sqlite3 oder pickle benutzen?
Ich möchte ja in diesen Beispiel nicht jedesmal nach dem Start des Scripts die Termine neu eingeben müssen.

[BlackJack]

@Hartmannsgruber: Die ganzen führenden doppelten Unterstriche sind unsinnig. Bei `Kalender` gibt es einen Getter, damit ist das sowieso nicht mehr intern wenn man die Daten einfach so heraus gibt, und bei `Termin` sind die ganzen Sachen die einen Termin ausmachen etwas worauf niemand zugreifen können soll? Damit sind diese Objekte im Grunde unbenutzbar. Wenn man einen Wert als nicht-öffentliche API kennzeichnen möchte, dann macht man das mit *einem* führenden Unterstrich.

Konkrete Datentypen sollten nicht in Namen vorkommen. Es interessiert ja weniger was für ein Typ ein Wert hat als mehr wofür er im Programm steht. `kalenderliste` könnte man zum Beispiel einfach `termine` nennen.

Datum und Uhrzeit würde ich nicht als zwei verschiedene Attribute modellieren sondern als *ein* `datetime.datetime`-Objekt.

Edit: Zum Thema speichern: Pickle würde ich nur für kurzfristig gespeicherte Daten verwenden. Für längerfristig gespeicherte nur wenn die Datentypen sich garantiert nicht ändern, also zum Beispiel Grunddatentypen. *Dann* stellt sich allerdings die Frage nach dem Mehrwert gegenüber JSON als Speicherformat. Speziell bei Kalenderdaten würde ich auch das iCal-Format ins Auge fassen, weil das in dem Bereich zumindest für den Datenaustausch ein Standardformat ist.

[Hartmannsgruber]

Vielen Dank
Das mit den doppelten Unterstrichen vor den Instanzvariablen, stand so im Buch das habe ich dann wohl in diesen Falle missverstanden.
Die anderen beiden Sachen, sind logisch und werde ich mir merken.

Zum Thema speichern:
Also pickle sollte man demnach nur zum kurzfristigen Speichern benutzen.
JSON und iCal sind also Formate, die für die dauerhafte Speicherung von Daten gedacht sind, verstehe ich das richtig?
Sqlite funktioniert wohl bei OOP nicht.

[BlackJack]

@Hartmannsgruber: Vielleicht hat der Autor des Buches das auch falsch verstanden. Ist leider nicht so selten das Leute Bücher über eine Programmiersprache schreiben in denen sie Konzepte aus anderen Sprachen unbedingt irgenwie umsetzen wollen und von einer anderen Sprache ``public``, ``private``, und ``protected`` als Zugriffsschutzstufen kennen und das nun irgendwie zwanghaft auf Python abbilden wollen.

Bei `pickle` ist das Problem das sich Datentypen und Modul-/Paketstrukturen nicht ändern dürfen, was aber bei vielen Programmen im Laufe ihrer Entwicklung passiert. Und dann kann man Daten die mit einer älteren Version des Programms mit `pickle` serialisiert wurden, nicht mehr mit Programmversionen deserialisieren bei denen sich zu viel geändert hat. Und wenn man sich auf die Grunddatentypen zum picklen beschränkt, dann stellt sich die Frage welchen Vorteil `pickle` noch gegenüber dem JSON-Format hat. `pickle` hat dann den Nachteil das es auf Python beschränkt ist, während JSON ein Standard ist, für den es für sehr viele Programmiersprachen Unterstützung gibt. Entweder direkt in der jeweiligen Standardbibliothek oder zumindest als externe Bibliothek. Pickle eignet sich zum ”live” Datenaustausch zwischen Pythonprozessen, wird zum Beispiel vom `multiprocessing`-Modul verwendet. Oder um einen Cache aufzubauen wo es nett ist wenn man berechnete Werte schneller laden kann als sie neu zu berechnen, wobei es aber nichts ausmacht wenn man sie neu berechnen muss weil sich die Datentypen oder die grobe Programmstruktur geändert hat.

JSON ist ein allgemeines Datenformat und iCal ist eines speziell für Kalenderdaten. Sqlite funktioniert natürlich auch. Warum sollte das mit OOP nicht gehen?

[Hartmannsgruber]

ich dachte Sqlite3 kann keine Objekte speichern und da meine Termine alles Objekte sind meinte ich das haut nicht hin.

Müsste ich dann theoretisch alle Instanzvariablen in eine Liste von Tuples packen und dann in der .db speichern?
Beim wiedereinlesen, wären dass aber nur noch dynamsich erzeugte attribute
Stimmt mein Gedankengang?

[BlackJack]

@Hartmannsgruber: Du musst Dir halt überlegen wie man den Objektentwurf auf einen relationalen Datenbankentwurf abbildet, und dann Code schreiben der die Daten von den Objekten in der Datenbank ablegt, sowie Code der aus den Daten wieder Objekte erstellt. Bei JSON oder iCal wäre es ja letztendlich ähnlich, die können beide nichts mit Deinen Klassen/Objekten anfangen. Selbst bei `pickle` funktioniert das so, nur dass dort der Code schon vorhanden ist der Objekte in eine Art Programm für eine stapelbasierte Sprache umsetzt, und aus so einem Programm wieder Objekte, und das der Code total generisch ist, also nichts über Deine speziellen Objekte wissen muss, ausser dass es ”Python-Objekte” sind.

Bei relationalen Datenbanken kann man sich auch noch eine entsprechende Bibliothek zur Hilfe nehmen, die bei der Abbildung von Objekten auf Datenbanktabellen hilft und auch Abfragen hinter den Kulissen generiert, die man dann nicht selber zu schreiben braucht. In Python bietet sich da SQLAlchemy an.

Edit: SQLite hatte ich in meiner ersten Antwort übrigens ausgelassen, weil ich für einen Anfänger nicht gleich noch relationale Datenbanken und SQL ins Spiel bringen wollte, für Daten die zumindest im Moment noch in einem noch einfacherem Format als JSON gespeichert werden könnten: CSV.

[BlackJack]

Was mir beim Original noch aufgefallen ist: `__repr__()` sollte man nicht für eine Darstellung verwenden die für Endbenutzer gedacht ist. Dafür ist `__str__()` vorgesehen. Die `repr()`-Darstellung ist für Programmierer zur Fehlersuche und sollte daher eindeutig sein, also zum Beispiel deutlich machen wo der dargestellte Wert anfängt und wo er aufhört. Konventionell verwendet man dort entweder etwas was als Audruck wieder zu einem gleichen Objekt ausgewertet werden kann, oder etwas was in ”spitze Klammern” eingefasst ist, und sowohl den Typ als auch das Exemplar möglichst eindeutig identifiziert.

Hier mal ein Versuch der die Termine als CSV speichern kann:

Code: Alles auswählen

from __future__ import print_function
import csv
from datetime import datetime as DateTime

ISO_TIME_FORMAT = '%Y-%m-%dT%H:%M:%S'


class Appointment(object):
    
    def __init__(self, timestamp, subject, description):
        self.timestamp = timestamp
        self.subject = subject
        self.description = description

    def __str__(self):
        return (
            '{0.timestamp:%Y-%m-%d %H:%M} {0.subject} - {0.description}'.format(
                self
            )
        )

    def as_row(self):
        return [
            format(self.timestamp, ISO_TIME_FORMAT),
            self.subject,
            self.description
        ]

    @classmethod
    def from_row(cls, row):
        timestamp, subject, description = row
        return cls(
            DateTime.strptime(timestamp, ISO_TIME_FORMAT), subject, description
        )


class Calendar(object):
    
    CSV_DELIMITER = ';'

    def __init__(self, appointments=()):
        self.appointments = list()
        self.add_many(appointments)

    def __len__(self):
        return len(self.appointments)

    def __iter__(self):
        return iter(self.appointments)

    def add(self, appointment):
        self.appointments.append(appointment)

    def add_many(self, appointments):
        for appointment in appointments:
            self.add(appointment)

    def save_csv(self, filename):
        with open(filename, 'wb') as csv_file:
            writer = csv.writer(csv_file, delimiter=self.CSV_DELIMITER)
            writer.writerows(appointment.as_row() for appointment in self)

    @classmethod
    def load_csv(cls, filename):
        with open(filename, 'rb') as csv_file:
            reader = csv.reader(csv_file, delimiter=cls.CSV_DELIMITER)
            return cls(Appointment.from_row(row) for row in reader)


def print_calendar(calendar):
    appointment_count = len(calendar)
    print(
        '{0} appointment{1}.'.format(
            appointment_count, '' if appointment_count == 1 else 's'
        )
    )
    for appointment in calendar:
        print(appointment)


def main():
    calendar = Calendar()
    calendar.add_many(
        [
            Appointment(
                DateTime(2014, 11, 11, 11, 11),
                'Hellau',
                'Hellau rufen.'
            ),
            Appointment(
                DateTime(2014, 12, 12, 12, 30),
                'Arbeiten',
                'Mittagessen kochen.'
            ),
        ]
    )
    print_calendar(calendar)
    calendar.save_csv('test.csv')
    another_calendar = Calendar.load_csv('test.csv')
    print_calendar(another_calendar)


if __name__ == '__main__':
    main()

CSV-Datei:

Code: Alles auswählen

2014-11-11T11:11:00;Hellau;Hellau rufen.
2014-12-12T12:30:00;Arbeiten;Mittagessen kochen.

[Hyperion]

@BlackJack: Ich finde das Laden und Speichern sollte man aus der Calendar-Klasse herausziehen. Beides hat ja nix mit der eigentlichen Aufgabe eines Containers zu tun. Tut man das, so bleibt ja imho auch nichts übrig, wozu dieser Container überhaupt einen Mehrwert bietet gegenüber einer Liste... man kann ihn imho einfach durch eine solche ersetzen

Das sich Objekte selber lesen und schreiben können finde ich auch nicht wirklich toll. SRP lässt auch hier grüßen... was passiert, wenn man das ganze als JSON, XML oder sonst was haben möchte? Noch andere ``from_format_xyz``-Methode in die Klasse "stopfen"?

[BlackJack]

@Hyperion: Alles bis zu einem gewissen Grad richtig, aber IMHO nicht generell und nicht bei diesem Projekt. Wenn das grösser wird, und/oder mehrere Formate unterstützt werden sollen kann man das ja auseinander dividieren, aber hier finde ich KISS erst einmal sinnvoller. Und auch bei ein oder zwei zusätzlichen Formaten würde ich einfach entsprechende Methoden in die Klassen ”stopfen”, solange nicht geplant ist einen allgemeineren Im- und Export-Mechanismus im Programm zu haben. Mit der eigentlichen Aufgabe hat das schon zu tun wenn man das will. Im Grunde *ist* es ja die eigentliche Aufgabe im Moment, denn sonst hätte man ja, wie Du selbst schon sagst, einfach eine Liste nehmen können.

[Hyperion]

Naja, die Lade- und Speicher-Methoden in eine Funktion zu transformieren erhöht die Komplexität ja eigentlich nicht - zumal Du jeweils die Lade-Operation davon eh als Klassenmethode implementiert hast. KISS sehe ich damit irgend wie nicht gefährdet! Und anstelle einer Klasse dann eine Liste nehmen ist imho ja noch mehr KISS

In Python 3 sähe das z.B. so aus:

Code: Alles auswählen

import csv
from datetime import datetime as DateTime
 
ISO_TIME_FORMAT = '%Y-%m-%dT%H:%M:%S'
CSV_DELIMITER = ';'

 
class Appointment(object):
   
    def __init__(self, timestamp, subject, description):
        self.timestamp = timestamp
        self.subject = subject
        self.description = description
 
    def __str__(self):
        return (
            '{0.timestamp:%Y-%m-%d %H:%M} {0.subject} - {0.description}'.format(
                self
            )
        )
 
def to_row(appointment):
    return [
        format(appointment.timestamp, ISO_TIME_FORMAT),
        appointment.subject,
        appointment.description
    ]

def from_row(row):
    timestamp, subject, description = row
    return Appointment(
        DateTime.strptime(timestamp, ISO_TIME_FORMAT), subject, description
    )
  

def save_csv(appointments, filename):
    with open(filename, 'w', newline='') as csv_file:
        writer = csv.writer(csv_file, delimiter=CSV_DELIMITER)
        writer.writerows(to_row(appointment) for appointment in appointments)


def load_csv(filename):
    with open(filename, 'r') as csv_file:
        reader = csv.reader(csv_file, delimiter=CSV_DELIMITER)
        return [from_row(row) for row in reader]
 
 
def print_calendar(calendar):
    appointment_count = len(calendar)
    print(
        '{0} appointment{1}.'.format(
            appointment_count, '' if appointment_count == 1 else 's'
        )
    )
    for appointment in calendar:
        print(appointment)
 
 
def main():
    calendar = []
    calendar.extend(
        [
            Appointment(
                DateTime(2014, 11, 11, 11, 11),
                'Hellau',
                'Hellau rufen.'
            ),
            Appointment(
                DateTime(2014, 12, 12, 12, 30),
                'Arbeiten',
                'Mittagessen kochen.'
            ),
        ]
    )
    print_calendar(calendar)
    save_csv(calendar, 'test.csv')
    another_calendar = load_csv('test.csv')
    print_calendar(another_calendar)
 
 
if __name__ == '__main__':
    main()

[BlackJack]

@Hyperion: Finde ich nicht so gut weil die Funktionen IMHO enger zu den Klassen gehören.