Best Practice Code refactern

Diskutiere Code refactern im Java Basics - Anfänger-Themen Bereich.
T

tommysenf

Java:
private void initTableSort() {
        boolean isAsc = buttonASC.isSelected();
        
        masterDataService.getBookings().sort((o1, o2) -> {
                 int comparism = 0;
                 if (radioId.isSelected()) {
                        comparism = o1.getId().compareTo(o2.getId());
                 } else if(radioBetrag.isSelected) {
                       comparism = Double.valueOf(o1.getAmount()).compareTo(Double.valueOf(o2.getAmount()));
                } ....
                return isAsc ? comparism : comparism *= -1
            });
        filteredTableView.setInput(masterDataService.getBookings());
    }
 
mrBrown

mrBrown

Du könntest die RadioButtons in einer ToggleGroup wrappen und den Comparator den RadioButtons jeweils als UserData mitgeben.

Dann bräuchte es nur einen Listener an der ToogleGroup, und den aktuellen Comparator bekämest du über ToogleGroup#getToogle#getUserData.
 
L

LimDul

Vielleicht noch mal eine etwas allgemeinere Anmerkung zu Refaktoring Vorgehen. Was meines Erachtens an der Stelle viel hilft sind Unit-Tests (bis hin zu Testdriven Development). Denn wenn man anfängt Unit-Tests zu schreiben, stellt man schnell fest:

* Ich komme an den Teil schlecht ran, den ich testen will
* Ich brauche riesig Setup-Aufwand um die Konstellation aufzubauen

Dann zerhackt man den Code in kleinere Einheiten (=mehr Methoden/mehr Klassen).

Beispiel auf deinen konkreten Code bezogen, was will man testen?

* Wenn ich Betrag aufsteigend sortieren will, wird korrekt sortiert. Was brauche ich da bei deinem Ursprungscode?
** Ich muss alle Radiobuttons erzeugen
** Ich muss den korrekten Radiobutton auf selected setzen
** Ich brauche einen masterDataService
** Ich brauche eine TableView

So, das ist extrem viel Aufwand - nur um zu testen, ob die Sortierung klappt.

Zerhackt man das in mehrere Teilprobleme:

* Ich ermittle aus den Radio-Buttons den Comparator (kann ich separat testen)
* Ich kann die Comparatoren separat testen
* Ich kann dann testen, ob die Übertragung aus dem masterService in die Sortiermethode klappt
* Ich kann dann testen, dass die Rückgabe aus der Sortiermethode in die TableView klappt

Insgesamt habe ich da über alle Tests sogar weniger Setup-Aufwand als bei dem Test deiner Methode.

Man kommt bei dem Vorgehen oft zu Stellen, wo man - nur um einen Aspekt separat zu testen - entscheidet Teile in eigene Methoden auszulagern.
 
L

lam_tr

Du könntest die RadioButtons in einer ToggleGroup wrappen und den Comparator den RadioButtons jeweils als UserData mitgeben.

Dann bräuchte es nur einen Listener an der ToogleGroup, und den aktuellen Comparator bekämest du über ToogleGroup#getToogle#getUserData.
Stimmt, das ist richtig einfach :)
 
L

lam_tr

Vielleicht noch mal eine etwas allgemeinere Anmerkung zu Refaktoring Vorgehen. Was meines Erachtens an der Stelle viel hilft sind Unit-Tests (bis hin zu Testdriven Development). Denn wenn man anfängt Unit-Tests zu schreiben, stellt man schnell fest:

* Ich komme an den Teil schlecht ran, den ich testen will
* Ich brauche riesig Setup-Aufwand um die Konstellation aufzubauen

Dann zerhackt man den Code in kleinere Einheiten (=mehr Methoden/mehr Klassen).

Beispiel auf deinen konkreten Code bezogen, was will man testen?

* Wenn ich Betrag aufsteigend sortieren will, wird korrekt sortiert. Was brauche ich da bei deinem Ursprungscode?
** Ich muss alle Radiobuttons erzeugen
** Ich muss den korrekten Radiobutton auf selected setzen
** Ich brauche einen masterDataService
** Ich brauche eine TableView

So, das ist extrem viel Aufwand - nur um zu testen, ob die Sortierung klappt.

Zerhackt man das in mehrere Teilprobleme:

* Ich ermittle aus den Radio-Buttons den Comparator (kann ich separat testen)
* Ich kann die Comparatoren separat testen
* Ich kann dann testen, ob die Übertragung aus dem masterService in die Sortiermethode klappt
* Ich kann dann testen, dass die Rückgabe aus der Sortiermethode in die TableView klappt

Insgesamt habe ich da über alle Tests sogar weniger Setup-Aufwand als bei dem Test deiner Methode.

Man kommt bei dem Vorgehen oft zu Stellen, wo man - nur um einen Aspekt separat zu testen - entscheidet Teile in eigene Methoden auszulagern.
Ja ich weiß was du meinst, ich versuch mal so langsam vor zu gehen, danke!
 
MoxxiManagarm

MoxxiManagarm

Aber ich glaube ich mache für jeden Control einen eigenen Listener und gebe dort die Sortierung mit, wie du weiter oben schon erwähnt hast.
Wie auch immer du dich letztlich entscheidest, der Grundgedanke läuft in jedem Ansatz darauf hinaus deiner initTableSort Methode mit Hilfe eines Comparators nur noch zu sagen was sie sortieren soll. Du definierst hier immer nur den Ascending Comparator, welcher durch comparator.reversed() bei Bedarf (Descending) umgedreht werden kann. Dadurch nimmt die Komplexität an der Stelle deutlich ab.
 
L

LimDul

Ja ich weiß was du meinst, ich versuch mal so langsam vor zu gehen, danke!
Das ist auch ein Lernprozess, der nie aufhört. Ich hab mittlerweile auch über 10 Jahre Berufserfahrung auf dem Buckel - und just diese Woche eine Review Anmerkung von einem Kollegen bekommen. Da ging es um einen Dialog, wo ich nur einen Test für den "Cancel" Button hatte. Da kam die Anmerkung "Da fehlt ein Test für den OK-Button". Ja, den hatte ich versucht zu schreiben, aber ging halt nicht. Aber dann halt noch mal rangesetzt und aus der Methode, die den Dialog baut zwei gemacht - eine public, eine protected und die Protected so geschnitten von den Parametern, dass ich die Konstellation sinnvoll testen konnte.
 
L

lam_tr

Das ist auch ein Lernprozess, der nie aufhört. Ich hab mittlerweile auch über 10 Jahre Berufserfahrung auf dem Buckel - und just diese Woche eine Review Anmerkung von einem Kollegen bekommen. Da ging es um einen Dialog, wo ich nur einen Test für den "Cancel" Button hatte. Da kam die Anmerkung "Da fehlt ein Test für den OK-Button". Ja, den hatte ich versucht zu schreiben, aber ging halt nicht. Aber dann halt noch mal rangesetzt und aus der Methode, die den Dialog baut zwei gemacht - eine public, eine protected und die Protected so geschnitten von den Parametern, dass ich die Konstellation sinnvoll testen konnte.
Ja das sind die Erfahrungen die man mit sich nimmt. Es ist echt wertvoll wenn man hier auf dem guten Wege geleitet wird, solche Dinge kann man nur gut verstehen wenn man das veranschaulicht bekommt.
 
L

lam_tr

Du könntest die RadioButtons in einer ToggleGroup wrappen und den Comparator den RadioButtons jeweils als UserData mitgeben.

Dann bräuchte es nur einen Listener an der ToogleGroup, und den aktuellen Comparator bekämest du über ToogleGroup#getToogle#getUserData.
Das gefällt mir sehr.

Code:
ToggleGroup tg = new ToggleGroup();
        tg.selectedToggleProperty().addListener(new ChangeListener<Toggle>() {
            @Override
            public void changed(ObservableValue<? extends Toggle> observable, Toggle oldValue, Toggle newValue) {
                if (newValue!=null) {
                    Object userData = newValue.getUserData();
                    System.out.println(userData);
                }
            }
        });
        
        radioId.setToggleGroup(tg);
        radioId.setUserData("ID");
        radioKonto.setToggleGroup(tg);
        radioKonto.setUserData("Konto");
        radioBetrag.setToggleGroup(tg);
        radioBetrag.setUserData("Betrag");
        radioDatum.setToggleGroup(tg);
        radioDatum.setUserData("Datum");
Ich habe nur auf die Schnelle getestet, da sollen die Comparatoren rein :)

@MoxxiManagarm
Jetzt ist es mir wesentlich klarer, danke dir auch.
 
MoxxiManagarm

MoxxiManagarm

Ich habe als Neuling übrigens auch meistens globale Listener verwendet, allerdings waren das auch alles mini-Projekte ohne Komplexität und noch vor Java 8 Lambdas. Besonders die Lambdas haben viel vereinfacht an der Stelle. Mit steigender Erfahrung weiß man bestimmte Dinge halt zu schätzen und würde es nie wieder anders machen wollen. Natürlich muss man diesen Dingen erstmal begegnen.
 
Thema: 

Code refactern

Passende Stellenanzeigen aus deiner Region:
Anzeige

Neue Themen

Anzeige

Anzeige
Oben