Backup Programm - Code-Review von euch erwünscht

Use-Case:
Also ich hab viele Dateien quer über das aktive Windows-System verteilt, die ich gerne sichern möchte. Zip-Datei.

Usage:
Zuerst muss eine Datei „dirs1.txt“ erstellt werden, in der in jeder Zeile der vollständige Pfad zu einem zu sichernden Verzeichnis steht.
Danach kann man das Programm starten. Wenn ein Verzeichnis mehr als 1000 Dateien oder mehr als 1GB enthält, wird man gefragt, ob man dieses Verzeichnis von der Sicherung ausschließen möchte oder nicht.
Danach wird eine Datei „ex1.txt“ erstellt, mit den auszuschließenden Dateien, diese werden beim nächsten Programmstart wiederverwendet, insofern die Datei nicht gelöscht wird.
Danach erstellt das Programm die Sicherung, schließt dabei aber die Verzeichnisse und Dateien von „ex1.txt“ aus, sowie Dateien, die versteckt sind oder nicht gelesen werden können.

Code:

import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.io.PrintWriter;
import java.nio.file.FileVisitResult;
import java.nio.file.FileVisitor;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.BasicFileAttributes;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.HashSet;

import javax.swing.JOptionPane;

import net.lingala.zip4j.ZipFile;
import net.lingala.zip4j.exception.ZipException;
import net.lingala.zip4j.model.ExcludeFileFilter;
import net.lingala.zip4j.model.ZipParameters;
import net.lingala.zip4j.model.enums.AesKeyStrength;
import net.lingala.zip4j.model.enums.EncryptionMethod;

public class BackupTool {

	private static final String backup_password = "pw";
	private static final long max_count = 1000;
	private static final long max_size = 1000 * 1000 * 1000;
	private static final File backup_file;
	static {
		SimpleDateFormat sdf = (SimpleDateFormat) DateFormat.getDateTimeInstance();
		sdf.applyPattern("yyyy-MM-dd-HH-mm-ss");
		backup_file = new File("backup1-" + sdf.format(new Date()) + ".zip");
	}

	public static void folderSize(File directory, long[] countAndSize) {
		File[] files = directory.listFiles();
		if (files == null || files.length == 0) {
			return;
		}
		countAndSize[0] += files.length;
		if (countAndSize[0] > max_count) {
			return;
		}
		for (File file : files) {
			if (file.isFile()) {
				countAndSize[1] += file.length();
				if (countAndSize[1] > max_size) {
					return;
				}
			} else {
				folderSize(file, countAndSize);
			}
		}
	}

	public static ArrayList<File> readDirs1() throws IOException {
		ArrayList<File> dirs1 = new ArrayList<>();
		try (BufferedReader br = new BufferedReader(new FileReader("dirs1.txt"))) {
			String l;
			while ((l = br.readLine()) != null) {
				dirs1.add(new File(l));
			}
		}
		return dirs1;
	}

	public static HashSet<File> readEx1() throws IOException {
		HashSet<File> ex1 = new HashSet<>();
		if (new File("ex1.txt").exists()) {
			try (BufferedReader br = new BufferedReader(new FileReader("ex1.txt"))) {
				String l;
				while ((l = br.readLine()) != null) {
					ex1.add(new File(l));
				}
			}
			return ex1;
		}
		return null;
	}

	public static ArrayList<File> createEx2(ArrayList<File> dirs1) throws IOException {
		ArrayList<File> ex1 = new ArrayList<>();
		for (File file : dirs1) {
			Files.walkFileTree(file.toPath(), new FileVisitor<Path>() {
				@Override
				public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException {
					File f = dir.toFile();
					if (f.isHidden()) {
						return FileVisitResult.SKIP_SUBTREE;
					}
					if (!f.canRead()) {
						return FileVisitResult.SKIP_SUBTREE;
					}
					long[] cas = new long[2];
					folderSize(f, cas);
					if (cas[0] > max_count || cas[1] > max_size) {
						if (JOptionPane.showConfirmDialog(null,
								"Exclude: " + f.toString() + " ?") == JOptionPane.YES_OPTION) {
							ex1.add(f);
							return FileVisitResult.SKIP_SUBTREE;
						}
					}
					return FileVisitResult.CONTINUE;
				}

				@Override
				public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
					return FileVisitResult.CONTINUE;
				}

				@Override
				public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException {
					return FileVisitResult.SKIP_SUBTREE;
				}

				@Override
				public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
					return FileVisitResult.CONTINUE;
				}
			});
		}
		return ex1;
	}

	public static void storeEx2(ArrayList<File> ex2) throws IOException {
		for (File file : ex2) {
			System.out.println(file);
		}
		try (PrintWriter pw = new PrintWriter("ex1.txt")) {
			for (File file : ex2) {
				pw.println(file.toString());
			}
		}
	}

	public static void createZip(ArrayList<File> dirs1, HashSet<File> ex1) throws ZipException {
		ZipParameters p = new ZipParameters();
		p.setExcludeFileFilter(new ExcludeFileFilter() {
			@Override
			public boolean isExcluded(File file) {
				return ex1.contains(file) || file.isHidden() || !file.canRead();
			}
		});
		p.setEncryptFiles(true);
		p.setEncryptionMethod(EncryptionMethod.AES);
		// Below line is optional. AES 256 is used by default. You can override it to
		// use AES 128. AES 192 is supported only for extracting.
		p.setAesKeyStrength(AesKeyStrength.KEY_STRENGTH_256);

		ZipFile zipFile = new ZipFile(backup_file, backup_password.toCharArray());
		for (File file : dirs1) {
			zipFile.addFolder(file, p);
		}

		System.out.println("fertig");
	}

	public static void main(String[] args) throws IOException {
		ArrayList<File> dirs1 = readDirs1();
		HashSet<File> ex1 = readEx1();
		if (ex1 == null) {
			ArrayList<File> ex2 = createEx2(dirs1);
			storeEx2(ex2);
			ex1 = new HashSet<>(ex2);
		}
		ex1.add(backup_file);
		createZip(dirs1, ex1);
	}
}

Maven:

		<!-- https://mvnrepository.com/artifact/net.lingala.zip4j/zip4j -->
		<dependency>
			<groupId>net.lingala.zip4j</groupId>
			<artifactId>zip4j</artifactId>
			<version>2.6.2</version>
		</dependency>

github: https://github.com/srikanth-lingala/zip4j

Troubleshooting: Die Dateien werden mit einem Passwort gesicht, allerdings ist das komplette Zip nicht mit einem Passwort gesichert… also man kann die Dateien sehen, ohne das Passwort zu kennen.

Was mir beim Überfliegen sofort auffält:

  • countAndSize ist hässlich. Java ist objektorientiert, warum nimmst du nicht einfach eine Instanz mit den Feldern count und size?
  • Wieso arbeitest du mit Implementierungen (z.B. HashSet) statt mit Interfaces (z.B. Set)? Wenn dir irgendwann einmal einfällt, dass ein LinkedHashSet besser wäre, ämderst du das halbe Programm.
  • der Stil wirkt sehr wie Java 7, z.B. keine Streams, sehr viele Schleifen, und ich denke, es gibt auch an einigen Stellen neuere Methoden um mit Files zu arbeiten.
  • einige der Variablennamen sind nicht so toll. Ich weiß nicht so recht, warum ich es überhaupt noch schriebe, weil diese Kritik bei jedem deiner Programmbeispiele kommt…
  • einige Methoden sind zu lang und erfüllen mehrere Aufgaben gleichzeitig
1 Like

Alle Methoden sind static? Warum nicht in unterschiedliche Klasse eine Aufgabentrennung machen…weiterhin werden parameter verwendet und dann auch wieder verändert (z.B. folderSize countAndSize)…Ja und auch würde raten, dass die Rückgaben Interfaces sind wie z.B. List, Set etc. anstatt ArrayList etc…wie schon von Landei bemerkt…(Also mal abgesehen davon, dass ich mir sehr gut überlegen würde, wenn ich ein Backup schreibe, dass mein Programm mit Tests gesichert wäre, ob alle Teile auch das machen was ich möchte. Ich sehe hier keinerlei Tests… oder gibt es das Ganze irgendwo auf GitHub …)…

Konvention für Konstanten ist die, dass man die Konstanten groß schreibt wie z.B. max_size, max_count

Hier mal ein einfaches Beispiel für das lesen eines Verzeichnisses und als Ergebnis wird das Ganze in eine List gesteckt: (JDK11):

var fileCollection = Files.list(Paths.get("...Verzeichnis.."))
          .filter(Files::isRegularFile)
          .collect(toList());

Weiterhin würde ich auch dringend raten sich mit Java 7+ Path zu beschäftigen (siehe obiges Beispiel) …weiterhin kann man mithilfe von JDK8+ Files.readLines etc. Dateiinhalte einfacher einlesen… anstatt mit br.readLine zu arbeiten (Buffer etc.)… weiterhin würde ich im Falle eines Fehler bzw. nicht vorhanden sein einer Datei niemals eine null zurück geben (readEx1())… in dem Falle würde ich einfach den leeren HashSet zurück geben. Dann könnte man im main einfach auf if (ext1.isEmpty())… prüfen…und den ganzen Geschichten auf Windows würde ich sehr genau aufpassen in welchem Encoding ich Dateien bzw. Dateinamen speichere… (storeEx2) …

Abgesehen davon sehe ich mitten im Code (createEx2) Dialog Options…keine saubere Trennung zwischen Aufgabe und Dialog. … warum dann überhaupt eine Konfigurationsdatei? Und meine Frage wäre, wenn ich schon eine Sicherung mache, warum begrenze ich dann die Anzahl von Dateien die in einem Verzeichnis vorhanden sind? Mein Vorgehen wäre hier, eine Black Liste zu machen, dass heißt alles zu sichern, außer den Verzeichnissen/Dateien die in einer Konfigurationsdatei angegeben sind. Damit das Programm auch OHNE mein Warten vor der Tastatur durchläuft…

Und ja Namensgebung usw. ist sehr wichtig…und das ist hier nicht wirklich der Fall… (siehe Kommentare vom Landei…).

BTW: Warum wird die Dateigröße auf auf 1000 * 1000 * 1000; begrenzt? Es gibt doch zip64 ? …

Gruß
Karl Heinz

1 Like

Bah, mir wird gerade schlecht. :face_vomiting:

Vielleicht habe ich mich bewusst gegen die neuen Features entschieden.

Für mich ist null hier angebracht. Ich finde es Quatsch, auf null im Allgemeinen zu verzichten.

Nein, das ist hier kein Problem.

Es ist nicht auf 1GB beschränkt, nur, ab dieser Grenze wird man gefragt, ob die Datei/ das Verzeichnis mit in das Zip genommen werden soll.
Dadurch kann ich Verzeichnisse, die ich sicher nicht brauche, ausschließen.

Prinzipiell soll das Programm zwei Dinge tun, eine Konfigurationsdatei erstellen, falls noch nicht vorhanden, und das „Zippen“.

Danke für eure Rückmeldungen.

Hm… neue Features??? (in der Zwischenzeit 6 Jahre+ alt…somit würde ich in dem Zusammenhang nicht mehr das Wort neu verwenden)…aktuell ist JDK 14…in den nächsten Tage wird JDK 15 GA kommen… JDK 11 ist aktuelle LTS Stand …

Ich habe leider schon sehr oft solche „Argumente“ von Entwicklern gehört…„Ich habe mich bewusst gegen … neue/Feature x … entschieden“…
Ich würde eher sagen, das ist eine Ablehnung von Features/Vereinfachungen usw. weil man nicht sieht welche Vorteile usw. die entsprechenden Features liefern …(persönliche Meinung!)

Gerade der Bereich Streams usw. (JDK8+) und Java NIO Path/Files usw. (Seit Java 7 somit 9 Jahre+) viele Vorteile bieten was Fehlerhandling usw. bieten. Try-With-Resource findet aber Verwendung (Java 7)…

Mal abgesehen davon, dass die Verwendung von var deutlich übersichtlicheren Code erzeugt…da man nicht mehr wiederholen muss, was man auf der Rechten Seite schon geschrieben hat…abgesehen davon ist es meist auch nicht wichtig…

  1. Habe ich das so nicht geschrieben sondern:

weiterhin würde ich im Falle eines Fehler bzw. nicht vorhanden sein einer Datei niemals eine null zurück geben (readEx1())… in dem Falle würde ich einfach den leeren HashSet zurück geben.

  1. Ich bin aber der Meinung null zu vermeiden. Weil es Fehler vermeidet und Code einfacher zu lesen macht.
    Hier ist ein recht einfacher Code…der aber schon Zeigt, dass im Falle des Fehler der HashSet einfach als Leer zurück gegeben werden könnte. Ansonsten muss man an jeder Stelle die Nullprüfung machen (in diesem Code exakt einmal der Fall). Das würde die Nutzung von null entfernen…Somit auch die potentiellen NPE… (hier im kleinen; Wer größere Code mengen schreibt…der lernt, dass null echt sch… ist… Aus dem Grund Vermeidung von null
2 Likes

Hierzu noch als Ergänzung: https://clean-code-developer.de/die-grade/orangener-grad/#Separation_of_Concerns_SoC

Hier empfehle ich meinen Azubis immer über jede Methode und Klasse ein JavaDoc Comment zu schreiben mit dem Inhalt, was die Klasse alles macht. Und wenn hier das Wort „und“ vorkommt, dann ist zu prüfen, ob die Klasse dafür die Kompetenz haben soll.

Bei Dir wäre es also „Die Klasse ist der Haupteinstiegspunkt in das Programm UND die Auswertung der Konfiguration UND das finden der pot. Dateien zum zippen UND das zippen selbst“.

Man müsste nun also für alle UND prüfen, ob das korrekt ist.

Ebenfalls hier noch der Hinweis, dass die Orchestrierung des Programms auch sehr komplex ist.

Möchte man die Klasse als Utility nutzen, so hätte man hier sehr viel Wissen gebraucht um die Funktionsweise der Klasse, damit man sie überhaupt nutzen kann.

https://clean-code-developer.de/die-grade/roter-grad/#Integration_Operation_Segregation_Principle_IOSP

Das Logging besteht hier aus einem einfachen Consolen Logging. Wenn man schon maven nutzt, dann wäre es einfach hier SLF4J oder ein entsprechendes Werkzeug zu nutzen. Selbst mit SYSOUT wären die Logausgaben mir zu wenig. Wenn etwas schief läuft, dann weiß ich nicht warum.

Das Exception Handling ist auch nicht vorhanden. Es wird einfach am Ende IOException von main geworfen. Sinnvolle Hinweise an den Nutzer wären hier vorzuziehen.

Das Passwort für die ZIP ist hart codiert und kann somit nicht geändert werden. Ebenfalls ist dies sehr einfach, so dass eine Bruteforce Attacke nach weniger als 26 ^2 Schritten bereits Erfolg hätte.

Alle anderen Informationen, z.B. wie die Backupdatei heißt ist auch vorgegeben. Das kann man so machen, aber i.d.R: mache ich ein Backup zu einem anderen Medium, weil ich sonst auf der aktuellen Platte mind. 40 % Platz haben muss.

Siehe auch Details: https://clean-code-developer.de/die-grade/gruener-grad/#Open_Closed_Principle

Insgesammt stark verbesserungswürdig. Eignet sich aber meiner Meinung nach gut, um objektorientierte Programmierung zu lernen an diesem Beispiel.

Naja, OO ist dann gut, wenn sie eine Verbesserung wäre. Welche Verbesserung hätte es, das Programm in mehrere Klassen aufzuteilen, außer mehr Code? Nicht mal Landei hatte das static moniert.

Zugegeben: public static void folderSize(File directory, long[] countAndSize) { ist Ur-C-Style…

Und ist die main-Methode echt zu kompliziert? Also deren Ablauf nicht mehr nachvollziehbar?

a. Ich bin nicht dein Azubi. ( :stuck_out_tongue_winking_eye: )
b. Die beste Doku des Codes ist doch der Code selber. (Hat jemand mal gesagt…)

Naja dazu müsste man verstehen, dass static nicht im Gegensatz steht oder das Gegenteil von OOP ist. Statische Methoden sind Teil der OOP. Sie werden immer dann verwendet, wenn die Ausführung einer Methode weder vom Zustand des Objekts abhängig ist noch den Zustand des Objekts ändert.

Und Ziel von Code Reviews ist es, sich mit seinem eigenen Code auseinander zu setzen und den kontinuierlich auszubauen und zu verbessern.

Siehe auch: https://clean-code-developer.de/die-grade/roter-grad/#Taeglich_reflektieren

Ich hatte versucht dazu die objektiven Kriterien von CCD dranzusetzen. Diese würde ich dir empfehlen entsprechend nach CCD zu studieren.

Das wäre nur dann der Fall, wenn dein Quellcode auch entsprechenden Konventionen folgt. Ich glaube kaum, dass, wenn man sich allein die main-Methode anschaut, dein Quellcode für irgentwen verständlich ist. Letzen Endes habe ich nicht gesagt, dass der JavaDoc für den User oder eine dritte Person, die dein Quellcode liest, da ist, sondern für DICH selbst. Das ist eine Technik, um ein besseren Quellcode zu schreiben, wie auch meine Vorredner es korrekt beschrieben haben.

Ich schreibe den Text übrigens so, dass wenn ein Dritter das hier liest auch seine Schlüsse daraus ziehen kann. Du musst dich hier nicht rechtfertigen, sondern du musst nun folgendes machen:

  1. Für dich bewerten, ob die Kritik angebracht ist
  2. Wenn ja, dann eine Änderung umsetzen
  3. Wenn nein, dann ist hier nichts zu tun.

Grundsätzlich merkt man schon, dass es dir an professioneller Entwicklungserfahrung mangelt. Würdest du mit anderen Leuten einen gemeinsamen Quellcode entwickeln, dann würden mind. die o.g. Standards in vielen Fällen angebracht werden. Wenn ein halbwegs guter Architekt an Board ist, dann würden solche Reviews bei jedem Commit (Merge Request) durchgeführt werden. Und deinem Architekten kannst du die hier, von dir vorgebrachten, Argumente einfach nicht bringen.

Die Code Review - das sollen die Verweise zu CCD auch zeigen - ist keine vollständig subjektive Ansicht, sondern mind. 75 % methodisch und entsprechend objektiv.

Hier ein persönlicher Tipp: Versuch mal die Tipps umzusetzen. Einfach so zu Spaß. Wenn du ernsthaft hinterher bist Java oder eine andere Sprache zu lernen, dann würdest du das auch machen.

Einerseits finde ich Code-Reviews, auch auf dieser „kleinen“ Ebene, sehr gut und sehr wichtig. Andererseits irritiert mich das Engagement der Antwortenden in diesem Thread schon gewaltig.

Ich sollte vielleicht mal intern eine Strichliste führen, wie oft CB schon gesagt wurde, bitteschön vernünftige Namen zu vergeben. Aber hier mal der passende Pointer: https://youtu.be/aAb7hSCtvGw?t=1827

Und noch kurz zu dem Punkt, den ich für allgemein wichtig halte: Kommentare / JavaDocs. Ich bin ja ein JavaDoc-Nazi. Ja, ich schreibe auch sowas wie

class Person
{
    /** The first name of this person */
    private final String firstName;
    ...
}

und manche empfinden das als „noise“. Aber ich muss mir nicht die Frage stellen „Kommentiere ich das oder nicht?“. Ich tue es einfach. Und direkt nach dem oben schon verlinkten Teil kommt https://youtu.be/aAb7hSCtvGw?t=1973 , was das hier…

…komplett aushebelt.

Kommentare zu schreiben machen einen selbst zu einem besseren Entwickler. Es zwingt einen zu einer anderen Form der Reflexion, und einer anderen Sicht auf den Code. (Das kann man auch erreichen, wenn man die Kommentare nicht schreibt, aber es ist schwieriger). Wenn man versucht, eine Klasse oder Methode zu kommentieren, und man merkt, dass das ~„schwierig“ ist, dann ist das ein Zeichen dafür, dass man nochmal nachdenken sollte.

Ansonsten nochmal ein Link direkt auf den Anfang des Videos: https://www.youtube.com/watch?v=aAb7hSCtvGw - wenn jemand das gesehen und verinnerlicht hat, und nebenbei noch Selbstverständlichkeiten berücksichtigt wie z.B. immer gegen interfaces zu programmieren, dann kann so ein „Code Review“ auf einer so viel höheren Ebene ansetzen, dass es vielleicht sogar „interessant“ wird, und nicht nur ein lästiges, gebetsmühlenartiges Wiederholen der immer gleichen Hinweise, von denen man ohnehin weiß, dass sie beim nächsten Kot-Review, das einem der Autor vorsetzt, wieder runterbeten muss.

Statische Methoden sind eigentlich nicht Teil von OOP weil genau der Teil „OO“ Fehlt „Objekt Orientierte“ …Wenn man es sehr konsequent sieht sind static Methoden typischerweise ein Hinweis auf Fehlende Nutzung von OOP …in der Praxis ist das aber kein Problem…führt zwar oft zu sog. Utility Klassen, die dürfen halt nicht zu groß werden. Statische Methoden gehen mehr in die Richtung funktionale Ansätze gerade in JDK8+ kann man das sehr schön beobachten.

Das ist dann der schlechteste Kommentar, den man machen kann. Was eine Klasse/Methode macht steht im Quellcode. Wenn man den nicht lesen kann (wg. Fehlender Java Kenntnisse) dann sollte man den auch nicht lesen. Die Kommunikation mit dem Leser des Codes findet über den Code statt nicht über Kommentare… Sprechende Variablenamen/Methodenname/SoC etc. helfen hier sehr viel weiter.

Das führt mit der Zeit dann dazu, dass viele/alle Klassen/Methoden Kommentar haben, aber schlicht falsch sind, da Code gerefactored wurde/wird aber die Kommentare nicht mit angepasst werden/wurden… Das ist einfach so…

Das ist ein Thema was man den Leuten dann in einem Projekt wieder mit viel Mühe abgewöhnen muss…weil es verschwendete Zeit ist in Prosa zu schreiben, was im Programmcode bereits steht. Die Zeit ist besser investiert den Leuten beizubringen wie man Tests schreibt und sprechenden Namen usw.

Wichtig ist, einen JavaDoc bzw. Kommentar zu schreiben, warum man etwas macht…z.B. weil gerade keine bessere Idee da war/Aus Performance/Oder man experimentiert gerade/Eine Entscheidung getroffen wurde es so zu machen…weil es z.B. eine Zwischenlösung ist etc. (Dafür gibt es u. A. @ImplNode/@ImplSpec).

Die Frage wäre hier, warum ein Abstraktionslayer für Logging nutzen(SLF4J)? Wenn überhaupt, dann direkt Log4j2/logback nutzen und zwar direkt. Die Frage, ob in dem Falle überhaupt ein Framework einzusetzen wäre. Console reicht völlig. Der Standard Stacktrace liefert hier genug infos…Da kommt es dann tatsächlich auf dem realen Benutzerkreis an…

Abgesehen davon verstehe ich den Zusammenhang mit Maven nicht?

Was haben Code Review mit einem Architekten zu tuen? Abgesehen davon würde ich Review lediglich auf dem finalen Stand eines Branches machen bevor es in den master geht…und nicht bei jedem commit…Abgesehen davon gibt es bzgl. des Thema Reviews durchaus unterschiedliche Meinungen (z.B. Pairen/Mob etc.)… Ja es hilft…aber nicht immer…weiterhin machen Merges eine Historie unleserlich…

Für mich ist das so ein Kommentar al la „Mr. Obvious“ ? Das heißt aus meiner Sicht schlicht Zeit verschwendet… Da der Kommentar keinerlei Zusätzliche Information liefert, als die, die schon aus dem Programmcode hervorgeht…

Oh jeh…Da prallen nun zwei komplett gegensätzliche Ansichten aufeinander.

Als Gegenpart nochmal kurz der schon oben gegebene Link: https://youtu.be/aAb7hSCtvGw?t=1973 , und der Kernsatz, der sich darauf bezieht:

If you don’t have a comment telling you what the specification is, what is the specification? Who knows! You have two choices: Either you guess (in which case your program probably doesn’t work), or you read the code, in which case the implementation becomes the specification, and it’s overspecified, and you no longer have the freedom to change that implementation, at all.


Ein kleiner Abstecher:

IIRC bist du ja Maven-Fan/Experte. Und es gibt einiges bei Maven (und bei dem Weg, ein Projekt in die Maven Central zu bringen), wo ich schon verscheidene Abstufungen von „Genervtheit“ empfunden habe. Aber es gibt eine Sache, die vermutlich viele andere Leute nerven würde, die ich mir aber wünschen würde: Ich finde, wenn man versucht, etwas in die Maven Central zu bringen, und in dem Code taucht irgendetwas auf, was public oder proected ist, und das hat keinen JavaDoc-Kommentar, dann sollte Maven sagen: " :fu: , nö, mach’ ich nicht!".

Aber bevor daraus nun ein Glaubenskrieg entsteht: Es ist auch noch wichtig, zu unterscheiden, an wen man Kommentare richtet (abgesehen davon, dass man sie für sich selbst schreibt), und was man kommentiert. Bei allem, was private ist, kann man noch sagen: Ja, das ist ein Detail, das geht niemanden was an, das kann sich noch ändern. Spätestens bei public und protected hört für mich aber jeglicher Argumentationsspielraum auf.


Wieder allgemeiner: Es geht (bezogen auf den zuerst zitierten Teil) nicht darum, ob man ~„den Quellcode lesen kann“. Ich kann jedenfalls nicht nachvollziehen, wie du so eine Aussage machen kannst, außer wenn du in der glücklichen Lage bist, dass du dich noch nie in eine „größere“ Codebase einarbeiten musstest.

Auf der obersten Ebene ist die Sprache an sich wichtig - Java ist schön und gut, das kennt man, kann man verstehen, gut lesen, und die IDE-Unterstützung ist etwas, wovon man in anderen Sprachen nur träumen kann. Aber spätestens bei Dingen, bei denen man viel guten Willen braucht, um sie noch als „Programmiersprache“ zu bezeichnen (ja, ich schaue dich an, JavaScript) wirst du ohne ausführlichSTe Dokumentation ziemlich schnell kotzen.

Auf einer etwas niedrigeren Ebene spielt der „Stil“ eine Rolle. Jemand, der 20 Jahre klassisches OOP-Java programmiert hat, und auf einmal mit ausgefeilten Stream/Lambda-Konstrukten zu tun hat, wird sich sehr schwer tun, zu verstehen, was da passiert. Ein anderes Beispiel ist, wenn jemand, der „nur Core-Java“ kennt, auf einmal an jeder Methode/Variablen eine Liste von @Annotationen sieht, und schlicht nicht weiß, was die machen. Und auch hier: JavaScript ist im Vergleich dazu nochmal umd Größenordnungen schwieriger zu handlen.

Man kann so gute Namen vergeben, wie man will. Wenn man die Wahl hat, 500 Zeilen Code zu lesen, um zu verstehen, was eine Klasse oder Methode wirklich macht, oder einen zweizeiligen Kommentar zu lesen, um zu verstehen, dass das, was sie macht, für die Aufgabe, die man hat, nicht relevant ist, dann ist letzteres angenehmer und viel ökonomischer.

Und deswegen habe ich, wenn ich Code schreibe, mehrere Ziele. Ich versuche, Code so zu schreiben, dass er

  • leicht lesbar ist
  • nachvollziehbar ist
  • verständlich ist
  • leicht zu testen ist
  • leicht zu ändern ist

Jaja, Binsenweisheiten. Aber immer, wenn ich Code schreibe, schreibe ich diesen Code mit einem, impliziten, übergeordneten Ziel:

Ich schreibe Code mit dem impliziten Ziel, dass niemand diesen Code jemals wieder lesen muss!

(Einschließlich mir selbst!)

(Niemand liest gerne den Code von anderen Leuten. :wink: )

Und ein Weg, um das zu erreichen, ist: Ein klares interface (auch bei privaten Dingen), mit dem klaren JavaDoc
/** The following 500 lines of code are (doing) THIS, and nothing else. Trust me. */

Zu den Kommentaren hat @Marco13 ja ausführlich geschrieben. Und es spiegelt exakt meine grundlegende Haltung zu Kommenraren wieder.

Dazu möchte ich lediglich ergänzen, dass mein Weg es ist den Anfängern auf diese Weise begreiflich zu machen, was SEPERATION of Concerns bedeutet. Weil in der Regel hast du eben kein SoC Verständniss von Anfang an.

Gegenfrage: Warum sollte man hier nicht abstrahieren? - Gerade weil du so viele Logging Frameworks hast, wäre es doch schlau in deiner Anwendung ein entsprechend hohen Layer zu verwenden. Man programmiert ja auch gegen Interfaces und nicht gegen Impl Klassen. (Ausführung siehe oben)

Ansonsten war meine Botschaft, dass SysOuts zu schwach sind. Und wenn man schon Maven nutzt, dann könnte man eine dep hinzufügen und hätte alles für ein korrektes Logging. Es wäre also vergleichbar wenig Aufwand für pot. (nicht hier) aber in größeren Projekten mehr nutzen.

BTW: Wenn man hier nun sagt, dass es hierfür kein Logging Framework und kein Exception Handling braucht, weil das Programm zu klein ist oder die Funktion zu gering, dann würde demnach auch eine CodeReview sinnlos sein und auch der Einsatz von OOP würde ich nach der Argumentation in Frage stellen. Weil dann ist es so, wie @CyborgBeta gesagt hat: Was hilft es, wenn man mehr Code und keine weitere Funktion hat.
Im Ergebnis ist das hier ja ein stark verkleinertes Programm, dass pars pro toto steht.

Sehe ich einfach anders. Ein Merge in den Master beinhaltet mit unter hunderte commits und etliche Änderungen. Eine Code Review dann zu machen würde je nach Releaseplan viel zu weit auseinander liegen. Man könnte die daraus gewonnenen Erkentnisse ggf. nicht mehr nutzen um technische Schulden zu reduzieren.

Und nach drei bis vier Monaten hätte man zusätzlich das Problem, dass sich keiner mehr daran erinnert, warum man das so und nicht anders gemacht hat.

Ebenfalls können in einer frühen Code Review auch fachliche Fehler gefunden werden. Diese lassen sich dann schnell korrigieren, weil der Merge-Request abgelehnt wird. Gleiches verhält es sich mit etwaigen Verstößen gegen Spezifikation oder Architektur.

=> Ganz klar: Jeder größere Merge sollte einem Code Review unterzogen werden.

Dabei spielt die Rolle des Architekten folgendes: Es muss ein erfahrener Entwickler die Code Review durchführen. Es muss jemand sein, der die Spezifikation und die Architektur kennt, um zu überpfüfen, ob die Implementierung dieser genügt. Den kannst du dann nennen, wie du willst. Lead Developer, Senior Developer, Architekt … Föllig egal.

Die letzten 4 Jahre habe ich nach GIT-FLOW gearbeitet. Inherent ist hier das Mergen von Feature Branches in den Develop. Von daher kann ich diese Aussage leider nicht navollziehen, da es zur täglichen Arbeit eines Entwicklers gehört. Man könnte, wenn man will, auch mit rebase arbeiten. Vielleicht meinst du ja das als Alternative.

Also mal eine Statement dazu. Wozu habe ich denn Tests? Das ist die Spec …besser als alles andere. Also kommen wir jetzt davon weg im JavaDoc zu schreiben was der Code macht (wie vorher postuliert) sondern die Spezifikation der Aufgabe zu beschreiben? Meine Meinung dazu: Das ist Aufgabe des Tests.

Also mal klar. Ich bin kein Maven Fan (ok ok … ja bin ich ) und bin eben auch Maven Committer … weiterhin ist die Frage, ob es Aufgabe eines Build Systems ist, zu definieren ob ein JavaDoc Kommentar da sein sollte oder nicht. Wenn Du das haben willst kann man das einfach mithilfe von CheckStyle (Maven Plugin) festlegen …das legt dann aber das jeweilige Projekt fest und nicht Maven noch Central …

Da wäre einfach eine Frage: Was Du unter „größere“ Codebase verstehst?

Wenn ich schon eine Klasse mit 500 Zeile habe, ist meiner Ansicht nach schon etwas schief gelaufen geschweige denn wenn eine Method so lang ist…

Aber genau daran sieht man, dass Kommentare überbewertet sind. Denn welchen Wert liefert dieses Kommentar? Wenn ich dem Code nicht traue - dann traue ich dem Kommentar logischerweise auch nicht.

Ich gebe dir recht, dass öffentliche Methoden/Felder javadocs haben sollten - zumindest wenn man diese anderen Leuten bereit stellt. Aber darin sollte nicht erklärt werden, was die Methode im Detail macht. Dafür ist dann i.d.T der Source-Code da. Ich will wissen was die Methode für Bedingungen hat. Welche Parameter steuern was und wie schaut das Ergebnis im groben aus.

In unseren CodeReviews würde ich jedes JavaDoc ankreiden was zu sehr die implementierungs details erklärt. Warum? Weil nach dem 3ten Bugfix die hälfte vom Kommentar nicht mehr stimmt - aber das Kommentar nie angepasst wurde.

Wie viel Beachtung Kommentare in der echten Welt tatsächlich bekommen sieht man daran, wenn man einfach mal eine größere Lib nimmt und nach „TODO“ sucht. Weswegen die Dinger bei uns nicht mehr erlaubt sind - außer es steht eine Ticketnummer dabei.

1 Like

Üblicherweise Abstrahiert man, um ein Problem zu lösen? Aber welches Problem liegt hier vor? Wenn ich gerade SLF4J anschaue ist dass doch nur dazu da, um ein Logging Framework unten drunter wechseln zu können? Wie oft passiert das? Meiner Erfahrung in den letzten 25 Jahren … Nie. Ich habe noch nie ein Projekt gesehen, dass tatsächlich sein Logging Framework gewechselt hat…somit Abstraktion die nicht Notwendig ist…Abgesehen davon wenn ich tatsächlich mal wechseln wollte/müsste hilft mir hier mein IDE sehr schnell…

Weiterhin zum Punkt bzgl. Interfaces? In welchen Fällen macht das Sinn? Allgemein macht das keinen Sinn…Wenn ich Datentypen wie z.B. Set/Map etc. nehme bin ich voll dafür…aber wenn ich z.B. einen Spring Boot Controller schreibe? Soll ich dann für den Controller auch noch ein Interface erstellen? Wozu? Welchen Vorteil liefert mir das?

Mit dem hinzufügen der Abhängigkeiten ist es aber nicht getan… Wenn man das richtig machen will, dass es auch etwas bringt, ist bedeutend mehr notwendig… In dem kleinen Beispiel sind System.out bzw. die Exception mehr als ausreichend. Richtiges Logging in einer Code Base ist nicht zu unterschätzen gar im Zusammenhang mit entsprechenden Systemen (ELK etc.) …Analyse Tools usw. Auch da sehe ich die Nutzung von SLF4J sehr kritisch…wie schon oben beschrieben.

Dann ist der Merge bzw. Branch zu groß und keiner kann wirklich eine Review etc. drauf machen und abgesehen davon läuft dann der Branch und der Master bzw. die Änderung zu weit auseinander… Merges bzw. Branches müssen klein sein…Branches dürfen nur kurzweilige sein (1-5 Tage). Wenn ich größere Sachen Anpassen muss, dann werden die auf mehrere kleinere Branches aufgeteilt… geschweige denn wenn ich nicht mit Branches arbeiten möchte was ja durchaus sinn macht… Branches sind ein wenig gegenteilig dem CI Gedanken…Weiterhin kann man die Reduktion bzgl. technischer Schulden einfach mit einfachen kleinen Branches in den Code einbringen (z.B. Refaktorings, Code Cleanups etc. etc.) Das ist einfacher und sauberer…

Und für mich noch wichtiger jeder Commit (Merge auf den Master) hat einen klare Funktion…und nicht mehrere Sachen auf einmal z.B. Technische Schulden beseitigen und gleichzeitig ein neues Feature reinbringen…

Im Rahmen eines Reviews wird das Problem besprochen/beschrieben und wenn man uneins ist oder Unklarheiten auftauchen, wird geprüft, ob das eventuell ein fachliches Problem ist, welches unklar/falsch gedeutet/spezifiziert wurde usw. Dann wird dass geklärt und entsprechend angepasst… Der Entwickler arbeitet gegen die Spec? Architektur kann man auch klären…

Welches Problem soll damit gelöst werden? Ich möchte ein Feature entwickeln und möglichst schnell ausliefern …? Somit einfach einen Feature Branch …Code Review und in den Master (ja z.B. Rebase oder eben auch Merge. Ich finde Merges gerade in Git recht schlecht verständlich; Ich arbeite seit 12 Jahren+ mit Git;)… Wozu brauche ich develop Branch? Und einen Release Branch?

Was ist eine größerer Merge? Ab wann gilt ein merge als groß?

Also kann kein sog. Junior ein Review machen? Das führt wieder zu dem Problem des Silo Wissens…Wenn reviews immer von eine kleinen Gruppe von Leuten gemacht wird führt das zwangsweise zu begrenztem Wissen in dieser Gruppe von Leuten… Sinn und Zweck von Reviews ist aber das Wissen über den Code im Team zu verteilen…

Also hier scheinst du etwas missverstanden zu haben :wink: Ich entwickle ja kein Interface „random“, sondern da wo es Sinn macht. Und dann implementiert man dann gegen dieses Interface, wenn es vorhanden ist. Ist doch ganz klar. Etwas anderes konnte aus der gesamten Diskussion meiner Ansicht nacht nicht gelesen werden können.

Und ehrlich gesagt habe ich noch nie eine ArrayList gegen LinkedList tauschen müssen :wink: Wobei wir von log4J zu logback gewechselt sind.

Ich würde hier also tendieren zu sagen, dass eigentlich klar ist, was man „gegen interface programmieren“ meint. Und da würde ich jetzt im Rahmen dieser Unterhaltung kein weiteren Punkt draus machen.

Jetzt klingst du ein bisschen „veraltet“.

Nein, überhaupt nicht. Schau dir mal Git-Flow an.

Ok…dann habe ich das missverstanden… Haken dran.

Den musst Du mir erklären?

  1. Ich kenne Git Flow schon etwas länger…
  2. Ich verstehe den Satz nicht? Oder soll ich den so verstehen, dass ein Commit verschiedene Sachen machen darf? Sprich Feature + Refactoring in einem commit?

Indirekt schon: Ich hatte vorgeschlagen, eine Instanz mit Feldern (wie count und size) zu verwenden, damit wären einige statische Methoden nicht-statisch geworden.

Naja - SLF4J ist jetzt nicht „Neu“ aber dennoch weit verbreitet. Und gefühlt „jeder“ setzt bei der Entscheidung zum Logging auf SLF4J und dann „ist egal“ was drunter ist.

Jetzt zu sagen „nein“ das brauche ich nicht, finde ich recht altmodisch. Vor allem, weil der Trend ja gerade zu dieser Nutzung geht.

Wir gehen von einem Flow wie diesem aus: Feature => Develop => RC => Master

Jetzt hast du ein Release: Quartalsweise

Ein Feature kann sein: Refactoring, neue Funktion, Bugfix etc.

Dann sammelt sich das ja alles im Develop. Daraus wird ein RC, da sammeln sich alle Bug fixes, und dann hast du ein riesen merge von den ganzen Features und Bug fixes im Master…

Wenn du ein Team aus 10 Developern hast, dann ist das eine ganze Menge, die da zusammen kommt. Wer soll das wann reviewn? - Außerdem ist das dann schon nach Tests und nach allem… Auch zu spät ein refactoring zu machen.

Zu der Sache mit SLF4J (was ich auch immer nehme und auf der Arbeit das tatsächliche Framework auch schon getauscht wurde dann):
Wenn man verschiedene Bibliotheken am Ende einbindet die alle sagen: Nö wir nehmen ein explizites Framework, ist uns doch egal. Dann hat man am Ende Kraut und Rüben mit dem Logging wo man zig konfigurationen braucht, anstatt dass der, der die Sachen benutzt vorgeben kann was genommen wird, je nach Bedarf