Home / Blog / Softwareontwikkeling / Hoe kun je als reviewer je code reviews verbeteren met Google?
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:
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.
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:
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:
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.
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.
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?
Controleer goed of de code de wijzigingen brengt die de auteur bedoelde te brengen. Stel vragen als:
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.
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.
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:
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.
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.
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.
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 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.
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.
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.
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:
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 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.
Commentaar kan bij een auteur snel negatief overkomen. Doe daarom je best om je commentaar te brengen als positieve feedback:
Wees als reviewer ook altijd netjes en accepteer uitleg. Zorg dat je jezelf niet blind staart op het aanhouden van jouw ideeën
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.
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:
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!
We nemen contact met u op!
Senet Eindhoven Gestelsestraat 258 5654 AM Eindhoven Bekijk op kaart
+31(0)40-2930395
KvK nummer: 17115078 Btw nummer: NL807989083B01
