Kode gjennomgang beste praksis

Internett gir et vell av materiale om kodevurderinger: om effekten av kodevurderinger på selskapskultur, om formelle sikkerhetsgjennomganger, kortere guider, lengre sjekklister, humaniserte anmeldelser, grunner til å gjøre kodevurderinger i utgangspunktet, beste praksis, mer best praksis, statistikk om effektiviteten av kodevurderingen for å fange feil, og eksempler på kodevurderinger har gått galt. Åh, og selvfølgelig er det bøker også. Lang historie kort, dette blogginnlegget presenterer Palantirs vurdering av kode. Organisasjoner med dyp kulturell motvilje mot fagfellevurderinger vil kanskje konsultere Karl E. Wiegers utmerkede essay om Humanisering av fagfellevurderinger før de prøver å følge denne veiledningen.

Dette innlegget er kopiert fra guide for beste praksis i vår Java Code Quality verktøykjede, Baseline, og dekker følgende emner:

  • Hvorfor, hva, og når skal jeg gjøre kodevurderinger
  • Forbereder kode for gjennomgang
  • Utføre kodevurderinger
  • Eksempler på kodegjennomgang

Motivasjon

Vi utfører kodevurderinger (CRs) for å forbedre kodekvaliteten og dra nytte av positive effekter på team- og selskapskultur. For eksempel:

  • Pendlere er motivert av forestillingen om et sett med anmeldere som vil se over endringsforespørselen: Pendleren har en tendens til å rydde opp i løse ender, konsolidere TODO-er og generelt forbedre forpliktelsene. Anerkjennelse av kodingskompetanse gjennom jevnaldrende er en kilde til stolthet for mange programmerere.
  • Deling av kunnskap hjelper utviklingsteam på flere måter:
    - En CR kommuniserer eksplisitt lagt til / endret / fjernet funksjonalitet til teammedlemmer som senere kan bygge videre på det utførte arbeidet.
    - Pendleren kan bruke en teknikk eller algoritme som anmelderne kan lære av. Mer generelt er kodevurderinger med til å øke kvalitetslinjen i hele organisasjonen.
    - Anmeldere kan ha kunnskap om programmeringsteknikker eller kodebasen som kan bidra til å forbedre eller konsolidere endringen; for eksempel kan noen andre jobbe samtidig med en lignende funksjon eller fikse.
    - Positiv samhandling og kommunikasjon styrker sosiale bånd mellom gruppemedlemmene.
  • Konsekvens i en kodebase gjør kode enklere å lese og forstå, hjelper til med å forhindre feil og letter samarbeid mellom vanlige og trekkende utviklingsarter.
  • Lesbarheten av kodefragmenter er vanskelig å bedømme for forfatteren hvis hjernebarn det er, og lett å bedømme for en anmelder som ikke har full kontekst. Leselig kode er mer gjenbrukbar, feilfri og fremtidssikker.
  • Uhell feil (f.eks. Skrivefeil) så vel som strukturelle feil (f.eks. Dødkode, logikk eller algoritmfeil, ytelse eller arkitekturproblemer) er ofte mye lettere å oppdage for kritiske anmeldere med et utenfor perspektiv. Studier har funnet at selv korte og uformelle kodevurderinger har betydelig innvirkning på kodekvalitet og feilfrekvens.
  • Samsvar og regelverk krever ofte anmeldelser. CR-er er en flott måte å unngå vanlige sikkerhetsfeller. Hvis funksjonen eller miljøet ditt har betydelige sikkerhetskrav, vil de dra nytte av (og sannsynligvis kreve) gjennomgang av de lokale sikkerhetsstudentene (OWASPs guide er et godt eksempel på prosessen).

Hva du skal gjennomgå

Det er ingen evig svar på dette spørsmålet, og hvert utviklingsteam bør være enige om sin egen tilnærming. Noen team foretrekker å gjennomgå enhver endring som er slått sammen i hovedgrenen, mens andre vil ha en "trivialitet" -grense under hvilken en gjennomgang ikke er nødvendig. Avveiningen er mellom effektiv bruk av ingeniørers (både forfattere og anmeldere) tid og å opprettholde kodekvalitet. I visse reguleringsmiljøer kan det være nødvendig med kodegjennomgang selv for bagatellmessige endringer.

Kodeomtaler er klasseløse: Å være den mest seniorpersonen på laget innebærer ikke at koden din ikke trenger gjennomgang. Selv om kode i sjeldne tilfeller er feilfri, gir gjennomgangen en mulighet for mentorskap og samarbeid, og diversifiserer minimalt forståelsen av kode i kodebasen.

Når du skal gjennomgå

Kodevurderinger skal skje etter at automatiserte kontroller (tester, stil, annen CI) er fullført, men før koden slås sammen til depotets hovedgren. Vi foretar vanligvis ikke formell kodevurdering av samlede endringer siden forrige utgivelse.

For komplekse endringer som skal flette inn i hovedlinjen som en enkelt enhet, men er for store til å passe inn i en rimelig CR, bør du vurdere en stablet CR-modell: Lag en primærgrenfunksjon / storfunksjon og et antall sekundære grener (f.eks. Funksjon / big-feature-api, feature / big-feature-testing, etc.) som hver innkapsler en delmengde av funksjonaliteten og som blir individuelt kodevurdert mot funksjonen / storfunksjonsgrenen. Når alle sekundære grener er slått sammen til funksjon / stor funksjon, lager du en CR for å slå sammen sistnevnte til hovedgren.

Forbereder kode for gjennomgang

Det er forfatterens ansvar å sende inn CR-er som er enkle å gjennomgå for ikke å kaste bort anmeldernes tid og motivasjon:

  • Omfang og størrelse. Endringer bør ha et smalt, veldefinert, selvforsynt omfang som de dekker uttømmende. For eksempel kan en endring implementere en ny funksjon eller fikse en feil. Kortere endringer foretrekkes fremfor lengre. Hvis en CR gjør vesentlige endringer i mer enn ~ 5 filer, eller det tok lenger enn 1-2 dager å skrive, eller det vil ta mer enn 20 minutter å gjennomgå, kan du vurdere å dele den opp i flere selvstendige CR-er. For eksempel kan en utvikler sende inn en endring som definerer API for en ny funksjon når det gjelder grensesnitt og dokumentasjon, og en andre endring som legger til implementeringer for disse grensesnittene.
  • Send bare fullstendige, egenvurderte (av diff) og selvtestede CR-er. For å spare kontrollørenes tid, må du teste de innsendte endringene (dvs. kjøre testpakken) og sørge for at de passerer alle bygg så vel som alle tester og kodekvalitetskontroller, både lokalt og på CI-serverne, før du tildeler anmeldere.
  • Refactoring-endringer bør ikke endre atferd; omvendt, bør en atferdsendrende endring unngå endring av omformering og kodeformatering. Det er flere gode grunner til dette:
    - Refactoring-endringer berører ofte mange linjer og filer, og vil derfor bli vurdert med mindre oppmerksomhet. Utilsiktede atferdsendringer kan lekke inn i kodebasen uten at noen legger merke til det.
    - Store refactoring-endringer bryter kirsebærplukking, omfasing og annen kildekontrollmagi. Det er veldig belastende å angre en atferdsendring som ble introdusert som en del av et repository-bredt refactoring begå.
    - Det skal brukes kostbar menneskelig gjennomgangstid på programlogikken i stedet for diskusjoner om stil, syntaks eller formatering. Vi foretrekker å avgjøre de med automatisert verktøy som Checkstyle, TSLint, Baseline, Prettier, etc.

Foretak meldinger

Følgende er et eksempel på en god forpliktelsesmelding som følger en bredt sitert standard:

Kapitalisert, kort (80 or eller mindre) sammendrag
Mer detaljert forklaringstekst, om nødvendig. Pakk den til omtrent 120 tegn. I noen sammenhenger, den første
linjen blir behandlet som emnet for en e-post og resten av teksten som brødtekst. Den tomme linjen som skiller
sammendrag fra kroppen er kritisk (med mindre du utelater kroppen helt); verktøy som rebase kan bli forvirret hvis du kjører
de to sammen.
Skriv engasjementsmeldingen med pålegg: "Løs bug" og ikke "Fast bug" eller "Fixes bug." Dette stevnet stemmer
opp med engasjementsmeldinger generert av kommandoer som git merge og git revert.
Flere avsnitt kommer etter blanke linjer.
- Kulepoeng er greit også

Forsøk å beskrive både hva engasjementet endrer og hvordan det gjør det:

> DÅRLIG. Ikke gjør dette.
Gjør sammenstilling igjen
> Bra.
Legg til jcsv-avhengighet for å fikse IntelliJ-kompilering

Finne anmeldere

Det er vanlig at pendleren foreslår en eller to anmeldere som er kjent med kodebasen. Ofte er en av anmelderne prosjektlederen eller en senioringeniør. Prosjekteiere bør vurdere å abonnere på prosjektene sine for å få beskjed om nye CR-er. Kodeomtaler blant mer enn tre parter er ofte uproduktive eller til og med motproduktive siden forskjellige anmeldere kan foreslå motstridende endringer. Dette kan indikere grunnleggende uenighet om riktig implementering og bør løses utenfor en kodegjennomgang i et forum med høyere båndbredde, for eksempel personlig eller på en videokonferanse med alle involverte parter.

Utføre kodevurderinger

En kodegjennomgang er et synkroniseringspunkt mellom forskjellige teammedlemmer og har dermed potensial til å blokkere fremgang. Følgelig må kodevurderinger være rask (i størrelsesorden timer, ikke dager), og teammedlemmer og kundeemner må være klar over tidsforpliktelsen og prioritere gjennomgangstiden deretter. Hvis du ikke tror at du kan fullføre en anmeldelse i tide, kan du gi beskjed til pendleren med en gang, slik at de kan finne noen andre.

En anmeldelse skal være grundig nok til at anmelderen kan forklare endringen på et rimelig detaljnivå for en annen utvikler. Dette sikrer at detaljene i kodebasen er kjent for mer enn en enkelt person.

Som korrekturleser er det ditt ansvar å håndheve kodingsstandarder og holde kvaliteten oppe. Å lese kode er mer en kunst enn en vitenskap. Den eneste måten å lære det på er å gjøre det; en erfaren anmelder bør vurdere å sette andre mindre erfarne anmeldelser på endringene sine og få dem til å gjøre en anmeldelse først. Forutsatt at forfatteren har fulgt retningslinjene ovenfor (spesielt med tanke på egenvurdering og sørge for at koden kjøres), her er en liste over ting en anmelder bør ta hensyn til i en kodevurdering:

Hensikt

  • Oppfyller denne koden forfatterens formål? Hver endring skal ha en spesifikk grunn (ny funksjon, refaktor, bugfix osv.). Oppnår den innsendte koden dette formålet?
  • Stille spørsmål. Funksjoner og klasser bør eksistere av en grunn. Når grunnen ikke er klar for anmelderen, kan dette være en indikasjon på at koden må skrives om eller støttes med kommentarer eller tester.

Gjennomføring

  • Tenk på hvordan du ville løst problemet. Hvis det er annerledes, hvorfor er det da? Håndterer koden din flere (kant) saker? Er det kortere / enklere / renere / raskere / tryggere, men likevel funksjonelt likeverdige? Er det noe underliggende mønster du oppdaget som ikke fanges opp med gjeldende kode?
  • Ser du potensiale for nyttige abstraksjoner? Delvis duplisert kode indikerer ofte at et mer abstrakt eller generelt stykke funksjonalitet kan trekkes ut og deretter gjenbrukes i forskjellige sammenhenger.
  • Tenk som en motstander, men vær hyggelig med det. Forsøk å “fange” forfattere som tar snarveier eller mangler saker ved å komme med problematiske konfigurasjoner / inputdata som bryter koden deres.
  • Tenk på biblioteker eller eksisterende produktkode. Når noen implementerer eksisterende funksjonalitet på nytt, er det ofte fordi de ikke vet at det allerede eksisterer. Noen ganger dupliseres kode eller funksjonalitet med vilje, for eksempel for å unngå avhengigheter. I slike tilfeller kan en kodekommentar tydeliggjøre intensjonen. Er den innførte funksjonaliteten allerede levert av et eksisterende bibliotek?
  • Følger endringen standardmønstre? Etablerte kodebaser viser ofte mønstre rundt navnekonvensjoner, nedbrytning av programlogikk, definisjoner av datatype, etc. Det er vanligvis ønskelig at endringer implementeres i samsvar med eksisterende mønstre.
  • Legger endringen sammenhengstid eller kjøretidavhengighet (spesielt mellom delprosjekter)? Vi ønsker å holde våre produkter løst koblet, med så få avhengigheter som mulig. Endringer i avhengigheter og build-systemet bør granskes sterkt.

Lesbarhet og stil

  • Tenk på leseopplevelsen din. Fikk du tak i konseptene på rimelig tid? Var flyten tilregnelig og var variabel og metodenavn enkle å følge? Var du i stand til å holde oversikt over flere filer eller funksjoner? Ble du satt ut av inkonsekvent navngivning?
  • Overholder koden retningslinjene for koding og kodestil? Er koden konsistent med prosjektet når det gjelder stil, API-konvensjoner, etc.? Som nevnt ovenfor foretrekker vi å avgjøre stildebatter med automatisert verktøy.
  • Har denne koden TODO-er? TODO bare hoper seg opp i kode og blir uaktuelle over tid. Be forfatteren sende inn en billett på GitHub Issues eller JIRA og legge ut nummeret til TODO. Den foreslåtte kodeendringen skal ikke inneholde kommentert kode.

vedlikeholdbarhet

  • Les testene. Hvis det ikke er noen tester og det burde være det, kan du be forfatteren skrive noen. Virkelig uprøvbare funksjoner er sjeldne, mens uprøvde implementeringer av funksjoner dessverre er vanlige. Sjekk testene selv: dekker de interessante saker? Er de lesbare? Senker CR den generelle testdekningen? Tenk på måter denne koden kan knekke. Stilstandarder for tester er ofte annerledes enn kjernekode, men fortsatt viktige.
  • Innfører denne CR-risikoen for å bryte testkode, iscenesette stabler eller integrasjonstester? Disse blir ofte ikke sjekket som en del av forhåndsforpliktelse / sammenslåingskontrollene, men å få dem til å gå ned er smertefullt for alle. Spesifikke ting å se etter er: fjerning av testverktøy eller modus, endringer i konfigurasjon og endringer i artefaktoppsett / struktur.
  • Bryter denne endringen bakoverkompatibilitet? Er det i så fall OK å slå sammen endringen på dette tidspunktet, eller bør den skyves inn i en senere utgivelse? Breaks kan inkludere database- eller skjemaendringer, offentlige API-endringer, endringer i arbeidsflyt osv.
  • Trenger denne koden integrasjonstester? Noen ganger kan kode ikke testes tilstrekkelig med enhetstester alene, spesielt hvis koden samhandler med eksterne systemer eller konfigurasjoner.
  • Legg igjen tilbakemelding på dokumentasjonsnivå på kodenivå, kommentarer og meldinger. Overflødige kommentarer rot koden, og terse engasjement meldinger mystifiserer fremtidige bidragsytere. Dette er ikke alltid aktuelt, men kvalitetskommentarer og tilsagnsmeldinger vil betale for seg selv. (Tenk på en tid du så en utmerket, eller virkelig forferdelig, gi beskjed eller kommentar.)
  • Ble den eksterne dokumentasjonen oppdatert? Hvis prosjektet ditt opprettholder en README, CHANGELOG eller annen dokumentasjon, ble det oppdatert for å gjenspeile endringene? Utdatert dokumentasjon kan være mer forvirrende enn ingen, og det vil være mer kostbart å fikse det i fremtiden enn å oppdatere det nå.

Ikke glem å prise konsis / lesbar / effektiv / elegant kode. Derimot er det ikke frekt å avvise eller avvise en CR. Hvis endringen er overflødig eller irrelevant, avslå den med en forklaring. Hvis du anser det som uakseptabelt på grunn av en eller flere dødelige feil, må du avvise det, igjen med en forklaring. Noen ganger er det rette resultatet av en CR "la oss gjøre dette på en helt annen måte", eller til og med "la oss ikke gjøre dette i det hele tatt."

Vær respekt for anmelderne. Mens motstridende tenking er praktisk, er det ikke funksjonen din, og du kan ikke ta alle avgjørelser. Hvis du ikke kan komme til enighet med din anmelder med koden som den er, bytter du til sanntidskommunikasjon eller oppsøk en tredje mening.

Sikkerhet

Kontroller at API-endepunktene utfører passende autorisasjon og autentisering i samsvar med resten av kodebasen. Sjekk for andre vanlige svakheter, for eksempel svak konfigurasjon, ondsinnet brukerinngang, manglende logghendelser, etc. Hvis du er i tvil, kan du henvise CR til en applikasjonssikkerhetsekspert.

Kommentarer: kortfattet, vennlig, handlingsbart

Vurderinger skal være kortfattede og skrevet på nøytralt språk. Kritiser koden, ikke forfatteren. Når noe er uklart, be om avklaring snarere enn å anta uvitenhet. Unngå eiendomspronomen, spesielt i forbindelse med evalueringer: “koden min virket før endringen”, “metoden din har en feil”, osv. Unngå absolutte vurderinger: “dette kan aldri fungere”, “resultatet er alltid galt”.

Forsøk å skille mellom forslag (f.eks. "Forslag: pakke ut metoden for å forbedre lesbarheten"), nødvendige endringer (f.eks. "Legg til @Override"), og punkter som trenger diskusjon eller avklaring (f.eks. "Er dette virkelig riktig oppførsel? Hvis så legg til en kommentar som forklarer logikken. ”). Vurder å gi lenker eller pekere til en grundig forklaring av et problem.

Når du er ferdig med en kodegjennomgang, må du indikere i hvilken grad du forventer at forfatteren vil svare på kommentarene dine, og om du ønsker å revidere CR-en på nytt etter at endringene er implementert (f.eks. "Føl deg fri til å fusjonere etter å ha svart til noen få små forslag ”vs.“ Vennligst vurder forslagene mine og gi beskjed når jeg kan se på nytt. ”).

Svar på anmeldelser

En del av formålet med kodevurderingen er å forbedre forfatterens endringsforespørsel; følgelig ikke bli fornærmet av anmelderens forslag og ta dem på alvor selv om du ikke er enig. Svar på hver kommentar, selv om det bare er et enkelt "ACK" eller "gjort." Forklar hvorfor du har tatt bestemte beslutninger, hvorfor noen funksjoner eksisterer, osv. Hvis du ikke kan komme til enighet med anmelderen, bytt til real- tidskommunikasjon eller søke en ytre mening.

Rettelser skal skyves til samme gren, men i en egen forpliktelse. Squash forplikter under gjennomgangsprosessen gjør det vanskelig for anmelderen å følge opp endringene.

Ulike team har ulik fusjonspolicy: Noen team tillater bare prosjekteiere å slå seg sammen, mens andre team tillater bidragsyteren å slå seg sammen etter en positiv kodevurdering.

Gjennomgang av personkoder

For de fleste kodevurderinger er asynkrone diff-baserte verktøy som Reviewable, Gerrit eller, GitHub et godt valg. Komplekse endringer eller omtaler mellom parter med svært ulik kompetanse eller erfaring kan være mer effektive når de utføres personlig, enten foran den samme skjermen eller projektoren, eller eksternt via VTC eller skjermdelingsverktøy.

eksempler

I følgende eksempler er foreslåtte gjennomgangskommentarer indikert av // R: ... kommentarer i kodeblokkene.

Inkonsekvent navngivning

klasse MyClass {
  private int countTotalPageVisits; // R: navnvariabler konsekvent
  private int unikeUsersCount;
}

Inkonsekvente metodesignaturer

grensesnitt MyInterface {
  / ** Returnerer {@link Valgfri # tom} hvis s ikke kan hentes ut. * /
  offentlig Valgfri  extractString (Strenger);
  / ** Returnerer null hvis {@code s} ikke kan skrives om. * /
  // R: skal harmonisere returverdiene: bruk Valgfritt <> også her
  public String rewriteString (String s);
}

Bibliotekbruk

// R: fjern og erstatt av Guava's MapJoiner
Streng joinAndConcatenate (Kart  kart, String keyValueSeparator, String keySeparator);

Personlig smak

int dagKonto; // R: nit: Jeg foretrekker vanligvis numFoo fremfor fooCount; opp til deg, men vi bør holde det konsistent i dette prosjektet

bugs

// R: Dette utfører numIterations + 1 iterasjoner, er det forsettlig?
// Hvis det er det, kan du vurdere å endre semantikken til numIterations?
for (int i = 0; i <= numIterations; ++ i) {
  ...
}

Arkitektoniske bekymringer

otherService.call (); // R: Jeg tror vi bør unngå avhengigheten av OtherService. Kan vi diskutere dette personlig?

Forfattere

Robert F (GitHub / Twitter)