johan12345
commented
6 years ago Hallo,
entschuldige bitte die späte Antwort. Mir ist auch klar, dass der Parser an einigen Stellen in Unordnung geraten ist - einige Teile, wie z.B. der Untis-Parser, existieren seit über 5 Jahren in ähnlicher Form und wurden immer um kleine Korrekturen erweitert ohne dabei viel aufgeräumt zu werden. Eine Überarbeitung ist auf jeden Fall fällig, momentan bin ich neben der Vertretungsplan-App mit einem Umzug und meiner Masterarbeit aber so ausgelastet, dass ich das jetzt gerade nicht stemmen kann. Gedacht hatte ich außerdem daran, den Parser nach und nach von Java auf Kotlin umzustellen, eine deutlich elegantere, aber vollständig mit Java kompatible Sprache, die die App und der Server bereits zum Teil nutzen.
Gerne kannst du hier Vorschläge machen und Verbesserungen angehen - weil unsere App mit über 600 Schulen und der dazugehörige Server weiter wie bisher funktionieren sollten, muss aber natürlich sichergestellt werden, dass dabei keine Änderungen in der Funktionalität entstehen - die Unit Tests sind zwar vorhanden, aber noch längst nicht vollständig - und dass die nötigen Änderungen am Server, der die Library benutzt, um die Pläne einzulesen, klein bleiben.
Ich kommentiere hier unten mal deine einzelnen Punkte:
nicht mit JSONObject arbeiten --> es gibt mit GSON und Jackson genug Möglichkeiten der Serialisierung von JSON-Daten.
Hier gibt es zwei unterschiedliche Fälle, in denen momentan JSONObject eingesetzt wird:
- Feld
datainSubstitutionScheduleData-SubstitutionScheduleData-Objekte werden genutzt, um dem Parser Informationen über den einzulesenden Plan zu geben. Hier gibt es einige Eigenschaften, die immer benötigt werden (Typ des Plans - Lehrer oder Schüler, zu nutzende API-Klasse und Art der benötigten Zugangsdaten) und viele, die abhängig von der jeweiligen API-Klasse sind. Da die Konfiguration in unserer Datenbank als JSON gespeichert wird, ist hier die einzige Einschränkung, dass die Daten als JSON-Object serialisierbar sein müssen, daher die Nutzung von JSONObject. Die - vermutlich gute, aber kompliziertere - Alternative wäre, eine KlasseParserConfigurationund dann für jede API-Klasse eine eigene Unterklasse davon zu definieren, die dann mit Jackson serialisierbar ist. Die Jackson-spezifischen Dinge (Serializer/Deserializer/Modules/Mixins) sollte man dann in ein separates Modul auslagen, sodass der Parser auch ohne Dependency zu Jackson genutzt werden kann. - Einige weitere Nutzungen in den eigentlichen Parsern, wenn der Schulserver tatsächlich JSON benötigt oder zurückliefert (z.B. beim
IphisParser). Hier könnte man natürlich auch für jeden Antworttyp des Servers eine entsprechende Klasse definieren, diese per Jackson mit Inhalt füllen und dann von dort aus die Daten wieder in dieSubstitutionSchedule-Datenstruktur umzuwandeln.
strikte Trennung von Logik und Datenhaltung. --> das Projekt scheint mir nicht nach dem Domain-Modell aufgesetzt worden sein, deswegen wüsste ich nicht wieso z.B. eine SubstitutionSchedule Anwendungslogik enthalten sollte.
Neben Gettern und Settern, hashCode und toString hat SubstitutionSchedule noch die Filtermethoden und das addDay.
Wenn man die Filter auslagern will, müsste das irgendwie so eine Utilities-Klasse sein, was wiederum nichts mehr mit Objektorientierung zu tun hat und die public API weniger intuitiv macht (SubstitutionScheduleFiltering.filter(schedule, ...) statt schedule.filtered...) . In Kotlin könnte man das als Extension function machen, dann wäre es im Code getrennt, aber trotzdem genauso aufrufbar.
addDay stellt sicher, dass die Tage des Vertretungsplans immer sortiert sind, dass der Stand des Gesamtplans (lastChange) immer dem jüngsten Stand eines einzelnen Tags entspricht, und dass Tage mit gleichem Datum zusammengeführt werden (addOrMergeDay). Eleganter wäre es vielleicht, das über eine Unterklasse von List zu lösen, aber ich sehe nicht, wieso das aus der Datenstruktur herausgenommen werden sollte.
Trennung von Parser und HTTP-Schnittstelle --> es ist nicht die Aufgabe eines Parsers die Daten sich irgendwo herzuholen, das macht ein REST-Controller. Das in einer abstrakten Schicht zu verstecken ist architektonisch falsch
Weil jedes Vertretungsplansystem seine Daten unterschiedlich bereitstellt, ist es nötig, auch die HTTP-Schnittstelle je API-Klasse einzeln zu implementieren. Zum Teil müssen auch erst einzelne Dateien eingelesen werden, um herauszufinden, wo weitere Daten abzurufen sind (z.B. bei Untis Monitor-Plänen mit mehreren Seiten, die über HTML ´´-Tags umgeblättert werden). Somit ist es schwierig, das komplett zu trennen. Was aber wünschenswert wäre, ist die Möglichkeit der Parallelisierung von HTTP-Requests und evtl. sogar auch dem darauffolgenden Parsen in den Fällen, wo die eben unabhängig voneinander möglich sind und eben nicht erst das Ergebnis vom vorherigen abgewartet werden muss, um die URL für den nächsten zu kennen (also wenn zum Beispiel von vornherein eine Liste von URLs für mehrere Seiten des Vertretungsplans übergeben wurde).
Enums benutzen für API Typen
Hier meinst du das api-Feld von SubstitutionScheduleData, richtig? Hier könnte man eventuell sogar direkt eine Referenz auf die Klasse des zu nutzenden Parsers nehmen, statt hier ein neues Enum einzuführen. Da die SubstitutionScheduleData auf dem Server aber auch serialisiert in der DB gespeichert wreden, muss es aber entsprechend eine Jackson-Anbindung geben, die in der JSON-Darstellung daraus wieder Strings macht und umgekehrt.
Entfernen von Apache Commons Lang 3 --> die Bibliothek ist nicht mehr zeitgemäß, grade durch das Fehlen von Generics. Viele Zeilen Code können hier gespart werden indem man auf Spring Web/Spring Boot migriert.
Commons Lang habe ich hier nur für EqualsBuilder und HashCodeBuilder genutzt, und in einem Fall für die Funktion StringEscapeUtils.escapeHtml4. Spring ist ein Web-Framework, von daher verstehe ich gerade nicht, wie das diese Einsatzzwecke ersetzen kann. Davon abgesehen wäre das ein Wechsel von Pest (487 KB JAR für 3 genutzte Methoden) zu Cholera (908 KB für Spring-Boot plus viele Dependencies), besser wäre es wohl, zwei kleine Bibliotheken genau für diese beiden Zwecke zu finden. In bestimmten Fällen (bei "data classes") werden equals und hashCode in Kotlin sowieso hinfällig.
Entwicklung einer einfach erweiterbaren Datenstruktur für den Parser --> Die Daten scheinen momentan einfach nur mit Regex Ausdrücken rausgeparst und in Objekten weggespeichert zu werden. Was ist aber wenn man diese mal erweitern muss? Dann steigt man kaum noch durch was wohin gehört.
In den meisten Fällen basieren die Parser nicht auf Regex, sondern JSoup (für die Vertretungspläne, die aus HTML- oder XML-Dateien eingelesen werden). Der Witz am substitution-schedule-parser ist gerade, dass er die verschiedenen Vertretungspläne in ein gemeinsames Datenformat (also die SubstitutionSchedule-Klasse) übersetzen kann. Natürlich werden nicht bei jedem Plan alle möglichen Felder in Substitution ausgefüllt, das hängt immer davon ab, welche Software die Schule nutzt (also welche API-Klasse dann zum Einlesen nötig ist) und welche Informationen die Schule evtl. ausgeblendet hat (z.B. aus Datenschutzgründen die Lehrernamen auf dem Plan für Schüler). Wenn irgendwann für eine neue API-Klasse ein neues Feld in Substitution benötigt werden sollte, kann das einfach hinzugefügt werden, da sehe ich jetzt kein großes Problem. Die App zeigt, abgesehen von der Detailansicht, allerdings immer nur Klasse (classes) bzw. Lehrer (teachers/previousTeachers), Stunde (lesson), Typ des Eintrags (type) und den über die SubstitutionTextUtils generierten Beschreibungstext an. In den meisten Fällen sollten neue Felder sollten entsprechend bei der Erzeugung des Beschreibungstextes berücksichtigt werden.
Viele Grüße, Johan
Ditscheridou
DreiDe
Hi,
ich hab mich die letzten Tage mal versucht in das Projekt einzuarbeiten und mir sind einige Sachen aufgefallen, die angegangen werden müssen, weil ansonsten die technischen Schulden bald nicht mehr vertretbar sind.
Ich würd das Refactoring gerne angehen, hab aber moment noch Probleme zu verstehen wie der Parser funktioniert, da die Unit Tests nicht wirklich aussagekräftig sind. Vielleicht könnt ihr mir da helfen?`
VG Didi