@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"
}
}
Erledigt
Die wollte ich verwenden zum iterieren von mehreren Sicherungen, also für jeden Benutzer der in der Liste ist.