Fråga till er som jobbar på företag med code-reviews

Intressant ämne. Jag jobbar sedan ca 18 månader för första gången på ett sådant ställe. Vi är tre personer med kompetens på koden - en skriver kod, en granskar. Riktlinjer utöver att någon ska granska saknas så vitt jag vet.

Jag brukar insistera på att commit-meddelandet ska innehålla hur ändringen har testats. Även syftet med ändringen och motivering varför det gjorts på just det viset bör framgå. Oavsett om det är en rookie eller en erfaren utvecklare så vill man i efterhand veta hur tankegångarna gick när koden skrevs och vilka hänsyn som måste tas vid nästa ändring.

När koden interagerar mot ett annat system, vars beteende/implementationsval inte är lätt att läsa från kod och dokumentation så brukar jag försöka få det kommenterat i koden.

Ovanstående ger automatiskt att ändringar blir lite mer genomtänkta och testade på ett genomtänkt vis. Det sistnämnda upplever jag är speciellt viktigt med folk som inte jobbat så länge. Detta är i en organisation där testare förväntas ta vid och göra blackbox-testning efter att PR granskats och committats. Enhetstester finns men kan bara täcka en oviktig bråkdel av funktionaliteten.

Jag brukar få mina PR genomsläppta rätt kvickt, men det händer ju att det finns problem kvar i dem som inte upptäckts vid granskning. De flesta regressioner vi får är i kollegors kod, där jag har granskat och där det är något specialfall vi båda har missat. Det händer att jag är rätt jobbig, men jag brukar fråga om mina klagomål är motiverade eller för petiga och med en öppen dialog så brukar det gå bra.

Lite av målet är ju att alla ska förstå all kod. Så om någon bara förstår 20% så har man ju identifierat ett problem. Varför är bara 20% möjlig att förstå? Ska inte varje klass/metod ha kommentarer som motiverar dess existens och vilka numrerade krav/användningsfall de implementerar?

Som andra säger så har ju code reviews har ju fler syften än bara att hitta buggar:

• Lärandesyfte i hur man kan skriva bra (eller dålig) kod

• Kunskapsdelning om hur man löste ett visst problem och vad man jobbat med, så att reviewern sen kan använda sig av koden för sina features, eller hjälpa till att lösa buggar om någon sån slinker igenom

• En extra check att man skrivit kod som faktiskt löser problemet, så man inte misstolkat något

• Se till att man inte råkat committa något som inte skulle vara med (typ en api nyckel eller extra debug printar)

Samtliga delar ovan kan man delta i oavsett sin kunskapsnivå eller erfarenhet med projektet, men på sin egen nivå.

Att hitta komplexa logikfel eller buggar i koden kanske jag inte förväntar mig att en junior ska göra, men jag värdesätter att man har ett par extra ögon på koden som kan ställa frågor om något ser oklart eller fel ut. Sen är det upp till automatiska tester och manuella regressionstester att identifiera om nån bugg trots allt slunkit igenom.

Tycker det här är skitviktigt att trycka på - En Pull Requests främsta syfte är inte att någon ska agera kod-polis, utan att man ska dela och sprida kunskap, antingen om hantverket i sig, eller om domänen/problemet man jobbar med. Om man sitter och kommenterar på namngivning, antal mellanslag kontra tabbar etc, är man helt fel ute.

Om man hela tiden har fokus på learning/teaching brukar det bli både värdefullt och trevligt, sitter man med rödpenna och påpekar lint-regler blir det sunkigt.

Man ska fostra en atmosfär där alla VILL bli code-reviewad, man kan ju ha missat något liksom.

Sen alla ceremonier om hur commit meddelanden ska skrivas, vad det ska finnas för beskrivningar, bilder, test-instruktioner etc i en PR är helt sekundärt i slutändan. Det som är viktigt är ju att koden funkar och att alla kan förstå den, inte bara idag, utan om ett år.