Java: Refactoring einer zu langen Methode...

Schlangenmensch

@Fragender Du teilst die Funktion so in Unterfunktionen mit sprechenden Namen, dass auch einem anderen Leser möglichst direkt klar ist, was wo gemacht wird und das Teilaufgaben eventuell auch für sich genommen getestet werden können.

@Schlangenmensch sagte in Java: Refactoring einer zu langen Methode...:

Du teilst die Funktion so in Unterfunktionen mit sprechenden Namen, dass auch einem anderen Leser möglichst direkt klar ist, was wo gemacht wird

Will ich aber gar nicht. ich mach nix ohne grund... und verstehe die zwei geschachtelten Schleifen als eine logisch-algorithmische Einheit ...

C Newbie

Anstatt drei Streams die jeweils min, max und average berechnen könntest du einen stream mit

java.util.stream.Collectors.summarizingDouble(ToDoubleFunction<? super T>)

verwenden.

Schlangenmensch

@Fragender Dann musst du das mit deiner QC diskutieren.

Ich persönlich fände es übersichtlicher, wenn das ganze aufgeteilt wäre in

public static void normalizeJsonData() throws IOException {
{
   File dir = new File(DIR_NAME);
   File[] files = dir.listFiles();
   if (files == null) throw new AssertionError();
   for (File file : files) {
      List<List<JsonObject>> chunks = GetChunks(file);
      JsonArray newArray = NormlizeChunks(chunks);
      WriteFile(file, newArray);
}

Und, die drei Zeilen aus der For Schleife würde ich wahrscheinlich auch noch in eine NormalizeJsonFile oder so packen.

P.S. Ich habe seit ewigkeiten keinen Java Code mehr geschrieben. Daher, keine Ahnung ob das so ginge, oder ob man das in Java tendentiell anders löst.

@C-Newbie sagte in Java: Refactoring einer zu langen Methode...:

Anstatt drei Streams die jeweils min, max und average berechnen könntest du einen stream mit
java.util.stream.Collectors.summarizingDouble(ToDoubleFunction<? super T>)
verwenden.

Danke für deinen Vorschlag. Ganz zufrieden bin ich zudem auch noch nicht mit:

@Fragender sagte in Java: Refactoring einer zu langen Methode...:

        JsonObject firstObj = array.get(0).getAsJsonObject();
        long start = firstObj.get("x").getAsLong() / two_hours * two_hours;

firstObj wird nur einmal verwendet und sollte inlined werden.

@Fragender sagte in Java: Refactoring einer zu langen Methode...:

    long one_hour = (1000 * 60 * 60);
    long two_hours = (1000 * 60 * 60 * 2);
    File dir = new File(DIR_NAME);
    File[] files = dir.listFiles();

one_hour und two_hours werden erst nach dir sowie files verwendet, sie könnten dementsprechend zwei Zeilen weiter unten deklariert werden ... andererseits sind dies quasi "Halbkonstanten".

@Fragender sagte in Java: Refactoring einer zu langen Methode...:

        JsonArray array = getJsonArray(file);
        JsonObject firstObj = array.get(0).getAsJsonObject();
        long start = firstObj.get("x").getAsLong() / two_hours * two_hours;

Ggf. fehlt hier ein length-Check (vor .get(0)) mit Abbruch, bin mir aber nicht ganz sicher, wie JSON.simple dies handhabt.

@Schlangenmensch sagte in Java: Refactoring einer zu langen Methode...:

@Fragender Dann musst du das mit deiner QC diskutieren.
Ich persönlich fände es übersichtlicher, wenn das ganze aufgeteilt wäre in

Das ging aus dem Eingangsposting nicht ganz klar hervor, aber die kontrollierende Instanz bin ich selber (und das Tool) ... ich schreibe 'ne Anwendung für mich selber. Wenn man so will, bin ich mein eigener Chef. ... Das hat auch viele Vorteile. Zum Beispiel brauche ich mich nicht mit Shice-Code, den andere geschrieben haben, befassen, und ich muss keine blödsinnigen Fragen beantworten, und ich muss niemandem guten Morgen wünschen, etc.

Habe mal ein klein wenig aufgeräumt:

    public static void normalizeJsonData() throws IOException {
        final long one_hour = 1000 * 60 * 60;
        final long two_hours = 1000 * 60 * 60 * 2;
        File dir = new File(DIR_NAME);
        File[] files = dir.listFiles();
        assert files != null;
        for (File file : files) {
            JsonArray array = getJsonArray(file);
            long start =
                    array.get(0).getAsJsonObject().get("x").getAsLong() / two_hours * two_hours;
            long stop = start + two_hours;
            List<List<JsonObject>> chunks = new ArrayList<>();
            List<JsonObject> newChunks = new ArrayList<>();
            Iterator<JsonElement> iterator1 = array.iterator();
            while (iterator1.hasNext()) {
                JsonObject jo = iterator1.next().getAsJsonObject();
                long x = jo.get("x").getAsLong();
                if (x >= start && x < stop) {
                    newChunks.add(jo);
                } else {
                    chunks.add(newChunks);
                    newChunks = new ArrayList<>();
                    newChunks.add(jo);
                    start = stop;
                    stop = stop + two_hours;
                }
                if (!iterator1.hasNext()) {
                    // last element
                    chunks.add(newChunks);
                }
            }

            JsonArray newArray = new JsonArray();
            Iterator<List<JsonObject>> iterator2 = chunks.iterator();
            while (iterator2.hasNext()) {
                List<JsonObject> chunk = iterator2.next();
                if (iterator2.hasNext()) {
                    // not last element
                    DoubleSummaryStatistics statistics =
                            chunk.stream()
                                    .mapToDouble(jo -> jo.get("y").getAsFloat())
                                    .summaryStatistics();
                    double min = statistics.getMin();
                    double max = statistics.getMax();
                    double avr = statistics.getAverage();
                    double newY = Math.abs(min - avr) >= Math.abs(max - avr) ? min : max;
                    long newX =
                            chunk.get(0).getAsJsonObject().get("x").getAsLong()
                                            / two_hours
                                            * two_hours
                                    + one_hour;
                    JsonObject newObj = new JsonObject();
                    newObj.addProperty("x", newX);
                    newObj.addProperty("y", (float) newY);
                    newArray.add(newObj);
                } else {
                    // last element
                    for (JsonObject jo : chunk) {
                        newArray.add(jo);
                    }
                }
            }

            try (FileWriter fw = new FileWriter(file, Charset.defaultCharset())) {
                fw.write(newArray.toString());
            }
        }
    }

jetzt dürft ihr wieder den obligatorischen Daumen nach unten geben...

Leon0402

Die Methode ist viel zu lange.

@Fragender sagte in Java: Refactoring einer zu langen Methode...:

Will ich aber gar nicht. ich mach nix ohne grund... und verstehe die zwei geschachtelten Schleifen als eine logisch-algorithmische Einheit .

Der Grund ist bessere Lesbarkeit, Verständlichkeit, Testbarkeit, Wartbarkeit, Widerverwendbarkeit. Wenn das nicht Grund genug ist, dann weiß ich auch nicht weiter Logisch Algorithmische Einheiten definieren sich nicht (nur) über Methoden.

Z.B. das iterieren übere alle Json Dateien, das auslesen der Json aus der Datei und das speichern der normalisierten Json Dateien haben erstmal per se nix mit dem normalisieren zu tun.

Kann ja sein, dass das normalerweise immer zusammen passiert, aber dann kann man auch ne Klasse / Modul etc. nutzen, um das auszudrücken. Aktuelle vermischst du auf jeden Fall Abstraktionslevels. Du nutzt doch aktuell auch schon Mehtoden von DoubleSummaryStatistics. Da stört es dich doch auch nicht, dass die ausgelagert sind. Gehört das nicht zur logisch algorithmischen Einheit?

Der Vorschlag von @Schlangenmensch ist auf jeden Fall top oben.

Quiche Lorraine

@Leon0402 sagte in Java: Refactoring einer zu langen Methode...:

Der Grund ist bessere Lesbarkeit, Verständlichkeit, Testbarkeit, Wartbarkeit, Widerverwendbarkeit. Wenn das nicht Grund genug ist, dann weiß ich auch nicht weiter

Kleinere logische Einheiten sind meines Erachtens auch Teil der Divide & Conquer Strategie.

Alleine wenn man schon ein größeres Problem in kleine Einheiten aufteilt, diese implementiert und testet, ist man meistens schon einen Schritt weiter als wenn man eine eierlegende Wollmilchsau-Einheit programmiert.

Swordfish

@Fragender sagte in Java: Refactoring einer zu langen Methode...:

Habe mal ein klein wenig aufgeräumt:

Ne, haddu nicht. Was kommt raus wenn Du von "Eine Methode genau eine Aufgabe" ausgehst?

@Swordfish sagte in Java: Refactoring einer zu langen Methode...:

Was kommt raus wenn Du von "Eine Methode genau eine Aufgabe" ausgehst

(Gesamt-)Ziel ist es, die Daten, die in Form von JSON vorliegen, zu "normalisieren" - also sie in irgendeiner Form zu modifizieren. Das kann man "kleinteiliger" machen, aber das sollte dann auch sinnvoll sein. Separations of concerns ist mir zwar ein Begriff, allerdings sind die "Eingangsvoraussetzungen", wann man SoC anwenden sollte, nicht schwarz oder weiß, sondern liegen in einer Grauzone, bzw. sind mit einem Ermessensspielraum verbunden, meiner Meinung nach. Diese Methode wird nur einmal verwendet und auch die "Methodenteile" werden nur von dieser Methode verwendet. Ich verwende/wiederhole also Teilimplementierungen an anderer Stelle nicht. Für mich ist das ein Zeichen dafür, SoC nicht anzuwenden und die Methode nicht weiter zu splitten.

Überzeugt mich aber gerne vom Gegenteil.