Sample-Sourcen ?

[routeconverter]

Hallo Thomas,

ich bin auch kein Freund, solche Diskussionen in einem Chat oder Forum zu führen – interaktiv kann man da besser abwägen, was die beste Option sein könnte.

Ich möchte lieber erst versuchen, die Abhängigkeit von PositionsModelImpl zum PositionHelper zu löschen. Es fühlt sich irgendwie falsch an, dass in einem Model Wissen über die Darstellung und Formatierung der Daten vorhanden ist. Was spricht dagegen, editCell() und getStringAt() herauszuziehen – und damit die Abhängigkeit von PositionsModelImpl auf PositionHelper zu lösen. Irgendwie haben ja auch die parseXXX() Methoden in einem Model wenig verloren und es würde die Idee des PositionsModelCallback weiter treiben.

Dann würde der von Dir geplante UnitTest gegen die Implementierung des Callbacks laufen. Damit wäre das PositionsModelImpl aus dem Spiel.

In einem zweiten Schritt könnte man dann in ParseHelper die Aufrufe wie RouteConverter.getInstance().getTimeZone() durch hineingereichte Models ersetzen, was Du ja bereits vorgeschlagen hast. Wenn ich mir den Code anschaue, dann sehe ich viele verteilte Codestellen, wo der PositionHelper aufgerufen wird. Es wäre wohl damit besser gewesen, ein kleines Dependency Injection Framework zu verwenden... ;-) Aber nicht alle Stellen brauchen die Abhängigkeit auf die hineingereichten Models sondern m.E. nur die, die aus editCell() und getStringAt() resultieren. Die könnte man aus RouteConverter-Klasse herauslösen (und es weniger zu einer Gottklasse machen könnte).

Konntest Du mir folgen?

[routeconverter]

Schritt eins war einfacher als gedacht:
https://github.com/cpesch/RouteConverter...a32c3f39c3

[lundefugl]

Eine Trennung der (ich nenne es mal so) "Arbeitslogik" vom Modell ist sicher nicht falsch. Für Unittests ist das durchaus etwas besser handhabbar.

Das eigentliche Hauptproblem sind die statischen Methoden im PositionHelper, die du als zweiten Schritt angesprochen hast. Wie du dir da die Lösung 100%ig vorstellst, kann ich mir noch nicht ganz vorstellen. Da du aber auch dort die Abhängigkeit auf RouteConverter.getInstance() auflösen wisst, ist das vermutlich ein richtiger Weg.

Gruß
Thomas

[routeconverter]

Ein bisschen Code sagt vielleicht mehr als endloses Getippe:
https://github.com/cpesch/RouteConverter...ion-helper

Bei DateTime war es einfacher, das ganz aus dem PositionHelper herauszulösen, denn es gab nur 2 Verwendungen
* DateTimeColumnTableCellEditor
* PositionsModelCallbackImpl

Ich habe das erstmal auf einen Branch gepusht, da es nur für DateTime umgebaut ist und mir DateTimeColumnTableCellEditor#extractValue und PositionsTableCellEditor#formatLabel noch nicht gefallen

[routeconverter]

Ich mache erstmal Schluß für heute: DateTime, Date, Time sind jetzt umgebaut – und die Abhängigkeiten vom PositionsModelCallback auf PositionHelper und RouteConverter.getInstance deutlich kleiner. Es gibt noch formatTime() und formatDate() im PositionHelper, die bin ich noch nicht losgeworden, weil das die Download/Exif/Gps- Spalten betrifft.

Ich hoffe, das hilft Dir bei den UnitTests

[lundefugl]

Hallo Christian

danke für die Anpassungen. Ich habe es mir schon mal angeschaut. Das sieht sehr gut aus. Ich habe auch schon mal einen kleinen Test gebastelt, der einfach die Werte für die drei relevanten Spalten setzt und da kommt keine Nullpointer-Exception mehr. Das ist perfekt so für mich.
Da kann ich mir in den nächsten Tagen Gedanken zu den ganzen möglichen Varianten machen, die da so auftreten können.

Und danach kann ich schauen, dass die Logik entsprechend angepasst wird. Das ist ja dann auch nur im Callback notwendig. Da bin ich auch sehr froh drüber, da man so keine Angst haben muss, dass das irgendwelche Auswirkungen auf andere Stellen, außerhalb der Tabellen-Bearbeitung hat.

Morgen wird da aber wahrscheinlich wenig passieren. Da habe ich andere familiäre Verpflichtungen.

Gruß
Thomas

[routeconverter]

Danke fürs schnelle Feedback. Ich habe den Branch in den master gemerged und werde am Wochenende nochmal drauf schauen.

[lundefugl]

Hallo Christian

auf meinem Featurebranch zur Positionseingabe habe ich jetzt den Unittest und auch die Logikanpassung umgesetzt. Lokal funktioniert das auch alles super.
(https://github.com/lundefugl/RouteConver...tion-times)

Mein Problem ist jetzt nur, dass bei dem globalen Build von Github scheinbar die Standard-Locale "US" ist. Und irgendwo vor meinem neuen Test wird nun schon ein Aufruf gemacht, so dass in slash.common.io.Transfer die statischen Formate mit der Default-Locale hochgezogen werden. Und die Datums- und Zeitdarstellungen sind damit nicht die, die ich erwarte.

Natürlich könnte ich jetzt alles auf das US-Format anpassen. Aber damit hat man dann das umgedrehte Problem, wenn man alle Tests miteinander bei sich lokal laufen lassen will.

Hast du eine Idee, wie man das in den Griff bekommen könnte ? Bzw. hattest du schon mal solche Probleme an anderen Stellen.

Gruß
Thomas

Nachtrag: Ich habe jetzt einen recht brutalen Hack gemacht, aber damit würde es gehen.

[routeconverter]

Mein Problem ist jetzt nur, dass bei dem globalen Build von Github scheinbar die Standard-Locale "US" ist.

Ich erinnere mich daran, dass ich damit bereits Probleme hatte bei den github Actions.

Und irgendwo vor meinem neuen Test wird nun schon ein Aufruf gemacht, so dass in slash.common.io.Transfer die statischen Formate mit der Default-Locale hochgezogen werden. Und die Datums- und Zeitdarstellungen sind damit nicht die, die ich erwarte.

Das ist vermutlich PositionsHelperTest#parseDateTime(String stringValue, String timeZonePreference) der Transfer#getDateTimeFormat(String timeZonePreference) aufruft?

Natürlich könnte ich jetzt alles auf das US-Format anpassen. Aber damit hat man dann das umgedrehte Problem, wenn man alle Tests miteinander bei sich lokal laufen lassen will.

Korrekt, das wäre keine Lösung.

Hast du eine Idee, wie man das in den Griff bekommen könnte ? Bzw. hattest du schon mal solche Probleme an anderen Stellen.

Ja, diese Probleme hatte ich auch. Der PositionHelperTest hat über parseDateTime() Seitenwirkungen auf die Transfer-Klasse. Dein neuer Test könnte denselben Weg versuchen:

CompactCalendar actualCal = parseDateTime(asDefaultLocaleTime("18.09.2010 03:13:33"), "UTC");

Nachtrag: Ich habe jetzt einen recht brutalen Hack gemacht, aber damit würde es gehen.

Du meinst Transfer#reinit() damit? Irgendwie unschön.

[lundefugl]

Hallo Christian,

Ja, diese Probleme hatte ich auch. Der PositionHelperTest hat über parseDateTime() Seitenwirkungen auf die Transfer-Klasse. Dein neuer Test könnte denselben Weg versuchen:

CompactCalendar actualCal = parseDateTime(asDefaultLocaleTime("18.09.2010 03:13:33"), "UTC");

Das wäre eine Variante. Allerdings würde das ein wenig den Test einschränken. In dem Test sollen eigentlich mögliche Eingaben vom Benutzer geprüft werden, auch wenn sie nicht 100%ig der Formatierung entsprechen. D.h. z.B. fehlende Nullen.
Wenn ich das erst im Test reformatiere, dann habe ich das im Testcode gemacht, aber ob das der Laufzeitcode korrekt macht, wird dann nicht geprüft.

Du meinst Transfer#reinit() damit? Irgendwie unschön.

Genau. Ich habe das jetzt mal etwas angepasst, so dass der Aufruf und die Prüfung nur noch intern wäre (ähnlich zu den Zeitzonen).
Was denkst du dazu ?

Eine andere Idee habe ich sonst nicht. Es wäre dann nur noch deine Variante aus PositionHelperTest, auf die ich den Test umbauen könnte.

Gruß
Thomas