Neulich haben wir im Team mal über Code Reviews gesprochen, weil unsere jüngsten Teammitglieder damit noch nicht so firm waren. Ich habe dazu einen kleinen Vortrag gehalten und hatte sehr viel Spaß daran mir ein paar Namen für diese Anti-Pattern auszudenken. Eigentlich ist es ja positiver und netter die Pattern statt der Anti-Pattern vorzustellen, aber wo ist denn da der Spaß? 😉 Das ganze ist natürlich nicht nur auf die Überprüfung von Quelltext, sondern auf nahezu alle Peer Reviews anwendbar. Allerdings unterstelle ich mal Softwareentwicklern, dass die Frequenz mit der man solche Prozesse durchläuft hoch ist. Ganz klar ist hierbei natürlich: nobody’s perfect! Auch ich nicht. Ich erkenne mich in manchen wieder und bin gespannt, ob ihr auch. 🙂

Anti-Pattern bei zu Bewertenden

Der Gottkomplex

„Ich hab da mal noch was mit gemacht“. Wenn das in einem vernünftigen Rahmen ist, kann das sehr cool sein. Die Kollegen sind bestimmt dankbar, wenn ein ekliges Übel endlich weg ist, um das man schon lange herumschlich. Treibt man das allerdings zu weit, dann wird das ganze schnell zu einem „Brocken“ (siehe unten) und frisst mehr Zeit als überhaupt für die Aufgabe angedacht war und mehr Ressourcen als man vielleicht hat. Hinzu kommt, dass wenn man etwas dermaßen Großes einfach mal so nebenbei macht, dass man später nur noch mittels der Historie der Versionsverwaltung überhaupt nachvollziehen kann, wann dieser große Change gemacht wurde. Bei fachlichen Storys, Bug-Fixes, etc gibt es sicherlich irgendwo ein Ticket oder eine Story, in der ein paar Sätze dazu stehen. Bei sowas nicht, weswegen sie später eventuell mal nicht mehr nachvollziehbar ist und nur mit viel Geklicke in der Versionsverwaltung mal wieder gefunden werden kann, wenn Fragen dazu aufkommen.

Der Schweizer Käse

Loch an Loch und hält doch? Was mir öfter zugespielt wird, sind so Merge Requests bzw Reviews im Stile von „Ist zwar noch nicht fertig, aber ich habe noch Probleme bei … . Kannst du mal gucken?“ Wenn das mal ist, geht das schon klar. Dafür sind wir ja ein Team und die Diffs sind schon besser anzuschauen als wenn man sich durch den ganzen Code klicken soll. Aber normalerweise sollte das, was man da zur Review freigibt auch fertig sein. Wird das nämlich zur Regel sich solchen Schweizer Käse zuzuschieben, dann investiert und vergeudet man jedes mal Zeit. Denn meistens ist coden ja nicht nur coden. Beispielsweise wenn vereinbart ist, dass man auch als Reviewer einen Build und Klicktest macht oder sogar irgendwelche Tests und Test Coverage prüft, dann macht man den ganzen Senf jedes Mal. Jeeeeedes Maaaal. Drum prüfe, was du zur Review gibst.

Der Brocken

+ 1.589.283 Zeilen Code, mmmmh …? Etwas viel, oder? Da freut sich doch der Reviewer … nicht! Ein dermaßen großer Diff gehört wohl eher gesplitted, weil ansonsten der Reviewer ewig sitzt. Es ist außerdem naheliegend, dass die Anzahl an angefassten Dateien einfach unheimlich schwer zu überblicken ist. Online-Tools wie Gitlab gehen beim Darstellen eines solchen Changes auch durchaus mal in die Knie. Wahrscheinlich sollte man sich eh fragen, ob das wirklich sinnvoll war, wenn es einen solchen Overhead generiert.

Der „Mein Baby gehört zu mir“

Klar, man ist stolz auf das, was man da gemacht hat. Da steckt schließlich Gehirnschmalz, Schweiß und (hoffentlich nicht) Blut drin. 😉 Und (hoffentlich) hat man, was man getan hat, ja auch aus einer Intention heraus gemacht. Und wenn dann einfach einer daherkommt und an allem rummeckert, kann man sich schon mal angegriffen fühlen. Hoffentlich meckert der Reviewer ja aber nicht an allem rum. 😉 Aber zu den Bewerter-Anti-Pattern kommen wir ja gleich. Wichtig ist im Hinterkopf zu behalten, dass der Reviewer nicht die Person bewertet, sondern den Code.

Der ist nicht nur dein Produkt, sondern auch dass deines Wissensstandes und der Umstände. Vielleicht hat man schlichtweg nicht alles gewusst, was man hier anwenden könnte? Niemand kann und weiß alles – gerade in einem Gebiet wie der Softwareentwicklung, die so überbordend von Technologien, Programmiersprachen und Prinzipien ist. Das ist ja der Grund, warum man die Review macht. Stattdessen ist sie, wenn man es zulässt, eine Chance sich zu verbessern. Würde man nicht auf das eine oder andere aufmerksam gemacht werden, wäre man letzten Endes vielleicht nicht mehr stolz auf „sein Baby“, sobald man auf anderem Wege mehr Erfahrung gewonnen hat.

Anti-Pattern bei Bewertern

Der Nit-Picker

Was würde ein Nit-Picker sagen? Vielleicht sowas wie „Eigentlich gefällt mir nichts.“ XD Oder sowas denken wie „Irgendwas muss ich noch finden.“ Tatsächlich habe ich mal in einem Projekt gearbeitet, wo es hieß, dass man etwas finden muss. Immer. In demselben Projekt habe ich mal von einem Kollegen mit sehr sehr viel Berufserfahrung eine Review zurückbekommen, in der ich darauf hingewiesen wurde doch mal den unnötigen Whitespace zu entfernen. Also der, der nicht mal auffällt, weil dort meinetwegen nach einer geschlossenen geschweiften Klammer noch ein Leerzeichen stand. Kein Scheiß. Ich glaube es gab nicht mal andere Befunde. Dann diskutierst du entweder oder du sitzt da und entfernst Leerzeichen. Inzwischen würde sowas dank Save Actions und Code Formatting eh nicht mehr auftreten. Das, was damals angemerkt wurde, war für mich damals Nit-Picking und ist es heute immer noch. Generell kommt es aber v.A. sehr stark darauf an in was für einem Projekt man ist und wie die Code-Qualitäts-Standards sind.

Ist es klar, dass das mit diesen schon nichts mehr zutun hat, dann versuchen Reviewer hier vielleicht persönlicher Vorlieben durchzusetzen und landen bei Nit-Picking. Wenn sich das häuft, sollten wohl eher die Standards neu verhandelt werden … . In den meisten Fällen ist es ja wahrscheinlich sogar gut gemeint. Man hat ja meist eine Intention oder Vorstellung, warum das jetzt besser ist. Tipp: eine Reviewanmerkung immer begründen, damit andere die Intention nachvollziehen können. Gibt es keine Begründung außer „weil ich das so lieber mag“ oder „weil ich nichts anderes gefunden habe“, dann merkt man als Reviewer wohl schon selber, dass man die Anmerkung auch getrost weglassen kann.

Der Narzisst

Eigentlich haben wir ja schon mal da oben festgestellt, dass niemand alles wissen kann. Aber trotzdem sind die eigenen Fähigkeiten manchmal doch Bestandteil von Angeberei. Manchmal denke ich, dass das unter den Informatikern besonders verbreitet ist, aber dieses „Look at me and my skills“ kommt wahrscheinlich in jeder Branche vor. Zwischen Angeberei und Beleidigungen gibt es für mich tatsächlich wenig Unterschied. Immer, wenn sich einer über den anderen stellt, dann zeugt das nicht von Teamgeist. Und man arbeitet eigentlich nie als Individuum, sondern liefert als Team. Deswegen bitte freundlich bleiben. Dazu gehört auch, dass man den anderen und seine oder ihre Arbeit anerkennt. Fairplay ist wichtig für das Zusammenspiel aller. Für mich ist ein Narzisst oder jemand der andere angreift eine größere Krücke als so ziemlich alle anderen der Muster hier. Glücklicherweise hatte ich in meinem bisherigen Berufsleben mit verschwindend gering wenigen Narzissten zutun. Aber die, die es gab, haben einen bleibend unangenehmen Eindruck hinterlassen.

Der Samthandschuh

Wenn man zu hart oder angeberisch ist, mag das das Eine sein. Auch nicht gut ist aber alles und jeden nur mit dem sprichwörtlichen Samthandschuh anzufassen. Wenn man Angst hat den anderen zu verärgern und sich deswegen Kommentare im täglichen Geschäft oder in der Review verkneift, dann geht eventuell etwas wichtiges und hörenswertes unter. Angst zu haben würde auch bedeuten, dass irgendwas im Team nicht rund läuft. Das geht übrigens auch andersrum: wenn man beispielsweise Kolleg*innen etwas nicht anmerkt, weil die neu sind, wenig Erfahrung haben, man sie nicht einschüchtern will … wem bringt das was? Der Code ist dann suboptimal und der Kollege oder die Kollegin lernt nichts dazu. Hinterher bedanken die sich bestimmt nicht dafür bei einem, wenn sie dann wochenlang etwas falsch machen und es dann heißt „Du bist doch aber schon so lange dabei“. Stattdessen macht der Ton die Musik. 🙂 Wenn man das vernünftig erklärt, dann gibt es nichts, weswegen man Bedenken haben sollte, oder?

Der Schrödingers Reviewer

Irgendwie gibt es einen Reviewer – aber irgendwie auch nicht! ^^ Bedeutet: man hat eine Code Review bzw einen Merge Request. Aber entweder guckt es ewig niemand an oder man hat schon jemand als Reviewer zugewiesen, aber deren Reaktion lässt auf sich warten. Was sind hier wohl die häufigsten Gründe? Zuviel zutun? Da ist ein gutes Maß: Reviews gehen vor, bevor neue Aufgaben angefangen werden. Ist ja nur vernünftig, damit bei niemandem Stau entsteht. Aufschiebeverhalten und Motivationsmangel müssen diskutiert werden. Was ist die Ursache dafür? Vielleicht findet man als Team Abhilfe. Der Schrödingers Reviewer ist aber auch einer, der partout keine Anmerkungen macht und die Reviews durchwinkt. Da bekommt man so ein ungutes Gefühl, als ob man keine Review bekommen hätte. In dem Fall lohnt es sich mal zu fragen warum. Vielleicht hat man etwas gemacht, was zuviel magic war – gerade dann lohnt es sich darüber zu reden. Vielleicht kommt auch raus, dass der andere nicht weiß wie man eine Review macht – das führte bei uns zu oben besagtem Vortrag. Reden hilft.

Der Spezi

„Ich kann Frontend nicht so gut, also mache ich keine Review bei Frontend.“ – Und lerne nie etwas dazu. Schade eigentlich! Eine Review kann eine Chance sein, sich auf schonende Art und Weise in etwas einzuarbeiten. Man muss da schließlich nicht gleich den ganzen Ballast anschauen, sondern sieht dankenswerterweise nur einen Ausschnitt. Klar, jeder hat so seine Vorlieben. Aber typischerweise macht doch heute in Teams eh jeder alles. Und spätestens wenn mal der Frontend-Pro im Urlaub ist, kann es ja sein, dass man ran muss. Na dann hätte ich bis dahin doch lieber schon mal etwas davon gesehen. 😉

Der Monologer

Inzwischen habe ich die Benachrichtigungen über Anmerkungen in der Review ausgeschalten. Weil ich häufig zwanzig Mails bekomme und es nur ein oder zwei Anmerkungen gibt. Warum? Weil meine Kollegen so gerne mit mir reden. 🙂 Oder mit sich selber. Der Monologer ist jemand, der eine Anmerkung schreibt, aber selber noch nicht ganz darüber nachgedacht hat. Dann fällt ihr oder ihm noch was ein. Und ach ja, das war ja so und so!? Wenn dann jeder Gedankenblitz aufgeschrieben wird und am Ende rauskommt, dass die Anmerkung falsch oder überflüssig war, dann bekommt man als der Coder hinterher einen schönen Roman zum Lesen zurück, ob es eigentlich wenig zutun gibt und das Nacharbeiten der Review-Befunde schnell gemacht wäre. Wenn man nicht soviel lesen müsste … . Drum prüfe, was du anmerkst, um allen Arbeit zu ersparen. Aber witzig ist das manchmal schon … XD

Der Beitrag ist übrigens nicht als Auskotzen oder Jammerbeitrag zu verstehen. Ich sitze regelmäßig an dem Einen wie an dem anderen Ende und habe auch leider schon das eine oder das andere Anti Pattern kennengelernt oder produziert. Und die sorgen doch immerhin für interessante Anekdoten aus dem Arbeitsleben, Running Gags und solche netten Pattern wie da oben, die hoffentlich zum schmunzeln bringen oder zeigen wie man es nicht macht, haha.

Da Code-Reviews von Personen für Personen gemacht werden, halte ich mich hier an Neigungen von Menschen auf. Natürlich gibt es auch Prozess-Anti-Pattern. Die äußern sich beispielsweise darin, dass nach fertiger Arbeit die ganze Architektur umgewälzt wird oder auffällt, dass die Arbeit überflüssig war. Autsch. Aber heute geht es eher um das Menscheln. Welche Anti-Pattern kennt ihr noch? Und bei welchem Pattern müsst ihr euch leider zu oft schuldig bekennen? Ich bin leider oft „Mein Baby gehört zu mir“ und eine Zeit lang war ich auch ein „Nit-Picker“ wegen meiner persönlichen Vorlieben wie Code idealerweise aussieht. Aber ich bessere mich … denke ich. 🙂

Netzgeflüster ist eine Kategorie meines Blogs in der ich mich immer zwischen dem 10. und 15. eines jedes Monats Themen aus IT, Forschung, Netzwelt und Internet widme genauso wie Spaß rund um die Arbeit mit Bits und Bytes. 🙂