Praktische tips voor code review

Het internet biedt een schat aan materiaal over codebeoordelingen: over het effect van codebeoordelingen op bedrijfscultuur, formele beveiligingsreviews, kortere handleidingen, langere checklists, gehumaniseerde beoordelingen, redenen om code-beoordelingen in de eerste plaats te doen, best practices, meer best praktijken, statistieken over de effectiviteit van codebeoordelingen voor het vangen van bugs en voorbeelden van codebeoordelingen die fout zijn gegaan. Oh, en natuurlijk zijn er ook boeken. Om een ​​lang verhaal kort te maken, deze blogpost presenteert Palantirs visie op codebeoordelingen. Organisaties met een diepe culturele terughoudendheid ten opzichte van peer reviews willen misschien het uitstekende essay van Karl E. Wiegers over Humanizing Peer Reviews raadplegen voordat ze proberen deze gids te volgen.

Dit bericht is gekopieerd uit de best practices-gidsen van onze Java Code Quality-toolketen, Baseline, en behandelt de volgende onderwerpen:

  • Waarom, wat en wanneer code reviews te doen
  • Code voorbereiden voor beoordeling
  • Code-beoordelingen uitvoeren
  • Voorbeelden van codebeoordelingen

Motivatie

We voeren code reviews (CR's) uit om de codekwaliteit te verbeteren en te profiteren van positieve effecten op de team- en bedrijfscultuur. Bijvoorbeeld:

  • Committers worden gemotiveerd door het idee van een set reviewers die het wijzigingsverzoek zullen bekijken: de committer heeft de neiging losse eindjes op te ruimen, TODO's te consolideren en in het algemeen de commit te verbeteren. Erkenning van codeerexpertise door collega's is een bron van trots voor veel programmeurs.
  • Kennis delen helpt ontwikkelingsteams op verschillende manieren:
    - Een CR communiceert expliciet toegevoegde / gewijzigde / verwijderde functionaliteit aan teamleden die vervolgens kunnen voortbouwen op het uitgevoerde werk.
    - De committer kan een techniek of algoritme gebruiken waarvan reviewers kunnen leren. Meer in het algemeen helpen code-beoordelingen de kwaliteitsbalk in de hele organisatie te verhogen.
    - Reviewers kunnen kennis hebben over programmeertechnieken of de codebasis die kunnen helpen bij het verbeteren of consolideren van de verandering; iemand anders kan bijvoorbeeld tegelijkertijd aan een vergelijkbare functie of oplossing werken.
    - Positieve interactie en communicatie versterkt de sociale banden tussen teamleden.
  • Consistentie in een codebasis maakt code gemakkelijker te lezen en te begrijpen, helpt bugs te voorkomen en vergemakkelijkt de samenwerking tussen reguliere en migrerende soorten ontwikkelaars.
  • De leesbaarheid van codefragmenten is moeilijk te beoordelen voor de auteur wiens brein kind het is, en gemakkelijk te beoordelen voor een recensent die niet de volledige context heeft. Leesbare code is herbruikbaar, vrij van bugs en toekomstbestendig.
  • Accidentele fouten (bijv. Typefouten) en structurele fouten (bijv. Dode code, logica of algoritmefouten, problemen met prestaties of architectuur) zijn vaak veel gemakkelijker te herkennen voor kritische reviewers met een extern perspectief. Studies hebben aangetoond dat zelfs korte en informele code-beoordelingen een aanzienlijke invloed hebben op de codekwaliteit en de bugfrequentie.
  • Compliance- en regelgevingsomgevingen vereisen vaak beoordelingen. CR's zijn een geweldige manier om veel voorkomende beveiligingsvallen te vermijden. Als uw functie of omgeving belangrijke beveiligingsvereisten heeft, zal deze profiteren van (en waarschijnlijk vereisen) beoordeling door uw lokale beveiligingscollega's (de gids van OWASP is een goed voorbeeld van het proces).

Wat te beoordelen

Er is geen eeuwig waar antwoord op deze vraag en elk ontwikkelingsteam zou het eens moeten zijn over zijn eigen aanpak. Sommige teams geven er de voorkeur aan om elke samengevoegde wijziging in de hoofdtak te beoordelen, terwijl anderen een drempel voor 'trivialiteit' hebben waaronder een beoordeling niet vereist is. De afweging is tussen effectief gebruik van de tijd van technici (zowel auteurs als reviewers) en het handhaven van codekwaliteit. In bepaalde regelgevingsomgevingen kan code-evaluatie vereist zijn, zelfs voor triviale wijzigingen.

Codebeoordelingen zijn klasseloos: de oudste persoon in het team betekent niet dat uw code niet hoeft te worden beoordeeld. Zelfs als in het zeldzame geval code foutloos is, biedt de beoordeling een mogelijkheid voor mentorschap en samenwerking, en diversifieert, minimaal, het begrip van code in de codebasis.

Wanneer beoordelen

Codecontroles moeten plaatsvinden nadat geautomatiseerde controles (tests, stijl, andere CI) met succes zijn voltooid, maar voordat de code samengaat met de hoofdtak van de repository. Over het algemeen voeren we geen formele code-evaluatie uit van geaggregeerde wijzigingen sinds de laatste release.

Voor complexe wijzigingen die als een enkele eenheid in de hoofdtak moeten worden samengevoegd maar te groot zijn om in een redelijke CR te passen, overweeg dan een gestapeld CR-model: maak een primaire vertakkingsfunctie / grote functie en een aantal secundaire vertakkingen (bijv. Functie / big-feature-api, feature / big-feature-testing, enz.) die elk een subset van de functionaliteit inkapselen en die individueel door een code worden beoordeeld ten opzichte van de feature / big-feature branch. Zodra alle secundaire takken zijn samengevoegd in functie / big-functie, maakt u een CR voor het samenvoegen van de laatste in de hoofdtak.

Code voorbereiden voor beoordeling

Het is de verantwoordelijkheid van de auteur om eenvoudig te beoordelen CR's in te dienen om de tijd en motivatie van reviewers niet te verspillen:

  • Omvang en grootte. Veranderingen moeten een enge, goed gedefinieerde, op zichzelf staande reikwijdte hebben die zij uitputtend behandelen. Een wijziging kan bijvoorbeeld een nieuwe functie implementeren of een bug verhelpen. Kortere wijzigingen hebben de voorkeur boven langere. Als een CR substantiële wijzigingen aanbrengt in meer dan ~ 5 bestanden, of het langer dan 1-2 dagen duurde om te schrijven, of het meer dan 20 minuten zou duren om te beoordelen, overweeg dan om het in meerdere zelfstandige CR's te splitsen. Een ontwikkelaar kan bijvoorbeeld een wijziging indienen die de API definieert voor een nieuwe functie op het gebied van interfaces en documentatie, en een tweede wijziging die implementaties voor die interfaces toevoegt.
  • Dien alleen complete, zelf-beoordeelde (door diff) en zelf-geteste CR's in. Om recensenten tijd te besparen, test u de ingediende wijzigingen (d.w.z. voert u de testsuite uit) en zorgt u ervoor dat ze alle builds en alle tests en codekwaliteitscontroles doorstaan, zowel lokaal als op de CI-servers, voordat reviewers worden toegewezen.
  • Wijzigingen opnieuw aanbrengen mag het gedrag niet veranderen; omgekeerd zouden gedragsveranderende veranderingen refactoring en codeformaatwijzigingen moeten vermijden. Daar zijn meerdere goede redenen voor:
    - Refactoring-wijzigingen raken vaak veel regels en bestanden en zullen bijgevolg met minder aandacht worden bekeken. Onbedoelde gedragsveranderingen kunnen in de codebasis lekken zonder dat iemand het merkt.
    - Grote refactoring-veranderingen breken cherry-picking, rebasen en andere magie van bronbeheer. Het is erg kostbaar om een ​​gedragsverandering ongedaan te maken die is geïntroduceerd als onderdeel van een repository-brede refactoring-commit.
    - Dure menselijke review-tijd moet worden besteed aan de programmalogica in plaats van stijl, syntaxis of het formatteren van debatten. Wij regelen liever die met geautomatiseerde tooling zoals Checkstyle, TSLint, Baseline, Prettier, etc.

Berichten vastleggen

Het volgende is een voorbeeld van een goed commit-bericht volgens een veel geciteerde standaard:

Samenvatting in hoofdletters, kort (80 tekens of minder)
Indien nodig een meer gedetailleerde verklarende tekst. Verpak het tot ongeveer 120 tekens of zo. In sommige contexten, de eerste
regel wordt behandeld als het onderwerp van een e-mail en de rest van de tekst als de hoofdtekst. De lege regel tussen de
samenvatting van het lichaam is van cruciaal belang (tenzij u het lichaam volledig weglaat); tools zoals rebase kunnen in de war raken als je rent
de twee samen.
Schrijf je commit bericht in de imperatief: "Fix bug" en niet "Fixed bug" of "Fixes bug." Deze conventie komt overeen
omhoog met commit-berichten gegenereerd door commando's zoals git merge en git revert.
Verdere paragrafen komen na lege regels.
- Opsommingstekens zijn ook goed

Probeer te beschrijven wat de commit verandert en hoe het doet:

> SLECHT. Doe dit niet.
Maak opnieuw compileren
> Goed.
Voeg jcsv-afhankelijkheid toe om de IntelliJ-compilatie op te lossen

Recensenten zoeken

Het is gebruikelijk dat de committer een of twee reviewers voorstelt die bekend zijn met de codebasis. Vaak is een van de reviewers de projectleider of een senior engineer. Projecteigenaren moeten overwegen zich op hun projecten te abonneren om op de hoogte te blijven van nieuwe CR's. Codebeoordelingen bij meer dan drie partijen zijn vaak niet productief of zelfs contraproductief omdat verschillende recensenten tegenstrijdige wijzigingen kunnen voorstellen. Dit kan duiden op fundamenteel meningsverschil over de juiste implementatie en moet worden opgelost buiten een codebeoordeling in een forum met hogere bandbreedte, bijvoorbeeld persoonlijk of in een videoconferentie met alle betrokken partijen.

Code-beoordelingen uitvoeren

Een codebeoordeling is een synchronisatiepunt tussen verschillende teamleden en kan dus de voortgang blokkeren. Daarom moeten codebeoordelingen snel zijn (in de volgorde van uren, niet dagen) en moeten teamleden en leads zich bewust zijn van de tijdsbesteding en dienovereenkomstig prioriteit geven aan de beoordelingstijd. Als je denkt dat je niet op tijd een review kunt voltooien, laat het de committer dan meteen weten zodat hij iemand anders kan vinden.

Een beoordeling moet voldoende grondig zijn, zodat de beoordelaar de wijziging met een redelijk detailniveau aan een andere ontwikkelaar kan uitleggen. Dit zorgt ervoor dat de details van de codebasis bij meer dan één persoon bekend zijn.

Als recensent is het uw verantwoordelijkheid om coderingsnormen te handhaven en de kwaliteitsbalk hoog te houden. Het beoordelen van code is meer een kunst dan een wetenschap. De enige manier om het te leren is om het te doen; een ervaren recensent moet overwegen om andere minder ervaren recensenten op hun wijzigingen te plaatsen en hen eerst een review te laten doen. Ervan uitgaande dat de auteur de bovenstaande richtlijnen heeft gevolgd (vooral met betrekking tot zelfbeoordeling en ervoor zorgen dat de code wordt uitgevoerd), is hier een lijst met dingen waar een recensent op moet letten in een code-evaluatie:

Doel

  • Behaalt deze code het doel van de auteur? Elke wijziging moet een specifieke reden hebben (nieuwe functie, refactor, bugfix, enz.). Behaalt de ingediende code dit doel?
  • Vragen stellen. Functies en klassen moeten niet voor niets bestaan. Wanneer de reden niet duidelijk is voor de recensent, kan dit een indicatie zijn dat de code moet worden herschreven of ondersteund met opmerkingen of tests.

Implementatie

  • Denk na over hoe u het probleem zou hebben opgelost. Als het anders is, waarom is dat dan? Behandelt uw code meer (rand) gevallen? Is het korter / eenvoudiger / schoner / sneller / veiliger maar functioneel gelijkwaardig? Is er een onderliggend patroon dat u hebt gezien dat niet wordt vastgelegd door de huidige code?
  • Zie je potentieel voor nuttige abstracties? Gedeeltelijk gedupliceerde code geeft vaak aan dat een meer abstract of algemeen stuk functionaliteit kan worden geëxtraheerd en vervolgens opnieuw kan worden gebruikt in verschillende contexten.
  • Denk als een tegenstander, maar wees er aardig tegen. Probeer auteurs te "vangen" die snelkoppelingen of gevallen missen door te komen met problematische configuraties / invoergegevens die hun code breken.
  • Denk aan bibliotheken of bestaande productcode. Wanneer iemand bestaande functionaliteit opnieuw implementeert, is dat meestal omdat ze niet weten dat het al bestaat. Soms wordt code of functionaliteit doelbewust gedupliceerd, bijvoorbeeld om afhankelijkheden te voorkomen. In dergelijke gevallen kan een codereactie de bedoeling verduidelijken. Wordt de geïntroduceerde functionaliteit al geleverd door een bestaande bibliotheek?
  • Volgt de verandering standaardpatronen? Gevestigde codebases vertonen vaak patronen rond naamconventies, decompositie van programmalogica, gegevenstypedefinities, etc. Het is meestal wenselijk dat wijzigingen worden geïmplementeerd in overeenstemming met bestaande patronen.
  • Voegt de wijziging compilatie- of runtime-afhankelijkheden toe (vooral tussen subprojecten)? We willen onze producten losjes gekoppeld houden, met zo min mogelijk afhankelijkheden. Wijzigingen in afhankelijkheden en het buildsysteem moeten grondig worden onderzocht.

Leesbaarheid en stijl

  • Denk na over je leeservaring. Heb je de concepten binnen een redelijke tijd begrepen? Was de flow gezond en waren de namen van variabelen en methoden gemakkelijk te volgen? Heb je meerdere bestanden of functies kunnen bijhouden? Was je afgeschrikt door inconsistente naamgeving?
  • Voldoet de code aan coderingsrichtlijnen en codestijl? Is de code consistent met het project in termen van stijl, API-conventies, enz.? Zoals hierboven vermeld, geven we er de voorkeur aan om stijldebatten af ​​te handelen met geautomatiseerde tooling.
  • Heeft deze code TODO's? TODO's stapelen zich gewoon op in code en worden na verloop van tijd oud. Laat de auteur een ticket indienen voor GitHub Issues of JIRA en voeg het issue-nummer toe aan de TODO. De voorgestelde codewijziging mag geen commentaarcode bevatten.

Onderhoudbaarheid

  • Lees de testen. Als er geen tests zijn en er zouden moeten zijn, vraag de auteur om wat te schrijven. Werkelijk niet-testbare functies zijn zeldzaam, terwijl niet-geteste implementaties van functies helaas veel voorkomen. Controleer de tests zelf: behandelen ze interessante gevallen? Zijn ze leesbaar? Verlaagt de CR de algehele testdekking? Bedenk manieren waarop deze code kan breken. Stijlstandaarden voor tests verschillen vaak van kerncode, maar zijn nog steeds belangrijk.
  • Brengt deze CR het risico met zich mee van het doorbreken van testcode, staging-stacks of integratietests? Deze worden vaak niet gecontroleerd als onderdeel van de pre-commit / merge-controles, maar ze laten zakken is pijnlijk voor iedereen. Specifieke dingen om naar te zoeken zijn: verwijdering van testhulpprogramma's of modi, wijzigingen in de configuratie en wijzigingen in de lay-out / structuur van artefacten.
  • Wordt deze achterwaartse compatibiliteit verbroken? Zo ja, is het OK om de wijziging op dit punt samen te voegen of moet deze naar een latere release worden gepusht? Onderbrekingen kunnen database- of schemawijzigingen, openbare API-wijzigingen, wijzigingen in de gebruikersworkflow, enz. Omvatten.
  • Heeft deze code integratietests nodig? Soms kan code niet voldoende worden getest met unit-tests alleen, vooral als de code samenwerkt met externe systemen of configuratie.
  • Geef feedback op documentatie op codeniveau, opmerkingen en berichten over vastleggen. Overbodige opmerkingen maken de code rommelig en kortere commit-berichten mystificeren toekomstige bijdragers. Dit is niet altijd van toepassing, maar kwaliteitsreacties en commit-berichten betalen zichzelf in de regel terug. (Denk aan een keer dat je een uitstekende, of echt verschrikkelijke, boodschap of opmerking zag.)
  • Is de externe documentatie bijgewerkt? Als uw project een README, CHANGELOG of andere documentatie bijhoudt, werd het bijgewerkt om de wijzigingen weer te geven? Verouderde documentatie kan meer verwarrend zijn dan geen, en het zal duurder zijn om het in de toekomst te repareren dan om het nu bij te werken.

Vergeet niet om beknopte / leesbare / efficiënte / elegante code te prijzen. Omgekeerd is het weigeren of afkeuren van een CR niet onbeleefd. Als de wijziging overbodig of irrelevant is, weiger deze dan met een toelichting. Als je het onacceptabel vindt vanwege een of meer fatale fouten, keur het dan af, opnieuw met een toelichting. Soms is de juiste uitkomst van een CR 'laten we dit op een heel andere manier doen' of zelfs 'laten we dit helemaal niet doen'.

Wees respectvol naar de recensenten. Hoewel vijandig denken handig is, is het niet jouw functie en kun je niet alle beslissingen nemen. Als u het niet eens kunt worden met uw reviewee met de code zoals deze is, schakel dan over naar realtime communicatie of zoek een derde mening.

Veiligheid

Controleer of API-eindpunten de juiste autorisatie en authenticatie uitvoeren die consistent is met de rest van de codebasis. Controleer op andere veelvoorkomende zwakke punten, bijvoorbeeld zwakke configuratie, invoer van kwaadwillende gebruikers, ontbrekende loggebeurtenissen, enz. Raadpleeg bij twijfel de CR bij een expert op het gebied van toepassingsbeveiliging.

Opmerkingen: beknopt, vriendelijk, bruikbaar

Beoordelingen moeten beknopt zijn en in neutrale taal zijn geschreven. Bekritiseer de code, niet de auteur. Als er iets onduidelijk is, vraag dan om opheldering in plaats van onwetendheid aan te nemen. Vermijd bezittelijke voornaamwoorden, met name in combinatie met evaluaties: "mijn code werkte vóór uw wijziging", "uw methode heeft een bug", enz. Vermijd absolute oordelen: "dit kan nooit werken", "het resultaat is altijd fout".

Probeer onderscheid te maken tussen suggesties (bijvoorbeeld 'Suggestie: extractiemethode om de leesbaarheid te verbeteren'), vereiste wijzigingen (bijvoorbeeld 'Add @Override') en punten die moeten worden besproken of opgehelderd (bijvoorbeeld 'Is dit echt het juiste gedrag? Als dus voeg een opmerking toe waarin de logica wordt uitgelegd. ”). Overweeg om links of verwijzingen naar diepgaande uitleg van een probleem te geven.

Wanneer u klaar bent met een codebeoordeling, geeft u aan in hoeverre u verwacht dat de auteur op uw opmerkingen reageert en of u de CR opnieuw wilt beoordelen nadat de wijzigingen zijn doorgevoerd (bijvoorbeeld: 'Voel je vrij om samen te voegen na het reageren aan de paar kleine suggesties ”versus“ Overweeg mijn suggesties en laat me weten wanneer ik nog een keer kan kijken. ”).

Reageren op beoordelingen

Een deel van het doel van de codebeoordeling is het verbeteren van het wijzigingsverzoek van de auteur; bijgevolg, wees niet beledigd door de suggesties van uw recensent en neem ze serieus, zelfs als u het niet eens bent. Reageer op elke opmerking, ook al is het maar een simpele "ACK" of "klaar". Leg uit waarom u bepaalde beslissingen hebt genomen, waarom een ​​bepaalde functie bestaat, enz. Als u geen overeenstemming kunt bereiken met de recensent, schakelt u over naar real- tijdcommunicatie of vraag een externe mening.

Fixes moeten naar dezelfde branch worden gepusht, maar dan in een afzonderlijke commit. Het pletten tijdens het beoordelingsproces maakt het voor de beoordelaar moeilijk om wijzigingen op te volgen.

Verschillende teams hebben een ander samenvoegbeleid: sommige teams staan ​​alleen projecteigenaren toe om samen te voegen, terwijl andere teams de bijdrager toestaan ​​samen te voegen na een positieve code review.

Persoonlijke code-beoordelingen

Voor de meeste codebeoordelingen zijn asynchrone, op diff gebaseerde tools zoals Reviewable, Gerrit of, GitHub een goede keuze. Complexe wijzigingen of beoordelingen tussen partijen met zeer verschillende expertise of ervaring kunnen efficiënter zijn wanneer ze persoonlijk worden uitgevoerd, voor hetzelfde scherm of dezelfde projector, of op afstand via VTC of tools voor het delen van schermen.

Voorbeelden

In de volgende voorbeelden worden voorgestelde beoordelingsreacties aangegeven met // R: ... opmerkingen in de codeblokken.

Inconsistente naamgeving

klasse MyClass {
  private int countTotalPageVisits; // R: geef variabelen een consistente naam
  private int uniqueUsersCount;
}

Inconsistente methode handtekeningen

interface MyInterface {
  / ** Retourneert {@link Optioneel # leeg} als s niet kan worden geëxtraheerd. * /
  public Optioneel  extractString (String s);
  / ** Retourneert null als {@code s} niet kan worden herschreven. * /
  // R: moet de retourwaarden harmoniseren: gebruik hier ook Optioneel <>
  public String rewriteString (String s);
}

Bibliotheek gebruik

// R: verwijderen en vervangen door Guava's MapJoiner
String joinAndConcatenate (Map  map, String keyValueSeparator, String keySeparator);

Persoonlijke smaak

int dayCount; // R: nit: ik geef meestal de voorkeur aan numFoo boven fooCount; aan jou, maar we moeten het consistent houden in dit project

bugs

// R: dit voert nummeringen + 1 iteraties uit, is dat opzettelijk?
// Zo ja, overweeg dan om de semantiek van de numIterations te veranderen?
voor (int i = 0; i <= numIterations; ++ i) {
  ...
}

Architectonische zorgen

otherService.call (); // R: Ik denk dat we de afhankelijkheid van OtherService moeten vermijden. Kunnen we dit persoonlijk bespreken?

auteurs

Robert F (GitHub / Twitter)