7 tips för bättre code reviews
En fråga dök upp på vår Slack om hur man gör bra code reviews på pull-requests. Frågan drog igång en diskussion med många tankar och förslag. Det är helt klart en viktig fråga som berör de flesta av oss, och något som flera grubblar på. Ett perfekt tillfälle att dela med oss av våra erfarenheter.
Det här inlägget kommer att täcka hur man blir en bättre reviwer. Vi kommer inte att täcka hur man skriver bättre pull-requests — se upp för det inlägget i framtiden. Detta inlägg skrevs gemensamt av Sarah Serussi, Christian Brevik, Anders Njøs Slinde, Magnus Dahl, and Mikael Brevik.
Låt oss gå vidare till tipsen!
1. Var snäll
Försök hålla en vänlig ton och en informell skrivstil. Utvecklaren har troligen lagt mycket tid och energi på att skriva koden. Kommer det därför pull-request med torftig info eller avhuggna meningar kan vem som helst tappa gnistan. Personens efterföljande “investering” i arbetet kommer att vara högre om du gör detta snyggt, och när de tittar på mållinjen är de förmodligen otåliga om att få sin kod mergad och i användarnas händer så snabbt som möjligt.
Du som reviwer har makten i denna situation, använd den försiktigt och sparsamt. Var snäll! Blir det utdragna diskussioner som inte leder någonstans, flytta dem till andra plattformar som Slack, med målet att få pull-request mergad istället för att dra ut processen.
2. Ta dig tid att förstå det underliggande målet för pull-requesten
Att förstå avsikten med en förändring är viktigare än att läsa den rad för rad. När du bara läser koden utan sammanhang får du förmodligen en ytlig bild av vad koden är tänkt att göra. Du kan fånga “triviala” fel som saknade semikolon, namngivningsproblem, stilistiska ändringar etc. Men det är mycket svårare att ta upp grundläggande frågor med själva lösningen och de problem den försöker lösa.
För att göra detta måste vi förstå avsikten med koden. Vad är det tänkt att ändra eller lägga till? Vad föranledde behovet av pull-requesten? Det betyder att reviwern behöver göra mer än att bara läsa koden.
Ta dig tid att grundligt förstå sammanhanget och konsekvenserna. Läs beskrivningen av pull-requesten noggrant, prova ändringarna själv (se tips #5) och ställ frågor (se tips #6). På så sätt kan vi kommunicera på en högre nivå för att föreslå alternativa lösningar, hitta logiska brister eller fånga missförstånd. Det kommer också att hjälpa oss som team att vara mer användarcentrerade. I stället för att bara kommentera koden kan vi kommentera lösningar och funktionalitet.
3. Ge förslag till förbättringar — visa lösningar snarare än problem
Ofta när du arbetar med en buggfix eller en ny funktion kan reaktionen bli att man vill skicka en pull-request på en gång. Andra gånger kanske du arbetar med en pull-request i flera dagar. Hur som helst är det väldigt lätt att förbise stavfel, tillfälliga loggsatser eller dålig namngivning av variabler när du arbetar med koden, det är därför granskningsprocessen är så viktig att ha.
När du granskar kod kan det vara lätt att titta på den med ett kritiskt öga och känna att målet är att hitta så många fel som möjligt. Även om det är okej att ha den här avsikten är det viktigt att kommunicera felen på ett sätt som är meningsfullt för avsändaren av pull-requesten. Det är också lätt att bli blind inför sin egen kod, så kom ihåg det när du upptäcker sådana misstag.
Många plattformar som GitHub eller Azure DevOps har en funktion som gör att du kan skicka förslag i pull-requesten. Det innebär att reviwern kan ge förslag på kodändringar för mindre misstag eller stavfel, i stället för att bara påpeka problem och lämna det upp till avsändaren att ta reda på det.
Till exempel är det verkligen svårt att namnge saker, så när du (en tidsfråga) hittar ett dåligt namnval, var konstruktiv och ge ett alternativ i stället för att kritisera utan att hjälpa situationen. Förslagsfunktionen är guld värd i dessa fall.
Om du hittar större fel eller kodblock som kan ersättas av något helt annat, bör du förklara för avsändaren vad du tänker och ge en potentiell lösning. Detta kan göras personligen, på Slack eller i kommentarsektionen i pull-requesten. Om en konflikt uppstår, involvera andra eller acceptera dina förluster.
4. Undvik stilistiska, alltför petiga kommentarer
Människor har olika åsikter om hur de föredrar att deras kod ska se ut. Att diskutera det i kommentarsavsnittet i en pull-request är kanske inte är rätt plats. Sådana saker hör inte hemma i en pull-request.
I början av ett projekt bör du och dina medutvecklare ställa in reglerna för vilken typ av automatisering du vill inkludera i ditt projekt, och någon form av formateringsverktyg är alltid bra att ha. Det gör att du och de människor du arbetar med kan ha en gemensam uppsättning regler — utan att behöva tänka på det senare. Det spelar egentligen ingen roll vad reglerna är, så länge ni är konsekventa.
Släpp skärmen för en sekund och tänk efter: varför tittar du på den här koden? Det ska handla om att få fler ögon på en uppgift, men det handlar också om att lära sig av varandra. Smakfrågor är egentligen inte så relevanta i denna diskussion.
5. Var inte rädd för att checka ut lokalt
Det fantastiska med kodgranskningar genom pull-requests är att det ändrar kontexten för hur du tittar på koden. Det tar dig ur den välbekanta inställningen som din IDE tillhandahåller och tvingar dig att se ändringarna i ett annat ljus. Det skiftet kan hjälpa dig att märka misstag som du annars inte skulle göra. Detta gäller både för den person som öppnade pull-requesten och granskarna.
Men ibland finns det svårigheter med att granska koden i ett webbaserat gränssnitt. Själva ändringsuppsättningen kan vara för stor. I det här fallet kan det vara svårt att navigera i ändringarna och förstå vad effekten är. Andra gånger kan logiken vara för komplex för att bara läsa. Man fastnar efter många försök att läsa rad för rad och “kompilerar” den i ditt huvud för att förstå vad som händer.
I dessa fall är det bästa sättet att bara checka ut ändringarna lokalt. På så sätt kan du använda din egen editor/IDE att navigera i stora ändringsuppsättningar och köra koden för att förstå påverkan eller logisk komplexitet. Med andra ord, kompilera och kör koden på din maskin, inte i ditt huvud.
Detta kan kännas som ett hinder för att vara produktiv som utvecklare eller team. Du vill inte vara en vägspärr för att landa förändringar. Men ett värre scenario är att hitta ett fel mycket senare, vilket kunde ha fångats i en kodgranskning. Ju längre tid det tar att hitta ett fel, från det att koden skrevs, desto svårare kan du ha att försöka fixa det felet.
6. Ställ (öppna) frågor
Man kan titta på en pull-request och tänka på den som en process för att kontrollera kodens robusthet. Och till en viss grad är det faktiskt så. Men det är inte allt. Det är också en handling av berättande för hur din kod kom till i det tillstånd som den är i.
Koden i sig kanske inte berättar hela historien när andra försöker förstå den. En del av det är kommentarer i koden, men det kan bara relatera till att förstå de mindre detaljerna i vissa filer eller vissa kodrader. En bredare bild av varför koden är som den är kan inte målas genom att bara läsa koden, kommentera eller databashistoriken.
Pull-requests och issues, och diskussionen som omger dem fyller ett viktigt utrymme när man försöker förstå orsaken till beslut som fattas när man utvecklar lösningar. Det betyder att det är extra viktigt att ha en faktisk dialog i processen när den utvecklas.
För dig som reviwer betyder det att om du inte förstår något ska du aldrig vara rädd för att ställa frågor. Det är mycket troligt att någon senare kommer att grubbla på precis samma sak som du gör. Det betyder att du kommer att hjälpa till att skriva historien om Varför Något kom till, inte bara det som kom till. Och samtidigt kan personen som öppnade pull-requesten reflektera över hur de har löst ett visst problem, och välja att göra det annorlunda om det behövs.
Det är viktigt att notera att även om det är bra att ställa frågor, försök att rama in dem på ett sådant sätt att en produktiv diskussion kan uppstå. Öppna frågor är ett bra verktyg att använda här — försök inte flytta diskussionen i en viss riktning som bara gynnar dig eller sätter människor på plats. Återigen, var snäll (se tips #1)! Öppna frågor ger dig mycket mer information än att bara bekräfta din egen förutfattade mening.
I stället för att ställa en sluten fråga såsom “Den här koden är rörig. Är detta verkligen ett bra sätt att hantera detta specialfall?”, ge den andra personen utrymme att berätta sin historia genom att fråga “Jag förstår inte riktigt den här koden. Kan du förklara hur det här löser specialfallet?
7. Optimera för output, inte för gatekeeping
Alla får det bättre om pull-requesten mergas så snart som möjligt. En långsam kodgranskningsprocess är mycket demotiverande för både reviwer och inlämnare.
Om något inte omfattas delar du upp det i en ny pull-request, eller tar diskussionen någon annanstans om det fortfarande kan mergas. Många detaljer kan åtgärdas senare om koden fungerar som helhet och inte bryter med den övergripande arkitekturen. Fokusera på de stora sakerna, inte de små problemen. Leta efter uppenbara buggar och logiska fel, samtidigt som du är mer förlåtande för de mindre problemen.
Processen bör vara en värdefull lärdom och inte användas som ett verktyg för gatekeeping. Kom ihåg att ni är i detta tillsammans som ett lag, och målet bör vara att hålla bollen rullande. Både för att hålla uppe motivationen i teamet, men också för att kontinuerligt leverera värde.
Ta reda på vad som fungerar för dig och ditt team. Skriv ner dina riktlinjer för hur du vill göra kodgranskningar och hitta en gemensam konsensus. Ha åtminstone konversationer om vad du tycker att bra kodrecensioner är.
Det är våra 7 tips för hur du gör bättre code reviews i ditt team. Kan du komma på något annat som har fungerat bra för dig eller om det finns något vi har missat?
Skriven av:
Sarah Serussi, Christian Brevik, Anders Njøs Slinde, Magnus Dahl, and Mikael Brevik.
