BLOG

Software ontwikkeling

Hoe kun je als reviewer je code reviews verbeteren met Google?

11 mei 2020

Elk zelfrespecterend software development team heeft of wil een code review proces om de kwaliteit van de geleverde software te waarborgen. Bij code reviews worden wijzigingen in de code nagekeken en gecontroleerd op interne standaarden door een (collega) ontwikkelaar. De inhoud van deze reviews kunnen voor elk bedrijf of zelfs per team verschillen, ze komen voor in alle vormen en maten die je maar kunt bedenken. 

Er kan bijvoorbeeld alleen gekeken worden naar stijl of leesbaarheid van de nieuwe code, maar er kan ook diepgaand gekeken worden naar:

  • ‘vervuiling’; is er code die niet bijdraagt aan de gekozen oplossing
  • statische en dynamische software-kwaliteiten
  • of de nieuwe code bijdraagt aan het architectuur
  • en nog veel meer

Welke aanpak en hoe diepgaand die is, weerspiegelt niet hoe ‘goed’ je technische onderlegging is als team. Er zal altijd een afweging plaats moeten vinden om te kijken hoeveel het werkelijk toevoegt binnen een project, bijvoorbeeld om te controleren op statische software-kwaliteiten als het ‘maar’ gaat om een simpele website zonder complexe logica.

Wat ontzettend belangrijk is, is dat duidelijk gecommuniceerd wordt waar naar gekeken wordt binnen het team. Programmeurs weten dan van elkaar wat ze kunnen verwachten en aan welke eisen voldaan moet worden. Wij hebben dit bijvoorbeeld (beknopt) verwerkt in onze Definition of Done. Daarmee proberen we de mindset te creëren dat werk ook werkelijk pas ‘af’ is als het voldoet aan onze softwarekwaliteit standaarden. 

Wie hier héél erg goed mee om is gegaan, is Google. Google heeft best practices openbaar gemaakt over hoe zij omgaan met code reviews en bekijken het vanuit twee perspectieven: ze hebben een best practices lijst voor de programmeur die code inlevert om deze na te laten kijken (de auteur), maar ook een voor degene die die de code nakijkt (de reviewer). Ze beschrijven hierin uitgebreid hoe zij de code review inhoudelijk uitvoeren. Google maakt gebruik van een Pull Request (PR) waarbij code alleen wordt toegevoegd aan hun product als hij met een positief resultaat door de code review komt. Hieronder zullen we per onderdeel kort even stilstaan bij de punten die Google opnoemt en wat ze daarmee willen bereiken.

Guidelines voor de reviewers 

Basisregels

Het review proces is een proces dat lastig te balanceren is. Vanuit het perspectief van een auteur worden er verbeteringen geïntroduceerd in een PR, maar vanuit het perspectief van de PR reviewer zitten er mogelijk alsnog fouten in die mogelijk verbeterd moeten worden. Dit kan zorgen voor spanningen. Als de reviewer hier te streng in is en alleen perfectie toestaat, ontstaat er een onbalans waarin auteurs geen verbeteringen meer willen toepassen omdat hun PR’s toch nooit goed gekeurd worden. Echter als de reviewer alles toestaat verstoort dat ook weer de balans, omdat de kwaliteit niet voldoende gewaarborgd wordt. Zonder code review is software gedoemd om in technische staat achteruit te gaan (technical debt). Dit wordt nog erger wanneer de tijdsdruk hoog is en er weinig tijd overblijft voor (verbeteringen in) kwaliteit. Balans vinden tussen: snelheid behouden in doorontwikkeling en kwaliteit waarborgen en verhogen, is iets waar altijd rekening mee gehouden moet worden. Beide partijen (de auteur en de reviewer) moeten samenwerken om deze balans te behouden. 

Om deze balans te behouden moet soms:

  • de auteur toegeven, door toch wat feedback van de reviewer toe te passen, 
  • de reviewer de wijzigingen toestaan ook al zijn ze niet perfect.

Perfecte code bestaat namelijk niet. Een PR die onderhoudbaarheid, leesbaarheid en complexiteit verbeterd zou niet dagenlang afgekeurd mogen blijven staan door enkele imperfecties. Een verbetering tegenover de huidige situatie is altijd goed. Mocht je als reviewer toch je feedback willen toevoegen, maar weet je van jezelf dat het niet opgepakt ‘hoeft’ te worden, dan is de afspraak bij Google om het aan te geven met ‘nit: <jouw commentaar>’ zoals hieronder weergegeven.

Door dit met het gehele team af te spreken dat deze ‘nits’ niet per se opgelost hoeven te worden, maar er wel naar gekeken mag worden, haal je de lading van deze imperfecties af en kun je concessies doen. Zo kan de auteur een aantal ‘nits’ uit de commentaar lijst halen en degene oplossen die er volgens hem of haar werkelijk toe doen. Dit kan ook toegepast worden voor wat meer educatieve stukken commentaar die niet perse verplicht zijn voor het goedkeuren van de PR.

Google schrijft ook enkele principes voor die gelden als gouden regels binnen een PR:

  • Technische feiten en data uit onderzoeken gaan vóór persoonlijke voorkeuren
  • Programmeerstijl is vaak een persoonlijke voorkeur. Als de code voldoet aan de standaard afgesproken programmeerstijlen is het goed genoeg.
  • Aspecten van software ontwerp (architectuur) zijn nooit alleen afgeleid van een persoonlijke voorkeur of stijlvoorkeur
  • Als bovenstaande regels niet toepasbaar zijn gaat het om consistentie. Hebben we dit al vaker uitgevoerd? Zo ja, op dezelfde manier programmeren indien dit de codebase niet met meer technical debt achterlaat.

Verder geldt het ook voor de reviewer dat als er een conflict of discussie ontstaat waar ‘geen uitweg’ in zichtbaar is, er met alle betrokken partijen samen wordt gezeten om verder te kunnen. 

Wij hebben bij Senet deze principes één-op-één overgenomen, en zien dat deze sterk geholpen hebben in de effectiviteit van onze interne code reviews.

Wat moet je zoeken?

Een code review gaat natuurlijk om het vinden van problemen of fouten in de nieuwe code. Maar waar moet je dan op zoeken? En waarom zijn dan precies die dingen belangrijk? Google beschrijft verschillende zaken waar naar gekeken moet worden, die (aangevuld met onze eigen ervaring) hieronder beschreven staan.

Design

Allereerst controleer je het ontwerp en de architectuur over het algemeen gevolgd wordt. Behoren de wijzigingen, die zijn gemaakt, werkelijk in de huidige repository? Of zou het in een library terecht moeten komen? En, is het correct om alle toegevoegde functionaliteiten te gebruiken die zijn toegevoegd op dit moment?

Functionaliteit en context

Controleer goed of de code de wijzigingen brengt die de auteur bedoelde te brengen. Stel vragen als:

  • Doet de code ook echt wat hij moet doen? 
  • Geef het een  positief effect voor de eindgebruikers? 
  • Is de wijziging ook positief voor collega-programmeurs? Zij moeten de code ook gaan gebruiken en onderhouden.

Verder verwachten we dat programmeurs hun code goed testen, technisch en functioneel. Echter als reviewer zijn er ook momenten om dit te controleren. Is gewijzigde functionaliteit bijvoorbeeld  begrijpelijk voor een eindgebruiker? Het is belangrijk om ook deze ‘pet’ even op te zetten en te controleren of alles vanuit dat perspectief ook logisch is.

Wat nóg belangrijker is, is dat samen met de auteur gebrainstormt moet worden over de code wanneer er meerdere threads die parallel van elkaar moeten draaien, geprogrammeerd zijn. Dergelijke code is dermate meer complex om te debuggen in de toekomst, dat het van veel waarde is om om nu alvast alle mogelijke complexiteiten die daar bij komen kijken eruit te werken. Denk daarbij aan ‘deadlocks’ en ‘concurrency problems’. Het slimste is echter om dergelijke constructies te vermijden indien het niet essentieel is. Minder complexe code die hetzelfde uitvoert is vaak wenselijk.

Complexiteit

Het is al vaker benoemt. Vermijd complexiteit zoveel mogelijk! Controleer per regel code, per functie en per class of deze door de wijziging complexer is dan hij zou moeten zijn. Code die complex is, is lastig te begrijpen voor je collega’s en voor je toekomstige zelf. Nu zit de auteur in de materie, maar als er een bug wordt gevonden over een maand of twee is de kans zeer aanwezig dat de programmeur zijn eigen code niet meer begrijpt. 

Het is nou eenmaal vele malen makkelijker om bugs te introduceren in complexe code. Begrijp ook dat complexiteit een relatieve term is. Als je een team hebt met meer juniors dan seniors, dan ligt je complexiteit-drempel lager dan wanneer je team vooral uit seniors bestaat.

Wees ook huiverig voor ‘over-engineering’. Het generiek maken van code, waarbij het generieke gedeelte niks toevoegt aan de toegebrachte functionaliteit. Code mooier maken is goed, maar er zijn grenzen. Wees hier als reviewer alert voor.

Tests

Indien ze (hopelijk) in je project worden toegepast, vraag dan in PR’s naar unittests, end-to-end tests en integratietests. Dergelijke tests zouden direct bij de code review bekeken moeten worden, en niet pas nadat de code in productie is genomen en eindgebruikers al blootgesteld zijn aan potentiële bugs. 

Kijk of de tests logisch en nuttig zijn. Stel vragen als:

  • Als er ooit een bug in de code sluipt, zullen de tests dan ook werkelijk falen?
  • Worden de correcte assumpties met  assertions gecontroleerd en worden verschillende scenario’s getest?

Naming strategy

Het beestje een goed naampje geven is een van de kunsten van een goede programmeur. Een van de belangrijkste vragen tijdens het programmeren is: welke naam moet deze module, class, functie, variabel, namespace, service, test, etc krijgen? Controleer of de naamgeving duidelijk en consistent is en wees niet bang om hier ook streng op te zijn. Vind je een naam niet correct voor iets wat je tegenkomt. Benoem dit. Dring aan tot nadenken over goede naamgeving en probeer hier samen met de auteur tot consensus te komen.

Comments

Het beschrijven van complexe code met commentaar lijkt een goed idee maar is juist iets wat je niet moet doen! Commentaar moet altijd beschrijven waarom code bestaat, niet wat het doet. Als je code complex genoeg is om het te gaan beschrijven met behulp van comments kun je je tijd beter besteden in het minder complex maken van je code. Er zijn natuurlijk uitzonderingen, bij complexe regular expressions is een stuk commentaar bijvoorbeeld altijd gewaardeerd, op die manier kan er ook op gecontroleerd worden. Maar een ‘rule of thumb’ is het vermijden van commentaar die code beschrijft. Houd in je achterhoofd dat comments, anders zijn dan documentatie van classes of functies. Met comments wordt bedoeld; de comments in de body van een functie.

Stijl & consistentie

Het hebben van een styleguide is essentieel voor een project. Hierdoor weten programmeurs welke stijl ze moeten aanhouden en weet de reviewer waarop gecontroleerd moet worden. 

Het belangrijkste doel voor een dergelijke styleguide is het vermijden van merge-conflicten en  consistentie/leesbaarheid van code voor iedereen. Als je toch wat stijlpunten wilt benoemen die niet in de styleguide zitten, benoem ze dan wederom met “nit: <jouw commentaar>”. Daarmee geef je aan dat je de code wilt verbeteren, maar het geen verplichting is omdat het niet in de styleguide staat.

Documentatie

Als een change list (CL) wijzigt hoe mede-programmeurs om moeten gaan met builds, tests,  of met je code, dan moet je de documentatie ook naar behoren bijwerken. Indien deze documentatie er nog niet is, zorg er dan voor dat die direct toegevoegd gaat worden. Zodra dergelijke documentatie mist controleer hier dan dus ook op en wees niet bang om een PR af te keuren voor ‘alleen wat missende documentatie’.

Lees elke regel

Lees elke regel goed! Scan niet over classes heen. Lees de code, alsof de code van jezelf is en je zelf een PR inschiet met de code die je bekijkt. In ieder geval lang genoeg zodat je elk stuk van de PR begrijpt en kunt beamen. Zodra de complexiteit beïnvloed hoe snel je de code review kunt afhandelen moet je dit aangeven bij de auteur. 

Geef complimenten

Het proces van code review is vooral opgebouwd op het vinden van fouten en die te verbeteren. Dit komt alleen wel, voor een auteur, altijd over als negatief commentaar. Dit kan tot frustraties leiden. Daarom is het belangrijk om ook positieve feedback te geven. Zie je mooie code? Benoem dit. Dat zal positiviteit binnen het team verhogen en mogelijk zelfs ertoe leiden dat auteurs meer ‘nits’ van jou op zullen pakken.

Navigeren binnen een PR

Alle wijzigingen die een CL met zich meebrengt in kaart brengen kan overweldigend zijn. Zeker als er flinke stukken logica gewijzigd zijn die ook invloed hebben op andere stukken van de applicatie. Waar een programmeur mogelijk uren of dagen mee bezig is geweest, moet jij in een kwartier in je hoofd zien te stampen en daar ook nog technisch onderlegde feedback op geven. Daarom is het slim om te weten hoe je het makkelijkste kunt navigeren binnen de PR om het jezelf makkelijker te maken en betere feedback te kunnen leveren.

Werk van abstract naar details

Begin met het op hoger niveau bekijken van de PR. Neem de beschrijving in je op en bekijk de wijzigingen in het algemeen. Kloppen de wijzigingen als je ze vergelijkt met de beschrijving? Als beide niet met elkaar overeenkomen kun je hier al direct feedback op geven. Vraag dan ook aan de auteur om dit eerst te corrigeren voordat je verder kijkt. Controleer daarna of de gemaakte wijzigingen logisch zijn als je bekijkt wat er gemaakt had moeten worden vanuit de originele vraag.

Ten tweede focus je op de belangrijkste onderdelen en de grootste wijzigingen. Als de PR te groot wordt om nog te bevatten, of de omvang ervan het proces zou vertragen, vraag dan aan de auteur om de PR op te splitsen in meerdere kleinere PR’s. Ook als je direct anti-patterns of technische fouten ziet moet je dit direct aangeven. Ook al heb je nog niet genoeg tijd gehad om de gehele PR te bekijken. Dit moet zo snel mogelijk om de volgende redenen:

  • Zodra een PR is ingeleverd door een auteur, starten ze direct nieuw werk terwijl ze wachten op feedback. Hoe sneller je erbij bent, hoe verser het in ieders hoofd zit waardoor het sneller en makkelijker op te lossen is.
  • Om deadlines te halen is het van belang om afgekeurde PR’s zo snel mogelijk oppakken en te verbeteren.

Tot slot bekijk je de gehele PR in detail. Dit kun je het beste doen in de volgorde waarin de tooling de bestanden aanlevert.

Snelheid van reviewen

Snelheid van afhandelen is ook een aspect wat belangrijk gevonden wordt door Google. Als reviews afhandelen traag gebeurt vertraagd het het gehele development proces. Het is daarbij ook belangrijk dat het gehele team snel kan opleveren en niet maar één individu:

At Google, we optimize for the speed at which a team of developers can produce a product together, as opposed to optimizing for the speed at which an individual developer can write code.”

Hoe lang zou dan een review maximaal mogen duren in doorloop tijd, gemeten vanaf het inleveren van een PR? 1 werkdag is het totale maximum. Hier geldt wederom dat de auteur nog vers in de materie zit als er snel feedback geleverd wordt.

Hoe je het beste commentaar en feedback geeft

Commentaar kan bij een auteur snel negatief overkomen. Doe daarom je best om je commentaar te brengen als positieve feedback:

  • Wees altijd aardig
  • Onderbouw je commentaar technisch en functioneel
  • Geef oplossingsgerichte feedback
  • Moedig auteurs aan om code te versimpelen t.o.v. code comments of je code uit te leggen in de PR

Wees als reviewer ook altijd netjes en accepteer uitleg. Zorg dat je jezelf niet blind staart op het aanhouden van jouw ideeën

Hoe om te gaan met discussies

Discussies zullen zeker voorkomen. Je zult moeten leren om hier goed mee om te gaan. Als auteur, maar ook als reviewer. Blijf altijd respectvol naar elkaar en blijf vooral ook professioneel. Niemand heeft altijd gelijk, soms heeft de auteur gelijk en soms heeft de reviewer gelijk. Als de auteur het niet eens is met je commentaar en er gefrustreerde discussies ontstaan zal er samen gezeten moeten worden om de discussie op te lossen. Soms zul je concessies moeten doen om bijvoorbeeld het commentaar van de PR in een volgende sprint op te lossen. Dan is de auteur namelijk opgelucht dat zijn PR goedgekeurd is en is de reviewer ook tevreden omdat zijn benoemd probleem direct opgepakt gaat worden in de volgende sprint.

Extra Senet tip: PR policies

Bovenop alle tips die Google heeft gegeven, geven wij ook een tip die wij zelf ook toepassen in ons code review proces in het reviewers gedeelte ervan. In onze tooling, Azure DevOps, is het mogelijk om PR policies toe te voegen. Dit zijn condities waar een PR aan moet voldoen voordat een PR door de tooling zelf de PR goedkeurt. 

Zoals hierboven afgebeeld zie je hoe in ons OERknal project, de PR’s alleen goedgekeurd mogen worden als:

  • De CI job’s succesvol zijn uitgevoerd om te verzekeren dat de PR de builds niet kapot gaat maken
  • Er een Work item (ticket) aan gekoppeld moet zijn om ervoor te zorgen dat altijd traceable is waarom bepaalde commits in bepaalde branches terecht zijn gekomen
  • Alle comments op ‘resolved’ gezet moeten worden om stukken commentaar niet te vergeten. ‘nits’ die niet opgepakt gaan worden, zullen ook als resolved gezet moeten worden.

Een code review proces voor jouw team

Ben je benieuwd naar meer details over ons review proces en hoe wij onze reviewers PR’s laten controleren? Of, heb je nog geen review proces en wil je hier beter over geïnformeerd worden? Neem dan vooral contact met ons op zodat we mee kunnen denken over een maatwerk review proces dat naadloos bij jouw team(s) aansluit!

 

Bronnen

  1.  Google’s Engineering Practices documentation, eng-practices maintained by google
  2. How to do a code review, eng-practices maintained by google
  3. The CL author’s guide to getting through code review, eng-practices maintained by google

Interesse in een gesprek?

neem contact op met Lars van der Sangen

Neem contact op

Zie onze privacyverklaring.