Style Fragen zu Backup-Programm mit json Anbindung

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
feldmaus
User
Beiträge: 284
Registriert: Donnerstag 12. Oktober 2006, 16:48

Guten Abend,

Ihr habt es geschafft, ich habe in mein Backup-Programm Json eingebaut statt Xml. Die Anbindung an Json ist auch kein Problem.

Da mein Code derzeit noch nicht so groß ist und bevor ich weiter mache, wollte ich Euch nochmal um Eure Meinungen zu meinem Style fragen. Ich habe versucht einige Sache aufzubrechen in kleinere Methoden, sodass ich sie Modular verwenden kann, wie auch die Lesbarkeit des Codes zu verbessern mit eindeutigeren Variablen.

Probleme habe ich auf jeden Fall mit der Doku und dem Zeilenumbruch. Ich habe zur Zeit viele manuelle Zeilenumbrüche und hätte gerne das Eric4 das für mich macht, habe aber keine Einstellung gefunden. Gleiches gilt für lange Code-Zeilen. Das ständige nach links und rechts Scrollen nervt mich und ist problematisch wenn ich den Code im Forum posten will. Gibt es hier einen Eric-Profi, der mir da einen Tipp geben kann?

backup.py

daten.json

Ich habe ein Projekt angelegt in Eric4 für dieses Programm und hätte gerne eine automatische Aktualisierung des Kopfes in meinem Source-Code. Also speziell das Aktualisierungs-Datum und die Versions-Nummer. Geht das in Eric4? Wie?

Grüße Markus
Benutzeravatar
darktrym
User
Beiträge: 784
Registriert: Freitag 24. April 2009, 09:26

Ich versuch's mal:
Bei Eingaben wo es nicht auf Groß-/Kleinschreibung ankommt, hätte ich vorher ein lower() durchgeschickt um die Auswahlfälle zu minimieren.
Einen Dok.-Hinweis, dass das Programm etwas unixartiges vorraussetzt(os.uname kennt mein Windows nicht).
Folgenden Code sieht mächtig verdächtig aus: "wasWillstDu in 'aAqQ' and wasWillstDu not in ''", wenn 1. Bedingung wahr ist, wie kann 2. dann nicht wahr werden.
Ein paar Namen sind etwas komisch gewählt: Es heißt Textmodus und warum nennst du logins benutzer_passwoerter.

Sind sicher noch ein paar mehr Ungereimtheiten, muss aber jetzt frühstücken.
„gcc finds bugs in Linux, NetBSD finds bugs in gcc.“[Michael Dexter, Systems 2008]
Bitbucket, Github
EyDu
User
Beiträge: 4881
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Hallo.

Hast du deinen Code eigentlich getestet? Die Stellen

Code: Alles auswählen

wasWillstDu == ('s' or 'S')
machen ganz sicher nicht, was du möchtest. Ich führe mal noch ein paar Dinge auf, die du verbessern solltest:
- Einhaltung von PEP 8; besonders Namen und maximale Zeilenlänge
- Wenn du mit Dateien arbeitest, solltest du das with-Statement benutzen
- Bei `speichereDatei` und `erstelleDatei` sollte zumindest noch ein `pass` stehen oder ein `NotImplementedError` geworfen werden.
- Du solltest an deinen Namen arbeiten. `reAusdruck` oder `variable` sagen nichts aus.
- Setze Strings nicht mit + zusammen, sondern benutze String Formatting
- ein `except Exception` ist eine schlechte Idee, da du damit alle Fehler abfängst. Du solltest nur die behandeln, die du auch behandeln kannst.
- Wenn du schon Exceptions abfängst, dann gehe mit diesen richtig um und gib nicht nur eine Nachricht aus. Nach einer Behandlung sollte sich das Programm wieder in einem gültigen Zustand befinden, was bei dir nicht der Fall ist. Es wird dann einfach an einer anderen Stelle abschmieren.
- `== True` solltest du weglassen.
- In deinem Code befinden sich viel zu viele magische Zahlen und Strings. Daraus solltest du Konstanten machen.
- `Backup` sieht als Klasse überflüssig aus. Eigentlich ist es nur eine Aneinanderreihung von Funktionen.

Sebastian
Das Leben ist wie ein Tennisball.
feldmaus
User
Beiträge: 284
Registriert: Donnerstag 12. Oktober 2006, 16:48

darktrym hat geschrieben:Folgenden Code sieht mächtig verdächtig aus: "wasWillstDu in 'aAqQ' and wasWillstDu not in ''", wenn 1. Bedingung wahr ist, wie kann 2. dann nicht wahr werden.
ich arbeite Deine anderen Punkte nachher nochmal durch, einen Punkt kann ich sofort beantworten.

Code: Alles auswählen

>>> a=""
>>> if a in 'abc':
...     print True
... 
True
das sollte nicht passieren.

Mein Programm wollte ich Objekt-Orientiert aufbauen, da noch der Gui-Modus hinzukommt. Auch wenn es zur Zeit mehr nach einer Ansammlung von Funktionen aussieht.

Grüße Markus
EyDu
User
Beiträge: 4881
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Code: Alles auswählen

a and a in "abc"
ist auch zu einfach ;-)
Das Leben ist wie ein Tennisball.
BlackJack

@feldmaus:

Programm
========

Bei den Docstrings kann ein Editor vielleicht Hilfestellung leisten beim Umbruch, aber beim Code muss man das wahrscheinlich schon selber tun. Neben Zeilenumbrüchen kann man bei zu tief verschachtelten Konstrukten auch darüber nachdenken die in mehr Funktionen aufzuteilen um die Einrückungstiefe zu verringern. Bei den Docstrings würde ich persönlich auf reStructuredText als Auszeichnungssprache setzen, oder zumindest etwas wofür es Werkzeuge gibt, die das interpretieren können. Also zum Beispiel ”epytext” für `epydoc`. Wobei das auch reStructuredText verarbeiten kann.

Neben den langen Zeilen erschwert immer noch das aneinanderkleben von Operatoren die Lesbarkeit. Um binäre Operatoren sollten Leerzeichen stehen. Ebenso um das ``=`` bei Zuweisungen ausserhalb von Argument- oder Parameterlisten.

Importe sollte man nicht in einer Zeile zusammen fassen. Es ist übersichtlicher pro Import eine eigene ``import``-Anweisung zu verwenden und die zu gruppieren nach Standardbibliothek, externe Module, und eigene Module. Und innerhalb der Module dann alphabetisch sortieren. Auf diese Weise sieht man besser wovon ein Modul abhängig ist und wenn man ein Modul hinzufügt, ist die Gefahr geringer das man es versehentlich mehrfach importiert. Die Trennung auf einzelne ``import``-Anweisungen und die Sortierung haben ausserdem Vorteile wenn mehrere Leute an einem Projekt arbeiten, weil es bei Veränderungen die Gefahr von Konflikten deutlich vermindert. Wenn zwei Leute unterschiedliche Module hinzufügen, würden sie das in unterschiedlichen Zeilen tun und wenn sie das gleiche Modul hinzufügen, geschieht das an der selben Stelle. So gibt es in der Regel keine nicht automatisch auflösbaren Konflikte und Module werden nicht mehrfach importiert. Das importierte `glob` wird übrigens nicht verwendet.

Die Klasse vermischt Benutzerinteraktion und Programmlogik miteinander. Das sollte man vermeiden. Man kann so die Benutzerinteraktion nicht unabhängig verändern, zum Beispiel um Entscheidungen per Programm zu treffen anstelle den Nutzer zu fragen, oder eine GUI oder eine Webschnittstelle auf die Programmlogik aufzusetzen. Die `Backup`-Klasse sollte nur die Sicherungslogik enthalten und über eine UI-Klasse vom Benutzer gesteuert werden. Damit würde auch so ein Argument wie `interface` in der `Backup.__init__()` entfallen. Welche UI man bekommt würde komplett in einer Klasse gekapselt.

Explizite Vergleiche auf literale `True` und `False` sind überflüssig. Ein ``if condition == True:`` ist das gleiche wie ``if condition:``; nur unnötig länger und aufwändiger.

Das Laden der JSON-Datei ist keine Methode sondern eine Funktion. Sollte man also entweder aus der Klasse heraus ziehen oder eine echte Methode daraus machen. Gleiches gilt für `variablenInEintragErsetzen()`.

Die Dokumentation von `variablenInEintragErsetzen()` entspricht nicht ganz der Wahrheit. Die Variablen werden nicht durch Plus-Zeichen begrenzt, sondern durch rückwärts gerichtete Schrägstriche und Plus (``\+``). Da scheint die Implementierung der Ersetzung durch. Ein ``for i in xrange(len(obj)):`` ist in 99% der Fälle unnötig kompliziert. Wenn man den Index trotzdem zusätzlich benötigt, sollte man die `enumerate()`-Funktion verwenden. Wie man da auf die Idee kommt die `__len__()`-Methode direkt aufzurufen, statt die dafür vorgesehene Funktion zu verwenden, ist mir auch ein Rätsel. Statt den zu ersetzenden Ausdruck erst zu suchen und wenn er gefunden wurde zu ersetzen, kannst Du auch *gleich* ersetzen. Wenn der Ausdruck nicht enthalten ist, dann findet halt keine Ersetzung statt, aber es muss nur einmal gesucht werden. An der Stelle würde ich aber sowieso etwas fertiges wie `string.Template` verwenden, statt mir eine Variablenersetzung selber zu basteln.

In `starteTextModus()` werden Programmlogik und Benutzerinteraktion vermischt und die Funktion macht zu viel.

Den Ausdruck ``wasWillstDu not in ''`` hatten wir schon mal. Das ist unnötig kompliziert und nicht jeder versteht auf Anhieb was da eigentlich geprüft werden soll.

Das Zählen von ``@``-Zeichen in `starteDatensicherung()` durch suchen des regulären Ausdrucks '@' in jedem einzelnen Zeichen einer Zeichenkette geht ja kaum umständlicher. Schau Dir bitte mal die Methoden auf Zeichenketten an. Es gibt da eine ganz Einfache zum Zählen von Vorkommen von Substrings.

Die Namen `haeufigkeitVonAtInServer` und `haeufigkeitVonAtInClient` werden in der Funktion zwar definiert, aber dann nicht verwenden.

An der Stelle fällt IMHO auch auf, dass die Bezeichnungen „Client” und „Server” in den Namen und in der Konfiguration nicht besonders gut gewählt sind. Die Begriffe hängen zu sehr von der Betrachtungsweise ab. Allgemeiner, deutlicher und passender wären „Quelle” und „Ziel”.

Rückgabe von Tupeln mit einem Fehlerwert, der darüber entscheidet ob der andere Wert im Tupel von Bedeutung ist oder nicht, ist ein ”code smell”. An der Stelle sollte man besser mit Ausnahmen arbeiten.

"Flag"-Argumente wie `wasWillstDu`, bei denen eine Funktion je nach Flag etwas semantisch anderes tut, sollte man vermeiden. Sichern und Wiederherstellen sind zwei unterschiedliche Dinge. Wenn die konkrete Implementierung viele Gemeinsamkeiten hat, kann man die ja in eine Hilfsfunktion/-Methode heraus ziehen. `localRemoteSync()` enthält auch zu viel ”kopieren und einfügen”-Quelltext. Die beiden Zweige für das Sichern oder Zurück spielen der Daten unterscheiden sich ja nur an einer Stelle. Die Funktion ist auch wieder genau das: eine Funktion und keine Methode.

Insgesamt würde ich, zumindest so wie es dort steht, den Sinn der Klasse in Frage stellen. Das könnte man auch mit Funktionen implementieren. Wenn Klassen, dann würde ich eher eine `Backup`-Klasse schreiben, die genau *einen* Eintrag repräsentiert, mit `sichern()`- und `zurueckSpielen()`-Methoden. Und eine `Backups`-Klasse als Container für solche `Backup`-Exemplare. Anstelle von einer `Backup`-Klasse würde es dann vielleicht am Ende auf mehrere Untertypen davon hinaus laufen, für die verschiedenen Arten (local→local, local→remote, …).


Konfigurationsdaten
===================

Das Format der JSON-Datei sollte man dokumentieren. Vielleicht im Moduldocstring oder einem extra Dokument.

In der JSON-Datei passiert ein bisschen zu viel über Indexpositionen in einer Liste. Das ist unübersichtlich und fehleranfällig. Ich sehe zum Beispiel nicht warum das gesamte Objekt eine Liste ist, oder warum in der Liste mit den Konfigurationsdaten der Eintrag an Index 0 etwas anderes darstellt als der Rest. In Listen sollte man möglichst nur gleichartige Daten zusammen fassen. `LetzteEinstellung` sieht mir auch ein wenig danach aus, als wenn man hier lieber ein Versionsverwaltungssystem für die Datei verwenden sollte, anstatt aus mehreren Konfigurationen über einen Index auszuwählen.

Die Daten enthalten sehr viel Redundanz. Zum Einen über die verschiedenen Einträge hinweg. Das erste Element in `Exclude` ist immer '--exclude'. Zum Anderen auch innerhalb eines Eintrags.

Das `Client` und `Server` könnten auch einfach nur *eine* Zeichenkette sein. Du hast ja sowieso schon eine Variablenersetzung implementiert, also könnte man anstelle von ``"Client": ["/home/", "/eigenedateien/"]`` auch ``"Client": "/home/${BENUTZER}/eigenedateien/"`` schreiben und im Programm dann `${BENUTZER}` ersetzen lassen.

Und man sollte den Anwender nicht zwingen unnötige Daten anzugeben die möglichst nicht mit echten Werten kollidieren, aber welche sein könnten. Wenn nichts von der Sicherung ausgenommen werden soll, könnte man `Exclude` weglassen oder auf ``null`` setzen.

Das `Frage`-Attribut erscheint mir auch komisch. Das ist teilweise redundant und enthält UI-spezifische Ausgaben.

So einen Eintrag könnte man letztendlich zum Beispiel auch so repräsentieren:

Code: Alles auswählen

"eigenedateien": {
    "Benutzer": ["markus", "maria", "bernard", "kaiserbert"],
    "Exkludieren": [],
    "Pfade": {
        "UnterPfad": "eigenedateien",
        "Quelle": "/home/${BENUTZER}",
        "Ziel": "${BENUTZER}@${SERVER}:/home"
    }
}
feldmaus
User
Beiträge: 284
Registriert: Donnerstag 12. Oktober 2006, 16:48

EyDu hat geschrieben:

Code: Alles auswählen

a and a in "abc"
ist auch zu einfach ;-)
ja Du denkst einfach zu einfach, das kann nicht gut sein. :mrgreen: Erledigt
feldmaus
User
Beiträge: 284
Registriert: Donnerstag 12. Oktober 2006, 16:48

BlackJack hat geschrieben:An der Stelle fällt IMHO auch auf, dass die Bezeichnungen „Client” und „Server” in den Namen und in der Konfiguration nicht besonders gut gewählt sind. Die Begriffe hängen zu sehr von der Betrachtungsweise ab. Allgemeiner, deutlicher und passender wären „Quelle” und „Ziel”.
Das sehe ich nicht so. Ein Server wird durch sein Funktion definiert und nicht durch den Standort des Betrachters. Wenn ich Jemanden Frage welcher sein Server zu Hause ist, dann hat diese person sofort ein klares Bild vor Augen. Ich weiß aber worauf Du hinaus willst. <Sichern> und <wiederherstellen> sind nicht wirklich aussagekräftig.
BlackJack hat geschrieben:"Flag"-Argumente wie `wasWillstDu`, bei denen eine Funktion je nach Flag etwas semantisch anderes tut, sollte man vermeiden. Sichern und Wiederherstellen sind zwei unterschiedliche Dinge. Wenn die konkrete Implementierung viele Gemeinsamkeiten hat, kann man die ja in eine Hilfsfunktion/-Methode heraus ziehen. `localRemoteSync()` enthält auch zu viel ”kopieren und einfügen”-Quelltext. Die beiden Zweige für das Sichern oder Zurück spielen der Daten unterscheiden sich ja nur an einer Stelle. Die Funktion ist auch wieder genau das: eine Funktion und keine Methode.
Also wäre nur die <ladeEinstellung>, <datenSicherung> und <systemSicherung> eine Methode der Klasse <Backup>? Den Rest würdest Du als Funktion umsetzen?

<starteTextModus> müsste raus und dafür eine eigene Klasse deklariert werden, z.b. <Interface> die als Methode <starteTextModus>, <starteGui> <übergebeFehler>, <neustarten>, <stoppen> hat. Wo würden dann die Texte gespeichert werden? Und eine Start-Datei namens <starteBackup.py> wo dann das Interface instanziert wird?
BlackJack hat geschrieben:Das `Client` und `Server` könnten auch einfach nur *eine* Zeichenkette sein. Du hast ja sowieso schon eine Variablenersetzung implementiert, also könnte man anstelle von ``"Client": ["/home/", "/eigenedateien/"]`` auch ``"Client": "/home/${BENUTZER}/eigenedateien/"`` schreiben und im Programm dann `${BENUTZER}` ersetzen lassen.]
Ja aber die benötigt eine feste Struktur. Und wenn ich vom Server zum Client übertragen will, dann muss ich target und source Argumente bei der Übergabe zu rsync tauschen. Deshalb habe ich Client und Server getrennt. Abgesehen davon liegen mir die Benutzer als Liste vor und bei der Variablenersetzung benötige ich ein Schlüssel-Werte-Paar.

Da ich ja mit diesem Programm vorwiegend im Textmodus arbeiten will, wollte ich mal fragen ob Jemand einen GUI-Designer für den Textmodus kennt?

Zu den Exceptions. Ich versuche gerade ein paar Exceptions auszuarbeiten. Wenn es den Ordner nicht gibt versucht er einen anzulegen, allerdings fehlt auch der übergeordnete Ordner, was nun? Die Fehlermeldung lautet:

Code: Alles auswählen

rsync: mkdir "/home/markus/.mozilla/firefox" failed: No such file or directory (2)

rsync error: error in file IO (code 11) at main.c(595) [Receiver=3.0.7]

rsync: connection unexpectedly closed (9 bytes received so far) [sender]

rsync error: error in rsync protocol data stream (code 12) at io.c(601) [sender=3.0.8]
Wie lautet jetzt die Exception? und wie bekomme ich rsync dazu rekursiv Ordner anzulegen?

Grüße Markus
BlackJack

@feldmaus: Was einen Server definiert oder nicht ist doch für das Backup-Programm völlig irrelevant. Da gibt es für Sicherungen Quellen und Ziele. Und beides können in anderen Kontexten Server oder Clients sein. Gerade weil jemand bei dem Begriff ”Server” ein bestimmtes Bild vor Augen haben könnte, ist das als Bezeichnung für die Quelle einer Sicherung (oder das Ziel) eine falscher Name.

Und es kommt sehr wohl auf den Standort des Betrachters an was Client und was Server ist. Wenn ich einen entfernten Rechner mit Webserver habe und dort mit einem Browser drauf zugreife, und gleichzeitig eine Remote-Verbindung mit X zu dem Rechner unterhalte, dann sind beide Rechner sowohl Server als auch Clients. Beides wäre aber für ein Backup-Programm völlig irrelevant, dem müsste ich sagen von welchem Rechner auf welchen anderen gesichert werden soll — also Quelle und Ziel.

Warum Du für die Benutzer-Ersetzung Schlüssel-Wert-Paare brauchst verstehe ich nicht? Du hast eine Liste von Benutzern und die kann man doch der Reihe nach für den Platzhalter einsetzen. Wo brauchst Du da Schlüssel und welche!?
feldmaus
User
Beiträge: 284
Registriert: Donnerstag 12. Oktober 2006, 16:48

BlackJack hat geschrieben:arum Du für die Benutzer-Ersetzung Schlüssel-Wert-Paare brauchst verstehe ich nicht? Du hast eine Liste von Benutzern und die kann man doch der Reihe nach für den Platzhalter einsetzen. Wo brauchst Du da Schlüssel und welche!?
Also ich habe meine Funktion zum ersetzen der Variablen jetzt so umgebaut:

Code: Alles auswählen

    def eintragAendern(self, eintrag, varUndWert):
        """Auf Grund der technischen Begrenzung durch die Datenbank-Datei, 
        werden als Variablen gekennzeichnete Ausdruecke (Kennzeichnung erfolgt
        durch Grossschreibung und Begrenzung durch Plus-Zeichen) in dem einge-
        lesenen Objekt <eintrag>, durch diese Methode mit ihren endgueltigen 
        Werten ersetzt.
        
        Beispiel fuer uebergebene Argumente:
        varUndWert = {"+SERVER+":"feld-server",
                      "+CLIENT+":"feld-bert"}"""
        
        for var in varUndWert:
            for key in eintrag:
                for i, zeichen in enumerate(eintrag[key]):
                    eintrag[key][i] = zeichen.replace(var, varUndWert[var])
        return eintrag
Also ich weiß nicht ob wir gerade aneinander vorbei reden. Willst Du meine Liste mit den Benutzern lequidieren? :-) Die wollte ich verwenden zum iterieren von mehreren Sicherungen, also für jeden Benutzer der in der Liste ist.

Quelle und Ziel anstatt Server und Client zu schreiben würde nicht mehr passen sobald ich die Richtung von rsync drehe, denn das soll mein Programm auch können. Besser wäre Dest1 und Dest2.

Grüße Markus
Benutzeravatar
mkesper
User
Beiträge: 919
Registriert: Montag 20. November 2006, 15:48
Wohnort: formerly known as mkallas
Kontaktdaten:

feldmaus hat geschrieben:Quelle und Ziel anstatt Server und Client zu schreiben würde nicht mehr passen sobald ich die Richtung von rsync drehe, denn das soll mein Programm auch können. Besser wäre Dest1 und Dest2.
Grüße Markus
Öhm, das ist Quatsch.
Du willst doch deine User nicht unnötig verwirren. Es gibt nur zwei sinnvolle Richtungen:

Code: Alles auswählen

backup Daten [nach] Sicherung
und

Code: Alles auswählen

restore Daten [aus] Sicherung
EDIT: Wortteil "Quelle" passt nicht, entfernt.
Antworten