Da wir Reviewen sollen, und ich einen kleinen Bug im Kunden-Tab fixen wollte, habe ich angefangen etwas zu refactoren / aufzuräumen, dachte ich mir, ich mache gleich unser erstes Review daraus. Das Meiste ist schon angepasst.

Nach Absprache sollen wir auf das Reviewclipse verzichten, da es zumindest bei mir und dem Manuel nur so lange läuft, bis man es verwenden will.



Review: Kunden anlegen
- Einrückungen waren absolut kaputt. 4 Leerzeichen, und dann um etwas weiter einzurücken ein einziger Tab. Ich habs nebenbei gefixt, um den Bug fixen zu können, den ich fixen wollte. Anders herum konnte ich es mit meinen Settings nicht lesen.

- Die Hierarchie des Widgets ist merkwürdig:

AbstraktKundeDialog
| -- KundeAnlegenDialog
| -- KundeBearbeitenDialog

Die Unterschiede zwischen den Klassen sind sehr gering bzw. nicht wirklich notwendig. Was hätte dagegen gesprochen, wenn das ganze ein Widget wäre, welches einfach immer einen übergebenen Kunden rendert? Bei einem neuen übergibt man einfach eine frisch-erstellte Klasse, und man hat nachher den geänderten Kunden. Insofern man keinen setzt, geht man immer von einem leeren Kunden aus.

Die Kopplung der instanzierbaren Klassen zu ihrem Parent ist viel zu stark ausgefallen. Warum müssen diese Klassen z.B. vollen Zugriff auf alle Input-Fields haben? Diese Widgets gehen niemanden etwas an. Um so weniger die Kinder wissen, um so einfacher ist es, das Basic-Widget zu editieren. Der Rest braucht einfach nur getter und setter zum Inhalt.

Du greifst z.B. auch mehrfach auf die Select-Boxes zu, die ja eigentlich Enums repräsentieren, und rechnest dir dann den passenen Wert aus. Das wurde auf mehreren Stellen gemacht. Mit dem Setter wird das ganze schon viel angenehmer anpassbar.

Das Handling der Combo-Boxes bez. dem "Wie kommen meine Daten da rein?" waren bei beiden Enums anders: Bei den Gesclechtern wurden die Werte "inline" eingetragen, während die Länder alle Ländernamen im Enum gepunkert haben, die dann per Foreach hinein gepackt wurden. Genau so wurden die Werte aus den Strings aus an unterschiedlichen Orten ermittelt. Wenn ich so etwas bei einem Enum editiere, möchte ich daraus ableiten können, wie überall diese Aktionen durchgeführt werden. Erwartungshaltung macht es auch einfacher, sich an Dinge zu erinnern, wenn man den Code mal zwei, drei Monate nicht mehr angesehen hat. (Bald bin ich wieder da, valadoc :/)

Du machst mit abstrakten Methoden komische Dinge: Button-Names über abstrakte Methoden festzulegen ist komisch. Ich habe einen setter daraus gemacht, der in den Konstruktoren aufgerufen wird. Das macht den Code wesentlich "linearer" zu lesen und entspricht mehr der Erwartungshaltung.

Genau so wird der Task, der pro View unterschiedlich ist, über abstrakte Methoden ausgelagert. Wie oben schon gesagt, sollte das ganze mmn. ein Widget sein. Was ein Widget machen soll, gehört in die Listener ausgelagert. So herum sieht das ganze auch aus, wie ein herkömliches Widget.

Die Implementierung der Check-Methode war ziemlich italiänisch und tief verschachtelt und hat zu viel gemacht. Die Tiefe kann man leicht ändern, indem man den ersten Teil heraus splittet und dann per Method Guard arbeitet. (dh: if(blah) {return <whatever>; })

Bez. der GUI: Feste Werte für die Windows sind keine gute Idee, da Größe und Spacing Style-abhängig sind. Genau so haben die Enums alle keinen Default-Value. Insofern man hier ein leeres Feld hinzu fügt, wirkt es so, als hätte man die Wahl, ob man etwas einträgt, oder nicht. Und wenn man sich einmal entscheidet, nichts auszuwählen, bekommt man die Rechnung serviert. Default-Values sind da mmn. angebrachter. so herum ist es auch beim Edit eindeutiger, wenn man nicht aufeinmal ein leeres Feld auswählen kann, welches man nicht auswählen kann.