Review & Team Happiness

Vorsicht Aufzeichnung!

Warum sollte mich das interessieren?

Effektiver lernen

Fokus auf Themen an denen man wachsen kann
Es geht nicht um Stylingänderungen

Lernen durch Annehmen von konstruktiver Kritik

Das Team wird besser

Review als:

Abgleich und Lern-Chance zwischen dem Team
Gelegenheit für alle anderen im Team, die sich da dran ein Beispiel nehmen

Weniger Reibung

Codereviews sind Reibung
Sich bewusst mit ihnen zu beschäftigen minimiert diese

Wertschätzung

Euer Reviewer

hat nicht unendlich Zeit
ist nicht euer Qualitätssicherungs-Experte
ist kein Hindernis, das überwunden werden muss

Methoden

Selber Reviewen

Bevor MR versendet wird
Stell dir vor, du kennst den Code noch nicht
Benutze dasselbe Environment wie der Reviewer

MR Beschreibung

Klare Einfuehrung in den Context
Screenshots? Videos?
Ticket verlinken
Hohe Flugebene

Automation

Style
Lint
Compile
Tests
Nie vorher schon zum Review abgeben

Verständnisfragen

In der Regel ein Zeichen für suboptimalen Code
Code verständlicher neu schreiben
Bei Problemen in Zukunft wird der Code und das Gitlog referenziert
Keine 1:1 Kommunikation

Do one thing

Ein MR sollte sich um ein Thema kümmern
Kleine und einfache Änderungen
Kein "Ich mach noch mal eben das mit"

Refactoring

vs

Behavior change

Nicht vermischen
Sonst hart zu reviewn

Beispiel Commitaufteilung:

Tests für vorhandenen Code hinzufügen
Refactoring
Änderung des Verhaltens und Anpassung der Tests

Refactoring

vs

Behavior change

"Zeig mir einen Merge Request mit 1000 Zeilen und ich finde 1 Fehler"

"Zeig mir einen Merge Request mit 10 Zeilen und ich finde 1 Fehler"

Große Changes aufteilen

Ab circa 400 Zeilen
Versuchen logische Pakete zu finden

-> Besseres Feedback & glücklicher Reviewer

Etiquette

Be nice - benutzt Emojis
"Welche Änderungen wären hilfreich?"

Kritik

Probleme finden ist gut
Reviewer und MR Sender sind im selben Team

Nachsicht mit Reviewer

Können sich genauso irren
Können Code falsch verstehen
Sind nur Menschen
Ist der Grund für das Missverständnis im Code?

MR Zustand

Team muss Konventionen haben:

Es sollte immer klar sein, wer einen MR bearbeitet
Wann Comments resolven?
- Kommentar mit "Done"
- Oder Kommentar warum Änderung nicht gut wäre
- Resolve durch Reviewer

How to git commit

Betreff vom Body trennen

Betreff mit bis zu 72 Zeichen, besser 50
Betreff immer Capitalized
Betreff nicht mit Punkt
Nicht jeder Commit braucht nen Body

Derezz the master control program

MCP turned out to be evil and had become intent on world domination.
This commit throws Tron's disc into MCP (causing its deresolution)
and turns it back into a chess game.

Imperativ

Refactor subsystem X for readability
Update getting started documentation
Remove deprecated methods
Release version 1.0.0

Imperativ

Hilfestellung:

If applied, this commit will your subject line here

Body

Maximal 72 Zeichen pro Zeile
Erkläre Was und Warum nicht Wie

commit eb0b56b19017ab5c16c745e6da39c53126924ed6
Author: Pieter Wuille <pieter.wuille@gmail.com>
Date:   Fri Aug 1 22:57:55 2014 +0200

   Simplify serialize.h's exception handling

   Remove the 'state' and 'exceptmask' from serialize.h's stream
   implementations, as well as related methods.

   As exceptmask always included 'failbit', and setstate was always
   called with bits = failbit, all it did was immediately raise an
   exception. Get rid of those variables, and replace the setstate
   with direct exception throwing (which also removes some dead
   code).

   As a result, good() is never reached after a failure (there are
   only 2 calls, one of which is in tests), and can just be replaced
   by !eof().

   fail(), clear(n) and exceptions() are just never called. Delete
   them.

Tips

GIT via Commandline
GIT rebase verinnerlichen
Vim
Jeder (zwischen) Commit builded

Quellen

https://mtlynch.io/code-review-love/
https://chris.beams.io/posts/git-commit/