Java Backend: Ist diese Methode zu komplex?

[CyborgBeta]

Moin,

ich habe in einem Review den Hinweis bekommen, dass diese Methode nur schwer verständlich sei. Stattdessen solle ich in mehrere kleinere Methoden splitten und Datentypen einführen. Aus meiner Sicht ist die Methode aber nicht zu lang und die zyklomatische Komplexität auch nicht zu hoch ... Könnt ihr bitte mal schauen, ob diese Methode aus eurer Sicht noch verständlich ist (oder ob ich wirklich eine Vereinfachung übersehen habe)? Danke.

  public static List<List<List<Object>>> getTodayData() throws Exception {
    DateFormat df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
    df.setTimeZone(TimeZone.getTimeZone("Europe/Berlin"));

    List<List<Object>> data = MDB.get();
    List<List<List<Object>>> tables = new ArrayList<>();
    data.sort(Comparator.comparing(a -> ((java.sql.Timestamp) a.get(0))));
    for (int i = 0; i < data.size(); ) {
      List<List<Object>> table = new ArrayList<>();
      boolean isFirstEntry = true;
      while (isFirstEntry || i < data.size() && data.get(i - 1).get(0).equals(data.get(i).get(0))) {
        table.add(data.get(i++));
        isFirstEntry = false;
      }
      tables.add(table);
    }
    for (List<List<Object>> table : tables) {
      table.add(
          Stream.concat(
                  Stream.of(table.get(0).get(0), "sum", "---"),
                  IntStream.range(3, 10)
                      .mapToObj(i -> table.stream().mapToDouble(r -> (double) r.get(i)).sum()))
              .collect(Collectors.toCollection(ArrayList::new)));
    }
    for (List<List<Object>> table : tables) {
      for (List<Object> row : table) {
        row.set(0, df.format(new Date(((java.sql.Timestamp) row.get(0)).getTime())));
        IntStream.range(3, row.size())
            .forEach(i -> row.set(i, String.format("%.2f", (Double) row.get(i))));
      }
    }
    return tables;
  }

MDB.get() gibt Daten von einer Datebank zurück (Tabellen in einer Tabelle ...).

 

[dewa]

Ich stimme deinem Reviewer zu. Du hast mehrere Schleifen hintereinander, die verschiedene Dinge tun. Diese könntest du jeweils auslagern und den Methoden Namen geben, die verständlich machen was getan wird.

Auch würde ich die Operationen auf table durchführen, bevor du sie zu tables hinzufügst, dann würdest du dir die zwei for loops darüber sparen.

Auch die verschachtelten Listen sind sehr unübersichtlich. Wenn der Return Type nicht unbedingt so sein muss, weil es irgendwo in bestehenden Code integriert wird, würde ich den auf jeden Fall ändern. Und selbst dann würde ich den Code wahrscheinlich refactorn. So sind Fehler beinahe vorprogrammiert

 

[CyborgBeta]

Danke, also 3 Schleifen == 3 Methoden?

 

[Krik]

Das hier hätte ich abgelehnt:

• List<List<List<Object>>> - Ich kann mir nicht vorstellen, dass so ein Datentyp benötigt wird. Und wenn doch, würde ich den selber bauen, sodass man versteht, was jede Liste enthält.
• while (isFirstEntry || i < data.size() && data.get(i - 1).get(0).equals(data.get(i).get(0))) { - Außer dem isFirstEntry ist das nicht selbsterklärend. Alles nach dem i < data.size() würde ich in eine extra Funktion auslagern, die sinnvoll benannt ist, z. B. hasEqualChildPredecessor oder so.

Und natürlich gilt noch der Einwand von @dewa, dass man die drei Schleifen auslagern sollte, da sie nichts miteinander zu tun haben.

 

[BeBur]

List<List<List<Object>>>

Die Signatur zeigt schon, dass man das refactorn sollte.

Danke, also 3 Schleifen == 3 Methoden?

3 Dinge werden getan == 3 Methoden

 

[CyborgBeta]

List<List<List<Object>>> - Ich kann mir nicht vorstellen, dass so ein Datentyp benötigt wird. Und wenn doch, würde ich den selber bauen, sodass man versteht, was jede Liste enthält.

Die Template-Engine braucht später so ein Konstrukt, um die Tabelle(n) aufzubauen.

Alles nach dem i < data.size() würde ich in eine extra Funktion auslagern, die sinnvoll benannt ist, z. B. hasEqualChildPredecessor

Danke, gute Idee.

dass man die drei Schleifen auslagern sollte, da sie nichts miteinander zu tun haben.

Das sehe ich (ein wenig) anders, weil die drei Schleifen theoretisch auch innerhalb einer Schleife stehen könnten. Beispiel:

Statt:

for (...) {
    doSomethingA(...);
}
for (...) {
    doSomethingB(...);
}
for (...) {
    doSomethingC(...);
}

Ginge auch:

for (...) {
    doSomethingA(...);
    doSomethingB(...);
    doSomethingC(...);
}

 

[BeBur]

Statt:
for (...) { doSomethingA(...); } for (...) { doSomethingB(...); } for (...) { doSomethingC(...); }
Ginge auch:

Statt doSomething{A,B,C} schreibst du jetzt noch was da tatsächlich passiert und schon hast du ein sinnvolles refactoring gemacht.

 

[Krik]

Das sehe ich (ein wenig) anders, weil die drei Schleifen theoretisch auch innerhalb einer Schleife stehen könnten.

Darum ging es mir nicht. Jede Methode oder Funktion sollte möglichst nur genau eine Aufgabe selbst erfüllen und alles andere auslagern.

Weil das...

Aus meiner Sicht ist die Methode aber nicht zu lang und die zyklomatische Komplexität auch nicht zu hoch

...sind nur Zahlen (Länge, Komplexität), aber sie sagen erst mal nichts über die Verständlichkeit des Codes aus. Manchmal ist es sogar besser, längeren Code oder höhere Komplexität zu haben, weil man ihn dann besser versteht oder er (in speziellen Fällen) "einfacherem" Code überlegen ist. Aber der Grund steht dann immer in einem Kommentar, sodass auch hier jeder nachvollziehen kann, warum das so ist.

 

[CyborgBeta]

schreibst du jetzt noch was da tatsächlich passiert

Das würde einen Nachmittag füllen. Aber ganz vereinfacht gesagt, stehen in einer Datenbanktabelle mehrere Tabellen in einer, die nur über die erste Spalte, den Timestamp, gruppiert werden können (alle Reihen einer Subtable haben den gleichen Timestamp). Das ist nicht besonders gut modelliert, gebe ich zu.

Die erste Schleife, zusammen mit dem Sort, splittet oder gruppiert die Gesamttabelle in mehrere kleine Tabellen auf.

Die zweite Schleife fügt jeder Subtable eine Summen-Reihe am Ende hinzu.

Die dritte Schleife formatiert alle double-Werte jeder Subtable mit/in genau zwei Nachkommastellen, da in der Frontend-Übersicht nicht so viele Nachkommastellen gebraucht werden. Ach ja, und der Timestamp wird noch in ein lesbares Format formatiert.

 

[Simon#G]

Ich würde das im Review mit dem selben Gründen bemängeln. Fünf Loops (Streams nicht mitgezählt) sind zu viel. Zumal man das wirklich problemlos aufteilen kann, schön mit erklärenden Funktionsnamen.

Der erste Loop sieht auch verdächtig aus wie ein groupBy? Ich bin kein Experte für Java Streams (verwöhnt von Kotlin), aber Collectors.groupingBy() klingt als könnte es an dieser Stelle helfen.

 

[Tornhoof]

Wie wär's mal mit Kommentaren?
Minimum irgendwo mal ein beispiel im Kommentar mit was denn die Zeile beinhalten sollte.
Ich würde auch den Empfehlungen mal folgen und die entsprechende Objekte einführen, die die Spalten abbilden.

Die Java lambda syntax is noch schlimmer als in C#.

 

[tollertyp]

Ich würde die Bücher empfehlen:

• Clean Code von Robert C. Martin
• Effective Java von Joshua Bloch

 

[Tornhoof]

Clean Code von Robert C. Martin

Yay, wollen wir nen Forumkrieg bzgl. dem Buch von Robert C. Martin anfangen

Heutzutage ist die die häufigste Meinung, dass das Buch die richtige Ideen hat, es aber zu dogmatisch umgesetzt wird und so einiges an den Ideen von neuen Sprachfeatures überholt wurde.
Robert C. Martin hat auch nicht wirklich dazu beigetragen, dass sein Buch weniger dogmatisch betrachtet wird.

Oder ums anders auszudrücken, mit dem Buch kannst einen perfekten Forumskrieg anfangen.

 

[CyborgBeta]

Die Java lambda syntax is noch schlimmer als in C#.

Da stimme ich dir sogar zu. Die erste Version, die ich hatte, war sogar komplett ohne Streams ... aber man muss mit der Zeit gehen.

 

[tollertyp]

Oder ums anders auszudrücken, mit dem Buch kannst einen perfekten Forumskrieg anfangen.

Alleine sich mit den Dingen mal zu beschäftigen hilft. Was man daraus mitnimmt ist jedem selbst überlassen. Den Horizont erweitert man selten, wenn man sich nur seinen eigenen Code und seine eigenen Ideen anschaut.
Und warum die Sachen aus Clean Code durch Sprachfeatures überholt sind erschließt sich mir nicht ganz.

 

[CyborgBeta]

@tollertyp Schau dir rein funktionale Programmiersprachen an. Zum Beispiel Haskell:

        map :: (a -> b) -> [a] -> [b]
        map f [] = []
        map f (x:xs) = f x : map f xs

        map quadrat [1,2,3] = [quadrat 1, quadrat 2, quadrat 3] = [1,4,9]

https://de.wikipedia.org/wiki/Haskell_(Programmiersprache)

da hast du viel von dem "Boilerplate" nicht, welches die strikte Abwärtskompatibilität nun mal mit sich bringt ...

 

[tollertyp]

Und in welchem Zusammenhang steht das mit meiner Aussage?

Und statt Streams würde ich manchmal auch eher einfachere Konstrukte machen und Hilfsmethoden:
edit: Hab einen Fehler gemacht, kommt gleich richtig
edit2: Also ich würde da vermutlich noch viel mehr machen, aber die Richtung meine ich:

      table.add(
          Stream.concat(
                  Stream.of(table.get(0).get(0), "sum", "---"),
                  IntStream.range(3, 10)
                      .mapToObj(i -> table.stream().mapToDouble(r -> (double) r.get(i)).sum()))
              .collect(Collectors.toCollection(ArrayList::new)));

eher zu sowas:

table.add(createSumRow(table))

...

private static List<Object> createSumRow(List<List<Object>> table) {
    List<Object> sumRow = new ArrayList<>();
    sumRow.add(getTitle(table));
    sumRow.add("sum");
    sumRow.add("---");
    IntStream.range(3, 10)
        .mapToObj(i -> table.stream().mapToDouble(r -> (double) r.get(i)).sum())
        .forEach(sumRow::add);
    return sumRow;
}
   
private static Object getTitle(List<List<Object>> table) {
    return table.get(0).get(0); // hier könnte gleich formatiert werden...
}

 

[Tornhoof]

Und warum die Sachen aus Clean Code durch Sprachfeatures überholt sind erschließt sich mir nicht ganz.

Robert C. Martin hat sich u.A. sehr gegen Themen wie explizite nullability syntax (Elvis Operator ?.) ausgesprochen, das mittlerweile in Java, Kotlin, C# und anderen umgesetzt wird.
Man kann von denen halten was man will, dogmatisch ablehnen ist sicher nicht die richtige Wahl und der Autor ist bekannt für seine absoluten Aussagen, die dann gerne dogmatisch wiederholt werden.

Im Fall von Robert C. Martin ist halt nicht nur der Leser schuld an der dogmatischen Umsetzung, bei so Sachen wie den Patterns der Gang of Four ists eher der Entwickler oder Sekundärautoren.

 

[tollertyp]

Wo wird die Syntax in Java umgesetzt? Habe ich was verpasst?

Aber dass heutzutage Personen wichtiger sind als Inhalte, das ist keine neue Erkenntnis.

 

[Tornhoof]

Syntax in Java umgesetzt? Habe ich was verpasst?

In Java ist's afaik noch Optional<>, da hast du Recht, da war ich zu schnell.
https://bugs.openjdk.org/browse/JDK-8303099 ist noch offen