Du verwendest einen veralteten Browser. Es ist möglich, dass diese oder andere Websites nicht korrekt angezeigt werden. Du solltest ein Upgrade durchführen oder ein alternativer Browser verwenden.
Wieso schreibt man so komplexen Code und macht nicht einfach ein paar Methoden mehr, damit es leserlich wird?
Also machen wir nur beim gegebenen Code einmal ein Refactoring:
a) Erster Schritt: Mal etwas vernünftiger Einrücken:
Java:
import java.io.File;
import java.util.Arrays;
import java.util.Comparator;
import java.util.Objects;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public class NFiles {
record Filename(File file, String prefix, int number, String ending) {
}
public static void shiftFilenamesOffset(final File dir, final int offset) {
Comparator<Filename> filenameComparator = offset <= 0 ? Comparator.comparingInt(Filename::number) : Comparator.comparingInt(Filename::number).reversed();
String re = "(.*?)(\\d+)\\.(.*+)";
Pattern pat = Pattern.compile(re);
Arrays.stream(Objects.requireNonNull(dir.listFiles()))
.filter(f -> pat.matcher(f.getName()).find())
.map(f -> {
Matcher mat = pat.matcher(f.getName());
mat.find();
return new Filename(f, mat.group(1), Integer.parseInt(mat.group(2)), mat.group(3));
})
.sorted(filenameComparator)
.forEach(fn -> {
File nf = new File(fn.file().getParent(), fn.prefix() + (fn.number() + offset) + "." + fn.ending());
System.out.println(fn + " -> " + nf + " -> " + fn.file().renameTo(nf));
});
}
public static void main(final String[] args) {
shiftFilenamesOffset(new File("test1"), 3);
}
}
b) Objects.requireNonNull - Was soll das erreichen? Wenn es null ist (das Verzeichnis nicht existiert, dann wird eine NPE geworfen. Arrays.stream würde das aber auch schon für uns machen. Wieso nicht eine vernünftige Validierung des Parameters?
Sprich einfach ein
Java:
if (!dir.exists() || !dir.isDirectory()) {
throw new RuntimeException("File does not exist or is not a directory!");
}
und schon braucht man dies nicht mehr.
c) Dann packen wir die Lambdas in separate Methoden (zusammen mit der Umbenennung erster Variablen) und erhalten dann etwas wie:
Java:
import org.jetbrains.annotations.NotNull;
import java.io.File;
import java.util.Arrays;
import java.util.Comparator;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public class NFiles {
record Filename(File file, String prefix, int number, String ending) {
}
static Pattern filenamePattern = Pattern.compile("(.*?)(\\d+)\\.(.*+)");
public static void shiftFilenamesOffset(final File dir, final int offset) {
if (!dir.exists() || !dir.isDirectory()) {
throw new RuntimeException("File does not exist or is not a directory!");
}
Comparator<Filename> filenameComparator = offset <= 0 ? Comparator.comparingInt(Filename::number) : Comparator.comparingInt(Filename::number).reversed();
Arrays.stream(dir.listFiles())
.filter(f -> filenamePattern.matcher(f.getName()).find())
.map(NFiles::createFilename)
.sorted(filenameComparator)
.forEach(fn -> renameFile(fn, offset);
}
private static Filename createFilename(File file) {
Matcher mat = filenamePattern.matcher(file.getName());
mat.find();
return new Filename(file, mat.group(1), Integer.parseInt(mat.group(2)), mat.group(3));
}
private static void renameFile(Filename filename, int offset) {
File nf = new File(filename.file().getParent(), filename.prefix() + (filename.number() + offset) + "." + filename.ending());
System.out.println(filename + " -> " + nf + " -> " + filename.file().renameTo(nf));
}
public static void main(final String[] args) {
shiftFilenamesOffset(new File("test1"), 3);
}
}
Damit hat man dann direkt einen etwas lesbareren Stand. Hier würde ich aber noch etwas weiter vorgehen:
statt dem .filter ein Parameter bei listFiles()
Dann die Zeile Array.steam(...) durch eine Methode ersetzen, die den Stream zurück gibt.
Natürlich noch paar weitere Variablen umbenennen
Die Methoden sind jetzt private static - das ist natürlich auch noch anzupassen. Wozu ist man in einer objektorientierten Sprache, wenn man dies komplett ignoriert?
Aber das nur als kleine Anregung für eigene Optimierungen.
Ja, guter Hinweis. Das Refactoring ist nicht komplett. Bezüglich Kommentar: Das wäre also bei mir hier eine (Factory-)Methode und man hätte dann den Methodennamen und JavaDoc um es deutlicher zu machen.
Ich hatte mal bei einem anderen Programmierer im Codereview angemeckert, dass eine private Methode keinen JavaDoc hatte.
Derjenige konterte, dass an einer private Methode kein JavaDoc erforderlich ist.
Insofern korrekt, weil JavaDoc eigentlich eine API-Doc ist, keine Implementierungs-Doc.
Leider ist für Implementierungs-Doc in Java nichts vorgesehen.
Man kann aber trotzdem JavaDoc als Implementierungs-Doc verwenden.
Ich frage mich außerdem, wie ein Methodenname ausdrücken soll, dass man beim Erhöhen des Index im Dateinamen am Ende beginnen muss, damit sich die Änderungen sich nicht gegenseitig in die Quere kommen. Ein Problem das nach meiner Meinung nicht unmittelbar ersichtlich ist. Nun ja, Andere sind eventuell schlauer, ich könnte mir vorstellen über dieses Problem zu stolpern. bei Dateien ist es wahrscheinlich nicht so schlimm, weil das Umbenennen einfach scheitert, aber im Speicher würde dies zu Datenhack führen.
Ich frage mich außerdem, wie ein Methodenname ausdrücken soll, dass man beim Erhöhen des Index im Dateinamen am Ende beginnen muss, damit sich die Änderungen sich nicht gegenseitig in die Quere kommen. Ein Problem das nach meiner Meinung nicht unmittelbar ersichtlich ist. Nun ja, Andere sind eventuell schlauer, ich könnte mir vorstellen über dieses Problem zu stolpern. bei Dateien ist es wahrscheinlich nicht so schlimm, weil das Umbenennen einfach scheitert, aber im Speicher würde dies zu Datenhack führen.
Du hast einen Ausdruck wie: Arrays.stream(dir.listFiles(f -> filenamePattern.matcher(f.getName()).find()))
Der ist relativ unleserlich. Ein typisches Refactoring ist bei sowas, den Ausdruck, der nicht ganz verständlich ist, in eine Methode zu verlagern. Also etwas wie Array.stream(dir.listFiles(this::isMatchingFile)) Arrays.stream(getMatchingFiles())
oder gar einfach getMatchingFileStream()
Was macht der Code?
Er holt die passenden Dateien,
holt sich dazu die Filename Instanzen (Der Name gefällt mir noch nicht, aber das ist erst einmal egal)
um diese dann zu sortieren
und dann zu verschieben.
Daher kann eine Methode getMatchingFiles, welche einen Stream<File> zurück gibt, zumindest einen Gedanken wert sein.
Ja, aber die Idee ist aus meiner Sicht ja nicht nur die Bereitstellung einer Dokumentation der API sondern eine generelle Unterstützung des Entwicklers. Die Dokumentation sollte die Entwicklungsumgebung ja mit einblenden können. Damit sieht man die, wenn man die Methode aufrufen will und hat daher direkt eine Kontrolle, ob man wirklich nichts vergessen hat. Aber das kenne ich vor allem von Visual Studio / C#, und da war das Ergebnis immer ein .xml File das wir generiert haben für Visual Studio. Daraus haben wir eigentlich nie HTML erzeugt (in dem damaligen Projekt. Ich schon paar Jährchen her.)
Und wichtig ist, dass man sich halt auf klare Coding Guidelines einigt. So sollte klar sein, wie man alles benennt. Und wenn es für etwas noch keine Vorgabe gibt (weil es bei einer Library nicht von außen erkannbar ist), dann legt man dennoch etwas fest, damit es einheitlich ist. So gesehen bin ich ganz bei Dir.
Ich bin aber auch der Meinung, dass man private Methoden durchaus vermeiden sollte. Das ist oft ein Zeichen, dass da in einer Klasse etwas drin ist, das zu komplex ist und daher in eine eigene Klasse verlagert werden sollte. Sprich: Ein angepasstes Design sollte geprüft werden. (Ich sage nicht, dass es immer so sein sollte. Oder dass dies die große Masse sein muss. Aber es sollte aus meiner Sicht bei einem Code Review geprüft werden. Das vereinfacht dann nach meiner Erfahrung dann auch die Unit Tests deutlich.
Bezüglich JavaDoc - wenn man bei privaten Methoden kein JavaDoc wünscht, ist das natürlich ok. Dann passen so wichtige Informationen in das JavaDoc der Klasse. Wobei dann natürlich der Vorteil mit der Entwicklungsumgebung entfällt, weil das ja nicht mehr eingeblendet werden kann oder es einfach zu viel wäre.
Das ist ja etwas, das man im Java Framework öfters findet - die Klasse hat wichtige Hinweise zur Nutzung und solche Besonderheiten immer in der Klassenbeschreibung. In der Methode selbst finden sich meist nur minimal gehaltene Hinweise.
es tut mir leid, aber ich kann mit deinem code review nix anfangen ... die Stream-Methoden sind ja bereits einzelne Methoden ... und wenn dir die einzelnen Aufrufe zu kompliziert sind, liegt das an dir (oder an Java).
es tut mir leid, aber ich kann mit deinem code review nix anfangen ... die Stream-Methoden sind ja bereits einzelne Methoden ... und wenn dir die einzelnen Aufrufe zu kompliziert sind, liegt das an dir (oder an Java).
Damit kann ich erst einmal gut leben. Auch Du wirst evtl. irgendwann einmal einsehen, dass es gut ist, lesbaren und testbaren Code zu schreiben. Aber das ist natürlich keine Verpflichtung. Du kannst weiter Code so schreiben, dass er nicht wirklich lesbar ist. Nur eben solltest Du Dir dann überlegen, sowas niemandem hier im Forum als Antwort zu geben. Das ist eine Zumutung.
Und bei Deiner Einrückung wirst Du auch kaum ernsthaft behaupten wollen, dass Du da einen schnellen Überblick hast. Wenn Du da nach 2 Wochen drauf schaust, wirst Du auch erst einmal anfangen, nach den Klammern zu schauen um zu sehen, wo was anfängt oder aufhört. Alleine schon da ist das erste Refactoring mit Zeilenumbrüchen / Einrückungen ein wichtiger, guter Schritt. Und Tests hast Du ja eh keine geschrieben. Manueller Test mit einem Verzeichnis und das war es, oder?
Daher ja: Keine Grundlage für eine Diskussion. Deine Meinung akzeptiere ich aber sie zeigt ein deutliches Bild von Deinem Stand.
Das stimmt, denn deine "Reviews", die etwas, was ja bereits richtig ist, verbessern sollen, sind für alle eine Zumutung, ja.
Aber, falls eine Persönlichkeitsstörung oder Ähnliches vorliegt, dann kannst du dafür nichts - und ich will mich deshalb auch nicht streiten.
🤷♂️
Wie wäre es damit, wenn ich mich aus diesem Thema in Zukunft heraushalten würde, und du darfst dann alles schreiben, was du möchtest? Nur haltlose Unterstellungen sind halt nicht so schön ...
Und da gehen halt die Meinungen stark auseinander wie Du selbst ganz deutlich merkst. Die Reaktion von @Apple’s Jünger, die Dich so massiv stört, drückt ja auch seine Meinung recht deutlich aus.
Was sollen diese Angriffe jetzt? Du hast Dich jetzt 2 Wochen so zivilisiert verhalten und dann jetzt doch wieder diese Entgleisung?
Du kannst gerne meinen, was immer du willst. Ich will Dich zu nichts bekehren. Daher ist mir das auch alles egal. Aber zu dem Thema gibt es nun wirklich mehr wie genug Literatur und Videos und so. Uncle Bob ist da wohl immer noch mit der beste Einstieg. Und wenn Textverständnis so Probleme bereitet, dann gibt es das als Videos:
