Fråga till er som jobbar på företag med code-reviews

Permalänk

Fråga till er som jobbar på företag med code-reviews

Jag är nyfiken på hur andra företag hanterar code-reviews, vad har ni för riktlinjer? Jag tycker det är knepigt ibland vad man ska godkänna eller inte med tanke på att alla har olika kompetens.
Ska någon som inte är så erfaren på det som pull-requesten gäller kunna godkänna den t.ex.? Allt kanske ser bra ut för en sådan person (utifrån det man kan bedöma) medan code-reviewer nr 2 upptäcker stora fel.
Är det vettigt att code-reviewa ens om man bara begriper 20% av koden?

Visa signatur

Dator: MB: Gigabyte B760 Gaming X | SSD: Samsung 990 Pro M.2 1TB + Crucial P3 M.2 1TB CPU: Intel Core i7 14700 2.1 GHz 61MB | RAM: Corsair 32GB (2x16GB) DDR5 4800MHz CL40 Vengean | Grafikkort: ASUS Dual GeForce RTX 4060 EVO 8GB OC | Chassi: be quiet! Pure Base 600 Silver | PSU: Corsair RM750e ATX 3.0 750W V3 | Optisk: ASUS BW-12B1ST Blu-Ray/DVD brännare | CPU-kylare: Thermalright Peerless Assassin 120 SE | Operativ: Windows 11 | Scanner: Canon Canoscan 9000F
Övrigt: Nintendo Switch, NES Mini, SNES Mini, Nintendo New 3DS, NES, Famicom AV, PS3, PS5, AppleTV 4K, Synology NAS DS923+ (32GB), iPhone 16 Pro 128GB, LG OLED 55C2,

Permalänk
Medlem

Intressant ämne. Jag jobbar sedan ca 18 månader för första gången på ett sådant ställe. Vi är tre personer med kompetens på koden - en skriver kod, en granskar. Riktlinjer utöver att någon ska granska saknas så vitt jag vet.

Jag brukar insistera på att commit-meddelandet ska innehålla hur ändringen har testats. Även syftet med ändringen och motivering varför det gjorts på just det viset bör framgå. Oavsett om det är en rookie eller en erfaren utvecklare så vill man i efterhand veta hur tankegångarna gick när koden skrevs och vilka hänsyn som måste tas vid nästa ändring.

När koden interagerar mot ett annat system, vars beteende/implementationsval inte är lätt att läsa från kod och dokumentation så brukar jag försöka få det kommenterat i koden.

Ovanstående ger automatiskt att ändringar blir lite mer genomtänkta och testade på ett genomtänkt vis. Det sistnämnda upplever jag är speciellt viktigt med folk som inte jobbat så länge. Detta är i en organisation där testare förväntas ta vid och göra blackbox-testning efter att PR granskats och committats. Enhetstester finns men kan bara täcka en oviktig bråkdel av funktionaliteten.

Jag brukar få mina PR genomsläppta rätt kvickt, men det händer ju att det finns problem kvar i dem som inte upptäckts vid granskning. De flesta regressioner vi får är i kollegors kod, där jag har granskat och där det är något specialfall vi båda har missat. Det händer att jag är rätt jobbig, men jag brukar fråga om mina klagomål är motiverade eller för petiga och med en öppen dialog så brukar det gå bra.

Lite av målet är ju att alla ska förstå all kod. Så om någon bara förstår 20% så har man ju identifierat ett problem. Varför är bara 20% möjlig att förstå? Ska inte varje klass/metod ha kommentarer som motiverar dess existens och vilka numrerade krav/användningsfall de implementerar?

Permalänk
Skrivet av KAD:

Lite av målet är ju att alla ska förstå all kod. Så om någon bara förstår 20% så har man ju identifierat ett problem. Varför är bara 20% möjlig att förstå? Ska inte varje klass/metod ha kommentarer som motiverar dess existens och vilka numrerade krav/användningsfall de implementerar?

Det skulle kunna handla om att man inte förstår sättet det har lösts på i kombination med den syntaxen som har använts. Har någon kodat C# i 1 år jämfört med någon som har gjort det i 20 år har man lite olika förutsättningar.

Visa signatur

Dator: MB: Gigabyte B760 Gaming X | SSD: Samsung 990 Pro M.2 1TB + Crucial P3 M.2 1TB CPU: Intel Core i7 14700 2.1 GHz 61MB | RAM: Corsair 32GB (2x16GB) DDR5 4800MHz CL40 Vengean | Grafikkort: ASUS Dual GeForce RTX 4060 EVO 8GB OC | Chassi: be quiet! Pure Base 600 Silver | PSU: Corsair RM750e ATX 3.0 750W V3 | Optisk: ASUS BW-12B1ST Blu-Ray/DVD brännare | CPU-kylare: Thermalright Peerless Assassin 120 SE | Operativ: Windows 11 | Scanner: Canon Canoscan 9000F
Övrigt: Nintendo Switch, NES Mini, SNES Mini, Nintendo New 3DS, NES, Famicom AV, PS3, PS5, AppleTV 4K, Synology NAS DS923+ (32GB), iPhone 16 Pro 128GB, LG OLED 55C2,

Permalänk
Medlem
Skrivet av oRBIT2002:

Det skulle kunna handla om att man inte förstår sättet det har lösts på i kombination med den syntaxen som har använts. Har någon kodat C# i 1 år jämfört med någon som har gjort det i 20 år har man lite olika förutsättningar.

Då är det väl ett utmärkt sätt att lära sig
Kom med frågor om hur koden fungerar, varför det är gjort som det är gjort osv. Det kanske kan kännas jobbigt att vara den som frågar en massa, men det kommer ge båda parter en djupare förståelse över koden.

Visa signatur

NZXT H510 Flow MSI B450 Tomahawk MAX
AMD Ryzen 5800X3D RX 7900XTX Kingston Fury 64GB
LG C2 42" 4K@120Hz AOC Q27G2U 1440P@144Hz

Permalänk
Medlem

Jag tycker att code reviews har flera syften, och det är inte bara att koden ska vara bra när den mergas.
Om man har väldigt olika nivåer av kompetens i ett team så kan code reviews vara ett bra sätt för den som gör reviewen att lära sig. Man kan ju också ha mer än en person som reviewar och kommenterar, och då ökar kompensspridningen ännu mer.

Ytterligare ett sätt är att kompletera med automatiserade code reviews med system som SonarQube eller CodeScene.

Permalänk

För att komplicera det ytterligare så har vi en faktor som erfarenhet här med. Även om man som granskar fattar vad som körs kanske man inte ser eventuella konsekvenser.

Visa signatur

Dator: MB: Gigabyte B760 Gaming X | SSD: Samsung 990 Pro M.2 1TB + Crucial P3 M.2 1TB CPU: Intel Core i7 14700 2.1 GHz 61MB | RAM: Corsair 32GB (2x16GB) DDR5 4800MHz CL40 Vengean | Grafikkort: ASUS Dual GeForce RTX 4060 EVO 8GB OC | Chassi: be quiet! Pure Base 600 Silver | PSU: Corsair RM750e ATX 3.0 750W V3 | Optisk: ASUS BW-12B1ST Blu-Ray/DVD brännare | CPU-kylare: Thermalright Peerless Assassin 120 SE | Operativ: Windows 11 | Scanner: Canon Canoscan 9000F
Övrigt: Nintendo Switch, NES Mini, SNES Mini, Nintendo New 3DS, NES, Famicom AV, PS3, PS5, AppleTV 4K, Synology NAS DS923+ (32GB), iPhone 16 Pro 128GB, LG OLED 55C2,

Permalänk
Skrivet av oRBIT2002:

Är det vettigt att code-reviewa ens om man bara begriper 20% av koden?

Helst vill man ju att koden skall granskas av någon bättre eller lika erfaren som en själv. Hen kan hitta fel, komma med tips om att man kan göra så här istället eller påpeka att det redan finns kod som gör precis det här. Men det viktiga är ytterligare ett par ögon på koden och det funkar med en mindre erfaren utvecklare också. En total rookie är ju inte optimalt. Det krävs att man har lite grundkoll på koden för att man skall kunna komma med vettiga synpunkter och 20% kanske är ett gränsfall. Men även en som bara kan koden till 20% kan hitta saker som man missat. Samtidigt så får ju den som granskar se lite mer av systemet och kanske levlar upp till 21%

Bara det faktum att jag vet att koden kommer granskas får i alla fall mig att bry mig lite extra. Under granskningen vill man inte ha en lång kedja med incheckningar av varenda steg om tagits. Jag brukar städa i incheckningarna, lite interactive rebase och squash så att det blir ett antal logiska steg som tas snarare än slumpvisa ändringar i den ordning jag råkade göra dem. Detta gör det lättare både för den som granskar och för mig själv om jag måste gräva i koden vi ett senare tillfälle.

Så svaret är: ja. Det är vettigt med kodgranskning även om den som granskar inte har järnkoll på all kod.

Permalänk
Medlem

Tycker det är viktigt att nyanställda också gör code reviews, detta för att bekanta sig med alla projektets delar.
Att se hur andra löst problem är också lärorikt. Har man ingen kodstandard känns det ännu viktigare.

Permalänk
Medlem

Som andra säger så har ju code reviews har ju fler syften än bara att hitta buggar:

  • Lärandesyfte i hur man kan skriva bra (eller dålig) kod

  • Kunskapsdelning om hur man löste ett visst problem och vad man jobbat med, så att reviewern sen kan använda sig av koden för sina features, eller hjälpa till att lösa buggar om någon sån slinker igenom

  • En extra check att man skrivit kod som faktiskt löser problemet, så man inte misstolkat något

  • Se till att man inte råkat committa något som inte skulle vara med (typ en api nyckel eller extra debug printar)

Samtliga delar ovan kan man delta i oavsett sin kunskapsnivå eller erfarenhet med projektet, men på sin egen nivå.

Att hitta komplexa logikfel eller buggar i koden kanske jag inte förväntar mig att en junior ska göra, men jag värdesätter att man har ett par extra ögon på koden som kan ställa frågor om något ser oklart eller fel ut. Sen är det upp till automatiska tester och manuella regressionstester att identifiera om nån bugg trots allt slunkit igenom.

Permalänk
Medlem

Det kan vara både bra och dåligt som med så mycket annat med reviews. "context" är viktigt för att få ut så mycket bra som möjligt men några saker att tänka på

  • Om man gör reviews när koden är färdig för incheckning blir det oftast jobbigt för alla. Bättre att be folk göra reviews för önskade delar under tiden utvecklaren håller på.

  • Diskutera kod är mycket bättre än att skriva kommentarer i reviews.

  • Helt ok att checka in dålig kod, fokusera på delar och tänk på att kod alltid "lever", skriv om bättre senare. Viktigaste är att kod skrivs så den kan refaktoreras eller bytas ut.

  • Erfarna kontra juniora, alla är olika och reviews kan göra folk rädda för att testa saker. Hur skall de lära sig om de aldrig får göra fel eller se konsekvenser av inte så bra lösningar.

Permalänk
Medlem
Skrivet av Ingetledigtnamn:

Helst vill man ju att koden skall granskas av någon bättre eller lika erfaren som en själv.

Ibland kan ju även de med lite sämre erfarenhet komma med något som är bra, exempelvis lite "out-of-the-box"-tänkande eller något annat användbart.

Visa signatur

Byt namn på Nvidia till NvidAI

Permalänk
Medlem

Har jobbat på ställen där man ska ha code reviews, men när personer skiver "hur tänkte du här", "hjärndött", "varför"? Då tröttnar man snabbt och skiter i (alltså inte bara till mig, utan generellt till alla i teamet).

Själv försöker jag skriva konstruktivt, ibland ge kodexempel i kommentaren. Man bör ju även prata med den som gjort PR. Det är givande och tagande.

Sen har vi dom som inte lyckas lära sig ett skit, och fortsätter skiva koden helt galet. Det är fan det värsta.

Visa signatur

Dator:
Intel Core i5 14600K 3.5 GHz 44MB
ASUS Prime Z790-P
G.Skill 32GB (2x16GB) DDR5 7200MHz CL 35 Triden
Gainward GeForce GTX 1080 8GB

Permalänk
Medlem
Skrivet av klk:

Det kan vara både bra och dåligt som med så mycket annat med reviews. "context" är viktigt för att få ut så mycket bra som möjligt men några saker att tänka på

  • Om man gör reviews när koden är färdig för incheckning blir det oftast jobbigt för alla. Bättre att be folk göra reviews för önskade delar under tiden utvecklaren håller på.

  • Diskutera kod är mycket bättre än att skriva kommentarer i reviews.

  • Helt ok att checka in dålig kod, fokusera på delar och tänk på att kod alltid "lever", skriv om bättre senare. Viktigaste är att kod skrivs så den kan refaktoreras eller bytas ut.

  • Erfarna kontra juniora, alla är olika och reviews kan göra folk rädda för att testa saker. Hur skall de lära sig om de aldrig får göra fel eller se konsekvenser av inte så bra lösningar.

Tycker det här är skitviktigt att trycka på - En Pull Requests främsta syfte är inte att någon ska agera kod-polis, utan att man ska dela och sprida kunskap, antingen om hantverket i sig, eller om domänen/problemet man jobbar med. Om man sitter och kommenterar på namngivning, antal mellanslag kontra tabbar etc, är man helt fel ute.

Om man hela tiden har fokus på learning/teaching brukar det bli både värdefullt och trevligt, sitter man med rödpenna och påpekar lint-regler blir det sunkigt.

Man ska fostra en atmosfär där alla VILL bli code-reviewad, man kan ju ha missat något liksom.

Sen alla ceremonier om hur commit meddelanden ska skrivas, vad det ska finnas för beskrivningar, bilder, test-instruktioner etc i en PR är helt sekundärt i slutändan. Det som är viktigt är ju att koden funkar och att alla kan förstå den, inte bara idag, utan om ett år.

Permalänk
Medlem

kloka kommentarer i tråden. det finns mycket att läsa om det här. jag tycker det är viktigt att försöka få till en kultur där alla tar code reviews på allvar och gör det respektfullt.

ju fler som kollar ju bättre, men använd sunt förnuft. jag kan till exempel göra en review men också skriva att även om jag tycker det ser bra ut så skulle det kännas bättre om en person som är expert på X också kollar på den. eller från andra hållet, för en viss ändring kan jag säga att jag framförallt vill att en viss person ska ta en titt pga hens expertis.

cheers