Best Practice Code refactern

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

lam_tr

Hallo zusammen,

wie würdet ihr diesen Code schöner schreiben, oft komme ich zu so ein Pattern und weiß halt nicht wie es besser gehen kann. Dieser Code soll Tabellenspalten sortieren, jenachdem welche Spalte selektiert ist. Es geht mir nur rein ums schöner schreiben.

Code:
    private void initTableSort() {
        boolean isAsc = buttonASC.isSelected();
        if (radioId.isSelected()) {
            masterDataService.getBookings().sort((o1, o2) -> {
                if (isAsc) {
                    return o1.getId().compareTo(o2.getId());
                }
                return o2.getId().compareTo(o1.getId());
            });
        } else if (radioBetrag.isSelected()) {
            masterDataService.getBookings().sort((o1, o2) -> {
                if (isAsc) {
                    return Double.valueOf(o1.getAmount()).compareTo(Double.valueOf(o2.getAmount()));
                }
                return Double.valueOf(o2.getAmount()).compareTo(Double.valueOf(o1.getAmount()));
            });
        } else if (radioDatum.isSelected()) {
            masterDataService.getBookings().sort((o1, o2) -> {
                if (isAsc) {
                    return o1.getBookDate().compareTo(o2.getBookDate());
                }
                return o2.getBookDate().compareTo(o1.getBookDate());
            });
        } else if (radioKonto.isSelected()) {
            masterDataService.getBookings().sort((o1, o2) -> {
                if (isAsc) {
                    return o1.getAccount().getName().compareTo(o2.getAccount().getName());
                }
                return o2.getAccount().getName().compareTo(o1.getAccount().getName());
            });
        } else if (radioTyp.isSelected()) {
            masterDataService.getBookings().sort((o1, o2) -> {
                if (isAsc) {
                    return o1.getBookingType().compareTo(o2.getBookingType());
                }
                return o2.getBookingType().compareTo(o1.getBookingType());
            });
        } else if (radioVorgang.isSelected()) {
            masterDataService.getBookings().sort((o1, o2) -> {
                if (isAsc) {
                    return o1.getProcess().getName().compareTo(o2.getProcess().getName());
                }
                return o2.getProcess().getName().compareTo(o1.getProcess().getName());
            });
        }else if (radioTaxRate.isSelected()) {
            masterDataService.getBookings().sort((o1, o2) -> {
                if (isAsc) {
                    return (int) (o1.getTaxRate()-o2.getTaxRate());
                }
                return (int) (o2.getTaxRate()-o1.getTaxRate());
            });
        }

        filteredTableView.setInput(masterDataService.getBookings());
    }
Grüße
lam
 
L

LimDul

Grundsätzlich gibt es bestimmt mehre Möglichkeiten. Das erste was mir ins Auge auffällt, ist der "lange" Lambda-Ausdruck. Ich versuche Ausdrücke der Form
Java:
(x) -> {
// Code
}
zu vermeiden, weil ich die schlecht lesbar halte und sie einfach enorm viel Platz aufnehmen.

Ich würde daher das erzeugen der Comparatoren und das sortieren trennen.

Erzeugen:
Code:
Comparator comparator = null;
if (radioId.isSelected()) {
comparator = Comparator.comparing(Booking:getId);
} else if (radioBetrag.isSelected()) {
comparator = Comparator.comparing(Booking:getAmount);
}
.// usw
if (!isAsc && comparator != null) {
comparator = comparator.reverse();
}
if (cmparator!=null) {
masterDataService.getBookings().sort(comparator);
}
Musst schauen, ob das erzeugen der Comparatoren grundsätzlich so geht (bei Amount bin ich mir nicht sicher, da das ggf. vorher noch in einen Double konvertiert werden muss)

Aber so finde ich das übersichtlicher.

Als weitere Ausbaustufe könnte man sich auch eine enum BookingSortKriterium überlegen und dann eine Methode getSortKriterium die je nach selektiertem RadioButton den entsprechenden Wert zurückliefert. Dann vereinfacht sich die if / else if Cascade in ein reines switch/case.

Als weiteren Schritt könnte man sich dann überlegen, das erzeugen der Comparatoren und das sortieren in eine eigene Klasse/Methode auszulagern, die als Eingabe bekommt:
* Die Liste
* Den Enum mit den Suchkriterium

Und die dann die Liste sortiert. Dann kann man die getrennt testen, die ist dann auch komplett unabhängig von der UI-Logik und im UI läge dann nur das Mapping radrioButtons => Enum und das auslesen/setzen der Liste.
 
L

lam_tr

Grundsätzlich gibt es bestimmt mehre Möglichkeiten. Das erste was mir ins Auge auffällt, ist der "lange" Lambda-Ausdruck. Ich versuche Ausdrücke der Form
Java:
(x) -> {
// Code
}
zu vermeiden, weil ich die schlecht lesbar halte und sie einfach enorm viel Platz aufnehmen.

Ich würde daher das erzeugen der Comparatoren und das sortieren trennen.

Erzeugen:
Code:
Comparator comparator = null;
if (radioId.isSelected()) {
comparator = Comparator.comparing(Booking:getId);
} else if (radioBetrag.isSelected()) {
comparator = Comparator.comparing(Booking:getAmount);
}
.// usw
if (!isAsc && comparator != null) {
comparator = comparator.reverse();
}
if (cmparator!=null) {
masterDataService.getBookings().sort(comparator);
}
Musst schauen, ob das erzeugen der Comparatoren grundsätzlich so geht (bei Amount bin ich mir nicht sicher, da das ggf. vorher noch in einen Double konvertiert werden muss)

Aber so finde ich das übersichtlicher.

Als weitere Ausbaustufe könnte man sich auch eine enum BookingSortKriterium überlegen und dann eine Methode getSortKriterium die je nach selektiertem RadioButton den entsprechenden Wert zurückliefert. Dann vereinfacht sich die if / else if Cascade in ein reines switch/case.

Als weiteren Schritt könnte man sich dann überlegen, das erzeugen der Comparatoren und das sortieren in eine eigene Klasse/Methode auszulagern, die als Eingabe bekommt:
* Die Liste
* Den Enum mit den Suchkriterium

Und die dann die Liste sortiert. Dann kann man die getrennt testen, die ist dann auch komplett unabhängig von der UI-Logik und im UI läge dann nur das Mapping radrioButtons => Enum und das auslesen/setzen der Liste.
Super klasse, genau sowas habe ich gesucht. Das mit dem Account geht nicht, man muss es zuerst konvertieren, wie? Und der zweite Ansatz mit dem Einpacken der Sortierung in eine eigene Klasse ist auch cool.
 
L

lam_tr

Ist das Konvertieren des Accounts hiermit gemeint

Code:
comparator = Comparator.comparing( t -> ((Booking)t).getAccount().getName());
Aber dann ist dieser Cast im Lamda nicht schön oder?
 
MoxxiManagarm

MoxxiManagarm

Ich kann dem Ansatz von LimDul kaum etwas hinzufügen. Einzig und alleine würde ich die if-elses noch entfernen, zumindest an dieser Stelle. Du hast Radiobuttons, d.h. es gibt keinen Multiselect. Ich würde auf die Änderung reagieren und jedem Radio-Button einen Value zuordnen. Dieser Value nenne ich jetzt einfach mal "selectedComparator". Du speicherst also den Comparator beim Event Radio Button Selection Changed.

Dann ist der Code zum Sortieren relativ straight forward:
Java:
Comparator<Booking> selectedComparator; // wird durch die Auswahl eines RadioButtons verändert

if (!buttonASC.isSelected()) {
  selectedComparator = selectedComparator.reversed();
}
masterDataService.getBookings().sort(selectedComparator);
 
L

lam_tr

Ich kann dem Ansatz von LimDul kaum etwas hinzufügen. Einzig und alleine würde ich die if-elses noch entfernen, zumindest an dieser Stelle. Du hast Radiobuttons, d.h. es gibt keinen Multiselect. Ich würde auf die Änderung reagieren und jedem Radio-Button einen Value zuordnen. Dieser Value nenne ich jetzt einfach mal "selectedComparator". Du speicherst also den Comparator beim Event Radio Button Selection Changed.

Dann ist der Code zum Sortieren relativ straight forward:
Java:
Comparator<Booking> selectedComparator; // wird durch die Auswahl eines RadioButtons verändert

if (!buttonASC.isSelected()) {
  selectedComparator = selectedComparator.reversed();
}
masterDataService.getBookings().sort(selectedComparator);
Sorry habe das noch nicht ganz verstanden wie du das mit Comparator in RadioButton verpacken meinst. Aber dein Ansatz wäre das entfernen der If/else oder?
 
MoxxiManagarm

MoxxiManagarm

Ja. Du könntest jedem RadioButton einen ActionListener ergänzen.
Also z.B.
Java:
radioId.addActionListener((ActionEvent e) -> selectedComparator = Comparator.comparingInt(Booking::getId));
Dann hast du an keiner Stelle ein riesiges if-else Konstrukt.
Die Sortierfunktion musst du dann nie wieder anfassen, wenn ein Feld hinzukommt. Wenn ein Feld hinzzukommt, welches du für de Sortierung anbietest, dann ergänzt du einfach wieder einen RadioButton der den entsprechenden Comparator als selected setzt.

Also das ActionEvent ist ein Beispiel für Swing. Du verwendest SWT? Da sieht es möglicher Weise etwas anders aus, aber ich denke das Prinzip ist klar.
 
Zuletzt bearbeitet:
L

lam_tr

Ja. Du könntest jedem RadioButton einen ActionListener ergänzen.
Also z.B.
Java:
radioId.addActionListener((ActionEvent e) -> selectedComparator = Booking::getId);
Dann hast du an keiner Stelle ein riesiges if-else Konstrukt.
Die Sortierfunktion musst du dann nie wieder anfassen, wenn ein Feld hinzukommt. Wenn ein Feld hinzzukommt, welches du für de Sortierung anbietest, dann ergänzt du einfach wieder einen RadioButton der den entsprechenden Comparator als selected setzt.
Interesssant, und wo trigger ich die Sortierung? Meine RadioButtons rufen ohnehin schon die #initTableSort() Methode auf
 
MoxxiManagarm

MoxxiManagarm

Dann könntest du selectedComparator nicht speichern und ihn stattdessen der initTableSort Methode als Parameter übergeben.

Also sinngemäß
Java:
radioId.addActionListener((Actionevent e) -> initTableSort(Comparator.comparingInt(Booking::getId)));

private void initTableSort(Comparator<Booking> comparator) {
 
L

lam_tr

Dann könntest du selectedComparator nicht speichern und ihn stattdessen der initTableSort Methode als Parameter übergeben.

Also sinngemäß
Java:
radioId.addActionListener((Actionevent e) -> initTableSort(Comparator.comparingInt(Booking::getId)));

private void initTableSort(Comparator<Booking> comparator) {
So geht das bei mir leider nicht. Ich habe es so implementiert

Code:
    public void onButtonAction(ActionEvent event) {
        Object source = event.getSource();
        if (source == buttonEinnahmenErstellen) {
           dispatchOnEinnahmeErstellen();
        }  else if (source == buttonNextDay) {
           ....
        } else if (source == buttonPreviousDay) {
           ....
        } else if (source == radioBetrag || source == radioDatum || source == radioId || source == radioKonto
                || source == radioTyp || source == radioVorgang || source == buttonASC) {
            initTableSort();
        }
    }
Ich weiß nicht ob das ein guter Ansatz ist, aber zumindest ist das eine zentrale Stelle für mich wenn ich nach Listener schauen will. Es wurde dann gehen wenn ich es ausplitte , dann wäre die if /else zweige hier anstatt in der initTableSort() Methode.
 
L

LimDul

Ich würde lieber jedem Button einen eigenen Listener spendieren (Das ist Java FX, oder? Da bin ich nicht so firm drin).

Grundsätzlich gilt: Keine Scheu vor mehr Klassen. Lieber aus einer langen Methode mehrere kleinere machen und dann aus einer Klasse mit vielen Methoden mehrere Klassen machen.

Bei dem Code oben würde eine statische Codeanalyse wie CheckStyle oder SpotBugs dir auf die Finger klopfen, weil die If-Bedingung viel zu viele Bedingungen hat.

Edit, Nachtrag:
Hier ist die Checkstyle Regel: https://checkstyle.sourceforge.io/a...metrics/BooleanExpressionComplexityCheck.html
Property max - Specify the maximum number of boolean operations allowed in one expression. Default value is 3.
 
MoxxiManagarm

MoxxiManagarm

Ich weiß nicht ob das ein guter Ansatz ist
Aus meiner Sicht nicht. Ich kann diesen Ansatz nicht leiden :D Das wird schnell unübersichtlich und fehleranfällig. Aber vor allem unflexibel, wie du nun siehst. Ich mag selbst halt auch keine großen if-else Konstrukte. Ich kann für fast alle if-else Konstrukte andere Varianten finden. Du hast hier halt auch ein solch riesiges Konstrukt.

Außerdem geben if-else einer Methode Komplexität, welche durch Code-Check-Programme durchaus auch angemeckert werden kann. Das wird dann im beruflichen Umfeld relevant.
 
L

lam_tr

Ich würde lieber jedem Button einen eigenen Listener spendieren (Das ist Java FX, oder? Da bin ich nicht so firm drin).

Grundsätzlich gilt: Keine Scheu vor mehr Klassen. Lieber aus einer langen Methode mehrere kleinere machen und dann aus einer Klasse mit vielen Methoden mehrere Klassen machen.

Bei dem Code oben würde eine statische Codeanalyse wie CheckStyle oder SpotBugs dir auf die Finger klopfen, weil die If-Bedingung viel zu viele Bedingungen hat.
Ja genau, ist JavaFX. Schlussendlich mache ich für jeden Control schon eine Methode. die onButtonAction Methode dispatch an der Stelle die controls zu den Listenern, ist das in dem Fall wirklich nicht Sinnvoll (wahrscheinlich wegen de 20 If/else Zweige wa?).
 
L

lam_tr

Ich kann für fast alle if-else Konstrukte andere Varianten finden
Was wäre das? Ich habe auch schon oft gelesen dass man sparsam damit umgehen soll if / else, aber wie geht man sonst anders vor. Wäre ein Switch Case mit Enum wie @LimDul oben erwähnt schon sinnvoller oder?
 
MoxxiManagarm

MoxxiManagarm

Ein Alternativ-Vorschlag hätte ich noch, wie du bei deinem ActionListener bleiben kannst. Dann müsstest du eine SubKlasse von deinem RadioButton erstellen, also Sinngemäß

Java:
class SortingRadioButton extends RadioButton {
  private Comparator<Booking> comparator;

  public SortingRadioButton(Comparator<Booking> comparator) {
    super();
    this.comparator = comparator;
  }
}


... 
else if (source == radioBetrag || source == radioDatum || source == radioId || source == radioKonto
                || source == radioTyp || source == radioVorgang || source == buttonASC) {
            initTableSort(initTableSort(((SortingRadioButton)source).getComparator()));
        }
 
MoxxiManagarm

MoxxiManagarm

Was wäre das? Ich habe auch schon oft gelesen dass man sparsam damit umgehen soll if / else, aber wie geht man sonst anders vor.
Ein eigener Listener für jeden Control für deinen Fall, außer es ist irgendwo echt sinnvoll den selben Listener zu verwenden.
 
L

LimDul

Was wäre das? Ich habe auch schon oft gelesen dass man sparsam damit umgehen soll if / else, aber wie geht man sonst anders vor. Wäre ein Switch Case mit Enum wie @LimDul oben erwähnt schon sinnvoller oder?
Das ist eine Möglichkeit.

Oft ist aber sinnvoller "weiter oben" (was immer das konkret heißt) schon zu verzweigen. Beispiel mit der onAction Methode:

n Elemente gehen auf eine onAction Methode, die wiederum an m andere Methoden delegiert. Elemeniere den Zwischenschritt das an eine Stelle zu verdichten und verpasse jedem Element eine eigene onAction Methode - da kann man mit Sicherheit auch viel mit Lambda Ausdrücken arbeiten um direkt an die Methoden zu delegieren.
 
L

LimDul

Ein Alternativ-Vorschlag hätte ich noch, wie du bei deinem ActionListener bleiben kannst. Dann müsstest du eine SubKlasse von deinem RadioButton erstellen, also Sinngemäß

Java:
class SortingRadioButton extends RadioButton {
  private Comparator<Booking> comparator;

  public SortingRadioButton(Comparator<Booking> comparator) {
    super();
    this.comparator = comparator;
  }
}


...
else if (source == radioBetrag || source == radioDatum || source == radioId || source == radioKonto
                || source == radioTyp || source == radioVorgang || source == buttonASC) {
            initTableSort(initTableSort(((SortingRadioButton)source).getComparator()));
        }
Dann könnte man eigentlich da schreiben
Java:
else if (source instanceof SortingRadioButton) { // usw.
 
L

lam_tr

Ein Alternativ-Vorschlag hätte ich noch, wie du bei deinem ActionListener bleiben kannst. Dann müsstest du eine SubKlasse von deinem RadioButton erstellen, also Sinngemäß

Java:
class SortingRadioButton extends RadioButton {
  private Comparator<Booking> comparator;

  public SortingRadioButton(Comparator<Booking> comparator) {
    super();
    this.comparator = comparator;
  }
}


...
else if (source == radioBetrag || source == radioDatum || source == radioId || source == radioKonto
                || source == radioTyp || source == radioVorgang || source == buttonASC) {
            initTableSort(initTableSort(((SortingRadioButton)source).getComparator()));
        }
Das würde eventuell gehen wen die Klasse in mein FXML lade. 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.
 
Thema: 

Code refactern

Passende Stellenanzeigen aus deiner Region:
Anzeige

Neue Themen

Anzeige

Anzeige
Oben