Ein guter Programmierer oder einer, der es werden will, sollte Ratschläge immer anhören. Und wenn dabei der Sinn, der hinter der Verbesserung steckt, auch so vorgebracht wird, das deutlich wird, warum der Code hinterher besser ist und welche Vorteile das bringt, vor allem ja für den, der ihn geschrieben hat, sollte das eigentlich auch angenommen werden.
Eigentlich bin ich der größte Kritiker meines Codes und refakturiere immer vor mich hin, wenn ich auch Codestellen stoße, die mir als Verbesserungswürdig auffallen. Sei es, weil ich es inzwischen besser weiß, sei es, dass es schon im Vorhaben, die Methode, die Klasse, oder was auch immer, später aufzuteilen geschrieben und dann aber vergessen wurde.
Ach ja, ich hab auch noch ganz andere Umgangsformen gesehen vor ein paar Jahren, bis hin zu physischem Kontakt, man musste dann die Leute in andre Teams stecken und schiesslich die Teams in andere Gebaeude.
Uebrigens, es ging um Java vs C/C++ Diskussionen die ausgeartet sind, kein Scheiss, war keine theoretische Diskussion sondern es ging um die Entscheidung wie der Kernel einer Simulations SW implementiert werden sollte.
Klar soll man kritisieren, das ist IMHO der beste Grund fuer PRs, „feedback“, man rechtfertigt seine Entscheidungen, korrigiert andere usw.
Oft ist es eben nicht einfach zwischen „das ist ja echte Scheissarbeit die du da abgegeben hast“ und „du bist scheisse“ zu erkennen, wir haben alle Stimmungen und Launen, der kritisierte aber auch der kritisierende, sind ja schliesslich keine Maschinen.
Wenn das Ziel ist zu etwas rueberzubringen geht man anders ran als wenn das Ziel ist jemandem den Tag zu versauen.
„Der Boss“ war immer der Meinung dass er die beste SW der Welt entwickeln laesst, hat selbst nie entwickelt, war ja Manager oder Director
In Teams sollte sich IMHO jeder um Qualitaet kuemmern und feedback geben, man muss sich halt auf etwas einigen, und nicht jeder SW Entwickler versteht dasselbe unter Qualitaet.
Dem Boss war es gut genug wenn das Ding kompiliert und ein bisschen Farbe auf die UI bringt, wenn man auf Buttons drueckt passiert was, dann hat er das schon als „AI powered disruptive new milestone in the history of humankind“ verkauft
m.E. sind die jungen die direkt von der Uni kommen, die alles Wissen und ihre Faehigkeiten komplett ueberschaetzen, dann muss man ploetzlich ernsthaft erklaeren warum der sein Haskell krams fuer seine privaten Projekte behalten darf, mir egal was sein Professor ihm erzaehlt hat.
Eklaert man ihm dass das alles recht komplex ist war er da in geschrieben hat und schwer zu warten (und mir wird ganz ehrlich uebel beim lesen der „Einzeiler“ die sich ueber 5 Zeilen ergiessen in FP), muss man sich nicht selten anhoeren dass man selber wohl nicht „gut genug“ ist wenn einem das schon zu komplex ist
Anstatt einer for Schleife mal schnell 'nen Integer Stream nutzen, alles schon gesehen
Ach ja, Pull Requests blockieren/ablehnen mache ich oefters, nicht nur wenn der Code murks ist, sondern vor allem wenn man den Rotz schlicht nicht braucht, man muss nicht alles optimieren/verkomplizieren und nicht ueberall pseudo-innovativ sein.
Kommt natürlich auf den Use-Case an, aber ich sehe nicht, warum das inhärent falsch sein sollte.
In unserem Produktionscode habe ich höchstselbst eine Utility-Methode Stream<B> zipWithIndex(Stream<A> stream, BiFunction<A, Integer, B> combiner) geschrieben, und ich schäme mich kein bisschen dafür
Ich stimme gern zu, dass Streams nicht immer die beste Lösung sind (mir gehen sie oft nicht weit genug, oft ist vavr.io deutlich besser), aber wenn man sie nutzt, sollte man sie auch so aufbohren, dass es Spaß macht, und man nicht zwischen den verschiedenen Programmierstilen hin- und herspringen muss.
Du bekommst einfach den Index zu den Elementen geliefert, und kannst sie dann verknüppern: zipWithIndex(Stream.of("a","b","c"), (s,i) -> s+":"+i)) gibt einen Stream mit „a:0“, „b:1“, „c:2“.
Bin ich froh, dass ich nicht in deinem Team bin. Nicht weil du etwas nicht kennst, sondern wegen der Einstellung: „Kenn ich nicht, gibts nicht!“. Bei einem Code Review kann und soll auch der Reviewende etwas lernen - ist zumindest meine Meinung.
Ein Kollege meinte mal zu mir, beim Code-Review soll ich mir nicht die Frage stellen, ob alle möglichen Regeln eingehalten wurden, sondern nur, ob ich den Code pflegen kann, wenn der andere im Urlaub ist/kündigt/krank wird.
Um ein Zitat aus dem Journalismus zu bemühen “Einer muss sich immer anstrengen, entweder, der der’s schreibt. Oder der der’s liest”.
Ich denke Uncle Bob gibt gute Ratschläge, wie man Code so schreibt, dass er leichter verständlich ist. Aber seine Regeln sind eher so etwas wie Grammatik.
Der zweite Teil, sind dann natürlich die Vokabeln. zipWithIndex ist für @Landei ein absolut valider Begriff, weil er sich schon lange mit funktionaler Programmierung beschäftigt. Für @anon2594669 ist der Begriff nicht geläufig. Wenn man jetzt @Landei in einem Team ist, dass noch kaum Erfahrung mit funktionaler Programmierung hat, dann gibt es 3 Optionen:
Dem Team funktionale Programmierung beibringen, also das Vokabular lehren
Solche Konstrukte sein lassen und sich dem Team anpassen
Es einfach durchdrücken und sich ärgern mit was für Idioten man zuusammen arbeitet
@anon2594669@Landei Ich denke das ist ein gutes Beispiel dafuer dass „Sinn fuer Qualitaet“ vom Team abhaengt und eben nicht abslut ist, auch was Spass macht kommt auf die Leute an, da braucht es eine Einigung im Team.
Ein team bei uns hat eine interessante Regel:
„Wenn die Reviewer deinen PR nach 45 Minuten lesen nicht verstanden haben, wird abgelehnt“
Ein typisches Problem IME ist, das Leute Wochenlang an einer Sache arbeiten und erst dann den ersten PR aufmachen, dutzende/hunderte Dateien sind geaendert, viel zu Komplex um da schnell durchzusteigen, keine Iterativen kleinen Aenderungen sondern einen Berg an Code… da kann das Team danach wohl kaum ohne den Author weitermachen.
Das an einem so spezifischen Beispiel wie dem zipWithIndex festzumachen, ist schwierg (vor allem, wenn Leute biased sind ). Aber bei mir würde die Frage, ob das bei einem Review durchgeht, auch davon abhängen, ob es eine
package de.company.core.important;
public class Customer {
public String str0;
public String str1;
public String str2;
@SuppressWarnings("rawtypes");
public synchronized <A, B> Stream<B> zipWithIndex(
Stream<A> stream, BiFunction<A, Integer, B> combiner) {
...
str0 = "Oh noes!";
str1 = "We got a state here!"
...
}
}
wäre, oder eine
// Copyright 2018 ....
package de.company.common.utils;
/** Some useful comment here */
class StreamUtils {
/** Some useful comment here */
static <A, B> Stream<B> zipWithIndex(
Stream<A> stream, BiFunction<A, Integer, B> combiner) {
...
}
...
}
Den Punkt, wo es sich lohnt, Code quasi „zeilenweise“ durchzulesen, muss man erstmal erreichen. Wenn man die Datei aufmacht, und der Anfasser der Scrollbar im Editorfenster zu einem Quadrat zusammenschnurrt (oder … *dramatische Musik* eine horizontale Scrollbar erscheint!) muss man eigentlich schon nicht weiterlesen Das dann klipp und klar zu sagen, und zumindest eine baseline einzufordern, ist aber wohl vielen schon zu „uncompassionate“.
Das passt bei Code mehr, als bei allem anderen, was so geschrieben wird. Und wenn man merkt, dass jemand sich beim Schreiben nicht angestrengt hat, ggf. weil er mit der „nach mir die Sintflut“-Mentalität einfach nur seine Aufgabe mit minimalem Aufwand erfüllen und in der restlichen Zeit auf Reddit rumsurfen will, kann man IMHO auch mal Klartext reden.
Bei mir ginge das wohl nicht durch das Review, weil mit die Methode zu generisch ist. Natürlich ist hier so etwas schnell von mir hingeschrieben. Im Team würde das dann ggf. diskutiert werden. Dann lernt auch jeder etwas dabei.
Generische Utiltiymethoden finde ich aber wirklich selten sinnvoll. Das ist bei diesem Beispiel (mMn) auch der Fall. Finde ich nicht gut, darfst Du gerne gut finden.
Die Stream-API hat so einige Lücken, und wenn man die schließen will, braucht man halt solche „generischen“ Methoden. Ist nicht schön, auf Utility-Klassen ausweichen zu müssen, aber immer noch besser, als einen wilden Stil-Mix zu bekommen.
Da hast Du nicht unrecht. In Java ist das nicht besonders toll gelöst.
Ich hätte vermutlich in Kotlin als Extension damit auch weniger Probleme. In Deinem Java-Beispiel weiß man erst einmal nicht, dass der Consumer sich auf den Stream bezieht (zumindest nicht anhand der Signatur).
Das dann klipp und klar zu sagen, und zumindest eine baseline einzufordern, ist aber wohl vielen schon zu „uncompassionate“.