Backup Programm - Code-Review von euch erwünscht

[kama]

@martin123

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.

Nee neu ist das Teil bei weitem nicht.
Ich bemängele halt, „Machen doch alle so.“ … Das mit darunter ist so eine Sache (wg. Konfiguration) abgesehen davon ist das Thema Speicher/Resourcen durchaus wieder ein Thema in Zeiten von Cloud usw. Ich bin eben dafür genau zu hinterfragen welchen Nutzen ich dadurch bekomme.

@martin123

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

Ich bin nicht der Meinung, dass man es nicht braucht. Es gibt durchaus Fälle wo es Nutzen bringt. Ich lege halt an der Stelle den Fokus auf „Nutzen“ …Aber es einfach nur so zu machen, weil es Trend ist halte ich für den falschen Weg.

@martin123

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

Also ein sehr konservativer Flow… (Sorry das ist jetzt „veraltet“ SCNR ;-))…

Warum mache ich denn solche Schritte von Feature auf Develop? und Dann in RC => Dann in Master???.. Releases Quartalsweise ist ja nun wirklich altbacken …Somit weit von CD etc. oder zumindest der Möglichkeit von CD entfernt. Die Frage ist warum benötige ich eine solche komplexe Branching Strategie? Wo ist das Problem? Werden noch manuelle Tests irgendwo gemacht? Welchen Nutzen liefern mit die Branches Develop/RC/ ? Was ist der Unterschied zwischen Develop/RC/Master? Welches Kriterium wird hier genommen? Vor allem der Unterschied RC<=> Master ? Also RC ist eine Version ein sog. Release Candidate ? Was ist dann auf dem Master immer nur der finale Release Stand OHNE RC ?

@martin123

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.

Unit Tests / Integrations Tests laufen bei jedem Commit auf jedem Branch zzgl. eventueller E2E Tests jede nacht noch mal auf dem Master…Bei einer entsprechenden Einschätzung eines Devs auf dem Jeweiligen Feature Branch werden die E2E/IT auch dort ausgeführt. Bisher hatten wir dieses Jahr zweimal den Fall, dass wir den Master tatsächlich kaputt hatten (Fehlgeschlagene(n) Tests). 2.5 h hat es im schlechtesten Fall gedauert…im anderen Fall hat 1.5 h gedauert…wg. E2E die sind aktuell unser Thema. (IT’s sind an einigen Stellen auch in Diskussion).

Wenn alles Unit Tests / IT’s / E2E durch sind definieren wird das als Lieferfähig (Das war am Anfang ein sehr hartes Brett was wir gebohrt haben). Das bedeutet, dass wir in der aktuellen Situation zwischen jeder Lieferung mind. 1:45 h Zeit benötigen (wenn E2E + UT + IT + Build laufen)…Das würde bedeutet, dass eine Änderung (neuer Commit) auf Master bis Lieferfähigkeit mind. 1:45 h dauert zzgl. Implementation / Analyse etc. . Also die technische Hürde hier bei 1:45 h liegt (an der arbeiten wir noch).

Jeder Branch wir gereviewed…keine Ausnahme…Verteilung im Team(10 Leute; Gleiches Spiel habe ich schon deutlich größeren Teams eingeführt.). Wir rebasen die Branches immer gegen den Master … in den Master wird ausschließlich per Fast-Forward gemerged (Keinerlei Merge commits). Ein Commit ein Feature bzw. Anpassung (Ticket basiert). Max. größe für einen Branch bzw. Ticket max. 1 PT (Aufwand)… bisherige Lieferungen Monatsweise seit 2 Wochen sind wir auf Wöchentlich; Eventuell sogar noch höhere Taktung)… Auf Grund der begrenzten Größe der Änderungen (Commits) ist somit auch noch ein Review möglich. Die Größe bzw. Menge war für viele am Anfang ein Problem (da gab es dann auch die Monster Commits…) ist aber in der Zwischenzeit (seit ca. 1.5 Jahren) ist das sehr gut geworden.

Aktuell sind wir dabei das Mergen in den Master (Auslösung durch Dev) und Rebases in den Feature Branches vollständig zu automatisieren…da das einfach Manuelle Arbeit ist die in 99.99% der Fälle automatisch geht.

[Marco13]

@kama

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.

Die Tests helfen nicht, den Code zu verstehen. Im Gegenteil. Of wird bei Tests mit recht hohem Aufand („künstlich“) ein bestimmter Systemzustand herbeigeführt, bei dem dann das Verhalten einer einzelnen Methode getestet wird. Man kann mit hohem Aufwand (mit viel Lesen) aus den Tests ableiten, ~„was die Spezifikation ist“. Aber wenn ich über Code browse, und verstehen will, was eine Methode updateCustomer(customer, product) genau macht, will ich mir nicht den Code und schon gar nicht den Test durchlesen müssen - da hat gefälligst dabei zu stehen: /** Adds the given product to the list of purchased products of the customer */ (jaja, die Methode könnte dann addToPurchasedProducts oder so heißen - ich denke aber, der Punkt sollte klar sein).

@kama

weiterhin ist die Frage, ob es Aufgabe eines Build Systems ist, zu definieren ob ein JavaDoc Kommentar da sein sollte oder nicht.

Nun, damit etwas in der Central landern darf, müssen einige strenge Bedingungen erfüllt sein - und in Zweifelsfall „cheaten“ sich die Leute da drumrum, indem sie eine leere JavaDoc JAR erstellen Und selbst wenn sie automatisch während des deployments erstellt wird, ist sie, sofern kein JavaDoc da ist, praktisch „leer“. Ich find’ das halt blöd.

@kama

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

So groß, dass man nicht an einem Tag den Überblick über alles bekommen kann, und schon für die Einarbeitung in den Teil, der für die ersten (einfachen) Aufgaben relevant ist, mehrere Tage benötigt werden.

@kama

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…

Ich habe nicht gesagt, dass eine Klasse oder Methode so lang ist, sondern dass man so viele Zeilen lesen muss - und das ist schon eine sehr niedrige Zahl. Selbst wenn die Methode „sauber“ und „kurz“ ist, kann schon bei sowas wie

void updateCustomer(Customer customer, Product product) {
    validateDatabaseConnection();
    DatabaseHandle databaseHandle = obtainDatabaseHandle();
    CustomerDetails customerDetails = 
        extendCustomerDetails(databaseHandle, customer.getId(), product);
    updateCustomerDetails(customerDetails, databaseHandle);
    closeDatabase(databaseHandle);
}

der Aufruf-Baum, den man im Kopf mit vielen Stack-Push+Pop-Operation durchlaufen muss, aus tausenden Zeilen bestehen. Und wenn man wissen will, wo ein bestimmter Bug liegt, kann es hilfreich sein, diese Baumsuche schnell abzubrechen, wenn man einen JavaDoc-Kommentar sieht, der sagt „This only writes the result to the DB, as it is - your bug is in another castle“.

@Tomate_Salat

Aber genau daran sieht man, dass Kommentare überbewertet sind. Denn welchen Wert liefert dieses Kommentar?

Nun, abgesehen davon, dass er einen zwingt, sich klar zu machen, was man da gerade gemacht hat, und er es jemandem vielleicht erspart, den Code lesen zu müssen, können bestimmte Kommentare extrem hilfreich sein. Um mal bei dem void updateCustomer(Customer customer, Product product)-Beispiel zu bleiben: Was macht diese Methode? Ja, man kann raten. Man kann sie vielleicht auch addToPurchasedProducts nennen, um „weniger raten“ zu müssen. Aber sowas wie

/** 
 * Adds the given product to the list of purchased products of the given customer.
 * 
 * This will update the list in the database. If the given product is null, or a 
 * product with the same Product#getType is already in the list, then nothing
 * will be done.
 * ...
 */

Sowas in einem Methodennamen wie addProductToListOfPurchasedProductsIfItIsNotNullAndNoProductWithTheSameTypeWasAlreadyPresent unterzubringen wäre doch etwas sperrig - und sowas durch das lesen des Codes rauszufinden könnte (je nachdem, wie tief man in den Call-Tree browsen muss, und wie „leicht erkennbar“ die entsprechenden Constraints sind) einige Minuten dauern, oder auch eine Stunde, oder, im schlimmsten Fall, übersehen werden und zu Fehlern führen.

Dass man im Kommentar keine Implementierungsdetails unterbringen sollte, ist klar. Aber es muss meiner Ansicht nach mindestens eine weitere Schicht geben, zwischen

• der Information „In der README dieser Lib steht, wozu sie gut ist“, und
• der Information „In foo/bar/Customers.java, in Zeile 312, wird mit der cusomer.getID auf eine Map zugegriffen“

Oder so: Code ist viel zu low-level, als dass man damit schnell und effizient ein mentales Modell der Programmstruktur aufbauen könnte.

[Tomate_Salat]

@Marco13

Um mal bei dem void updateCustomer(Customer customer, Product product) -Beispiel zu bleiben: Was macht diese Methode? Ja, man kann raten. Man kann sie vielleicht auch addToPurchasedProducts nennen, um „weniger raten“ zu müssen.

Klingt für mich nach einer Ausrede für schlechten Code zu haben. Das was du an Information gewinnst verlierst du sehr schnell wieder. Wenn du über eine Methode drüber ließt, die weitere Methoden aufrufen, dann schaust du dir ganz sicher nicht das javadoc einer jeden Methode an.

Btw: bringen wir das negativ-beispiel. Was ist wenn das JavaDoc nicht mehr aktuell ist und du dich aber darauf verlässt? Dann bist du doch besser gezwungen den Code einer Methode zu lesen. Der lügt nämlich nicht.

@Marco13

Sowas in einem Methodennamen wie addProductToListOfPurchasedProductsIfItIsNotNullAndNoProductWithTheSameTypeWasAlreadyPresent unterzubringen wäre doch etwas sperrig […] (je nachdem, wie tief man in den Call-Tree browsen muss, und wie „leicht erkennbar“ die entsprechenden Constraints sind)

Brauchst du auch nicht. Das musst du beim aufrufen nicht wissen. Es reicht aus, dass es passiert. Und was passieren soll, sollte nicht über das JavaDoc vorgegeben werden, sondern über einen Test.

@Marco13

Dass man im Kommentar keine Implementierungsdetails unterbringen sollte, ist klar. Aber es muss meiner Ansicht nach mindestens eine weitere Schicht geben, zwischen

Aber genau das hast du in diesem Beispiel drin. Imho sollte diese Methode nichtmal wissen, dass das ganze in einer Datenbank landet.

Wie gesagt: ich bin nicht gegen JavaDocs. Aber diese müssen so allgemein sein, dass du Sie an ein Interface packen kannst. Ein JavaDoc stellt auch niemals den Vertrag - schon alleine deswegen nicht, weil du es nicht automatisieren kannst. Dafür hast du letztendlich Tests.

Das Problem an JavaDocs sind nämlich nicht die JavaDocs sondern der Mensch davor. Wir haben in der Firma schon sehr viel Arten der Dokumentation ausprobiert und bisher war meine Erfahrung die: je näher am Code desto besser. Selbst das schreiben von Tests versagt regelmäßig. Aber die die existieren können brechen und zwingen einen dann dazu, etwas anzupassen. Zusätzlich kannst du mit Sonar erzwingen, dass eine TestCoverage eingehalten werden muss. Da sind dann zwar auch einige Schwachsinnstests dabei - aber es wurden auch genügend sinnvolle forciert.

Vermutlich könntest du auch JavaDocs mit tools erzwingen. Aber wie gesagt: du kannst nicht automatisiert prüfen ob diese auch überhaupt noch gültig sind.

[CyborgBeta]

Ich sehe es ähnlich, an gewissen Stellen sind JavaDoc Kommentare vielleicht angebracht, aber generell bleibt gut geschriebener Code die beste Dokumentation.

Noch ein Punkt vielleicht, „die Methode macht zu viel…“ Früher gab es mal einen Punkt, dass Methoden vertikal nicht zu lang werden durften… heutzutage hat man sich von diesem Gedanken allerdings wieder verabschiedet.

[Tomate_Salat]

@CyborgBeta

Früher gab es mal einen Punkt, dass Methoden vertikal nicht zu lang werden durften… heutzutage hat man sich von diesem Gedanken allerdings wieder verabschiedet.

Wohl eher du. Nach wie vor gilt, dass man Methoden möglichst kompakt halten sollte. Und wenn man sich ans SRP hält, dann funktioniert das schon fast von ganz alleine.

[Marco13]

@Tomate_Salat

Klingt für mich nach einer Ausrede für schlechten Code zu haben. Das was du an Information gewinnst verlierst du sehr schnell wieder.

Klingt für mich nach einer Ausrede, keine Kommentare zu schreiben

@Tomate_Salat

Wenn du über eine Methode drüber ließt, die weitere Methoden aufrufen, dann schaust du dir ganz sicher nicht das javadoc einer jeden Methode an.

Das ist der Punkt: Wenn man sich nicht die JavaDocs der Methoden ansieht, was dann? Den Code jeder einzelnen Methode lesen? Und deren aufgerufene Methoden auch? Für mich wäre das Zeitverschwendung.

@Tomate_Salat

Was ist wenn das JavaDoc nicht mehr aktuell ist und du dich aber darauf verlässt?

Dann hat jemand einen Fehler gemacht. Bug report und fertig.

@Tomate_Salat

Brauchst du auch nicht. Das musst du beim aufrufen nicht wissen. Es reicht aus, dass es passiert.

Wenn der Bug report lautet: „Jedes Produkt landet nur ein mal in der Liste der gekauften Produkte“, dann sieht man bei einem Blick auf die JavaDoc: Das ist „by design“. Man muss gar nicht prüfen, ob da ein Fehler vorliegt. (Vielleicht muss man schauen, was man ändern muss, damit das Verhalten anders ist, aber die JavaDocs der aufgerufenen Methoden sollten einen schnell zu dem Punkt führen, wo die Änderung vorgenommen wird. Und ja, mir ist klar, dass die JavaDoc der aufrufenden Methode dann vielleicht vergessen wird. Aber da muss man dann irgendwann etwas an den Tag legen, was man „Gewissenhaftigkeit“ nennen könnte).

Allgemein kann man da wohl (offensichtlich) sehr unterschiedliche Ansichten haben. (Ich würde aber gerne mal ein Repository von einem der Anti-JavaDoc’er sehen…)

[Tomate_Salat]

@Marco13

Klingt für mich nach einer Ausrede, keine Kommentare zu schreiben

Nö. Ich hab nie was gegen Kommentare gesagt - sie müssen halt nur einen Mehrwert bieten.

@Marco13

Das ist der Punkt: Wenn man sich nicht die JavaDocs der Methoden ansieht, was dann? Den Code jeder einzelnen Methode lesen? Und deren aufgerufene Methoden auch? Für mich wäre das Zeitverschwendung.

Angenommen du hast eine Methode die 20 weitere aufruft. Da schaust du dir weder den Code der Methoden an noch das JavaDoc. Die Orientierung ist der Methodennamen. Deswegen ist der Methodennamen immer Teil der Dokumentation.

@Marco13

Dann hat jemand einen Fehler gemacht. Bug report und fertig.

Das ist eben der Punkt. Richtig - dann hat jemand was falsch gemacht. Nur die die nicht die JavaDocs updaten werden auch nicht die JavaDocs prüfen. Es kommt also nicht zum Bug Report.
Für öffentliche libs wird das natürlich etwas zuverlässiger funktionieren, weil manche Nutzter sowas reporten.

@Marco13

Wenn der Bug report lautet: „Jedes Produkt landet nur ein mal in der Liste der gekauften Produkte“, dann sieht man bei einem Blick auf die JavaDoc: Das ist „by design“.

Da widerspreche ich dir auch nicht. Das Problem ist ja auch nicht das initiale JavaDoc sondern das, wie es nach einiger Zeit ausschaut. Use-Cases können und werden sich mit der Zeit ändern. Sei es wegen dem Business, der Nutzbarkeit oder rechtlichen Anforderungen.
Wird das JavaDoc nicht mehr angepasst, dann erzählt es ab dem Zeitpunkt eine Lüge. Im worst-case entdeckt das jemand, hält den existierenden Code für einen Bug und führt das alte Verhalten wieder ein.

Deswegen würde ich JavaDoc niemals als Vertrag durchgehen lassen. Dein Vertrag sind die Tests. Die müssen bei einer Änderung mit angepasst werden. Und das was da drin steht, davon kann man ausgehen: das ist gewollt! Solange dein Ticket nicht sagt: änder das, dann hat der restliche Code auch weiterhin so zu funktionieren.

@Marco13

Und ja, mir ist klar, dass die JavaDoc der aufrufen de n Methode dann vielleicht vergessen wird. Aber da muss man dann irgendwann etwas an den Tag legen, was man „Gewissenhaftigkeit“ nennen könnte).

Ganz ehrlich: wenn das funktionieren würde wäre das toll. Wenn das bei dir (auf Arbeit) funktioniert, dann ist das beneidenswert. Meine Erfahrung zeigt aber, dass sowas am Anfang versucht wird gewissenhaft zu machen - aber von Zeit zu Zeit lässts nach.

Nach wie vor bin ich ja auch überhaupt nicht gegen JavaDocs. Meine Emfpehlung wäre halt es möglichst allgemein zu halten (so dass es zu einem interface passt). Das dürfte dafür sorgen, dass das JavaDoc länger gültig bleibt (auch wenns vergessen wird).