Mit Containerelement meine ich ein Element das alle anderen enthält, so dass man nur dieses eine Element austauschen braucht.
Um Code und GUI zu trennen solltest Du am besten mal die ganze Logik ohne die GUI programmieren. Einlesen der CSV-Datei, das erstellen eines Benutzernamens usw. hat nichts mit GUI zu tun. Einiges davon ist auch in einer Klasse fehl am Platz. Wenn self nicht benutzt wird, sollte man sich immer fragen, ob das überhaupt eine Methode sein sollte.
Der Quelltext ist mit Tabs eingerückt, statt mit vier Leerzeichen pro Ebene. Ausserdem gibt es ein paar Zeilen, die länger als 80 Zeichen sind.
Bei ``except`` sollte man immer konkrete Ausnahmen angeben, sonst wird dort wirklich alles "verschluckt". Beim Importieren würde ich die Ausnahmebehandlung einfach weglassen. Dann sieht man auch was genau fehlt.
Dann solltest Du das Programm mal nach Instanzvariablen absuchen, die keine sind. self.fileobject und self.filecontent in SchoolTool zum Beispiel. Wenn Attribute nur in einer einzigen Methode verwendet werden, sollten es keine Attribute sondern lokale Namen sein.
Das Einlesen der CSV-Datei ist recht umständlich. Datei-Objekte sind "iterable", d.h. man kann direkt in einer Schleife über die Zeilen iterieren. self.filecontent ist ein überflüssiger Zwischenschritt.
lines = open(self.filename)
self.userarray = [line.strip().split(';') for line in lines]
lines.close()Wenn Du bei Python auch so aktuell sein willst wie beim GUI-Toolkit, also nur das neueste voraussetzt, kannst Du in Python 2.5 auch das ``with``-Schlüsselwort aktivieren und verwenden. Zum aktivieren muss als erster Import ``from __future__ import with_statement`` im Quelltext stehen. Dann sieht das Einlesen so aus:
with open(self.filename) as lines:
self.userarray = [line.strip().split(';') for line in lines]Für CSV-Daten die aus Tabellenkalkulationen oder ähnlichem stammen, würde ich allerdings das csv-Modul benutzen. Das CSV-Format ist nämlich gar nicht so simpel.
SchoolTool.startoperation() ist zu lang. Das lässt sich sicher auf mehrere Funktionen aufteilen.
simulate lässt sich einfacher setzen, die Methode aus der GUI gibt doch schon einen Wahrheitswert zurück:
simulate = self.ui.checkBox.isChecked()
Und beim Testen dann nicht explizit mit 0 oder 1 vergleichen, sondern simulate einfach als Wahrheitswert verwenden.
Ob man die LDAP-Verbindung (self.conn) unbedingt an das Objekt binden muss, ist fraglich. Wenn die Verbindung fehlschlägt, dann wird das zwar an der GUI ausgegeben, aber der Code läuft einfach weiter!?
self.sambaAlgoBase und self.sambaSID sind eigentlich lokale Namen.
Mit cleardata und realnames werden zwei parallele Datenstrukturen aufgebaut deren Inhalt eigentlich zusammengehört. Zusammen mit der "Indexerei" die dann in der ``for``-Schleife folgt, ist der Quelltext viel unverständlicher als er sein müsste. Wenn man so etwas schreibt…
for i in range(len(container)):
do_something(container[i])…kann man sich in 99% der Fälle den Index i sparen und gleich über die Elemente von container iterieren:
for element in container:
do_something(element)Und konstante Indexe sollte man nach Möglichkeit vermeiden und stattdessen einen aussagekräftigen Namen verwenden. Die beiden temporären Listen würde ich ganz weglassen und die ``for``-Schleife so beginnen:
for user_class, surname, given_name in self.userarray:
username = create_username(given_name, surname)
surname = clear_name(surname)
given_name = clear_name(given_name)
userrname = '%s %s' % (surname, given_name)Wobei username und userrname viel zu ähnlich sind, als das sie keine Verwirrung stiften würden.
random.seed() muss man eigentlich nicht aufrufen. Das wird beim ersten Import des random-Moduls automatisch gemacht.
Bei createusername() hat der "Block" mit der ersten Schleife keinen Effekt, weil das Ergebnis von der nächsten Schleife wieder "überschrieben" wird.
Kompakter liesse sich die Funktion so schreiben:
def createusername(fn, sn):
fn_tmp = clearname(fn.split(' ', 1)[0].split('-', 1)[0])
sn_tmp = clearname(''.join(sn.split('-')))
return ('%s.%s' % (fn_tmp, sn_tmp)).lower()Bei clearname(), was auch eine Funktion sein sollte, gibt's eine ähnliche Schleife die keinen Effekt hat. Wenn ich das richtig verstanden habe, kann man diese Schleife mit einer Zeile ersetzen:
name = '-'.join(s.strip().capitalize() for s in name.split('-') if s) + '-'In checkstart() kann man ein wenig Quelltext einsparen, wenn man eine Schleife über Attribute und Meldungen und nur eine ``if``-Abfrage benutzt.
Die ganzen "Getter" in SchoolConfig kann man sich sparen. Das ist "unpythonisch".
