Jak děláte code review?

[B.H.L.]

Ahoj.

Zejména (ale nejen) v souvislosti s nástupem LLM generátorů kódu mi vrtá hlavou, jak přistupovat ke code review. Jak je děláte u Vás?

Je to jenom formalita, kdy to bezmyšlenkovitě odbouchnete?
Procházíte každý řádek změněného kódu a zamýšlíte se nad tím, jako byste to psali sami?

V našem týmu se to bere spíš jako formalita vynucovaná architekty, takže si to většinou bez čtení schválíme a jdem dál. Je to norma?

Máte nějaký metody jak si code review zefektivnit či zrychlit?

[Reklama]

[skrivy]

Procházím to a hledám v tom principiální chyby nebo věci, které by mohly ohrozit výkon aplikace.

[oss]

Co si vsimam, tak ludia co robia cez LLM si nechavaju aj code review robit cez LLM a po tyzdni sa do kodu uz vobec nepozru (a to nazyvaju sami seba programatormi).

[Ink]

U LLM, ale i non-LLM kódu je potřeba hlavně pochopit, jak daný commit řeší zadaný problém. Dělá to, co má? Nedělá to něco navíc, co nebylo v zadání (pochopil pisatel kódu zadání, bylo jasné, co má dělat)? Nedělá to moc složitě (z výpočetního hlediska, struktury)? Drží to guidelines pro psaní kódu? Je kód srozumitelný, dobře okomentovaný, používá smysluplné identifikátory?

Některý commit lze schválit "formálně", když člověk na první dobrou vidí, že je vše v pořádku. Jindy je třeba se na to podívat pořádně. V dnešní době by ale každopádně měl první kolo code review dělat nějaký LLM, odhalí toho docela dost a rychle.

[HelloWord]

..mňa by skôr zaujímalo, či vznikajú aj "názorové konflikty" medzi programátormi, ktorí CodeReview vykonávajú. V praxi čo sledujem, CodeReview vykonávajú aspoň viac než dvaja programátori. Nebavíme sa o malých commitoch. Vždy ma hlavné slovo jeden z programátorov ? Leader ? Máte aj takú skúsenosť, že CedeReview programátori boli menej skúsení od Vás a prišlo k "ťažkej" výmene názorov ?

[Reklama]

[B.H.L.]

Procházím to a hledám v tom principiální chyby nebo věci, které by mohly ohrozit výkon aplikace.

Takže neřešíte třeba až tak úplně business logiku, jenom jestli tam nejsou nějaký úplně strašný a nebezpečný prasárny?

Co si vsimam, tak ludia co robia cez LLM si nechavaju aj code review robit cez LLM a po tyzdni sa do kodu uz vobec nepozru (a to nazyvaju sami seba programatormi).

Což je ovšem naprosto pochopitelný - jenom kontrolovat cizí kód je nevděčná práce a troufnu si tvrdit, že přinejlepším vědomí a svědomí málokdo dokáže jenom čtením porozumět cizímu kódu tak dobře, jako by ho psal sám.

Ale u části projektů vlastně na kvalitě kódu nesejde - Hlavně že to dělá, co to dělat má.

U LLM, ale i non-LLM kódu je potřeba hlavně pochopit, jak daný commit řeší zadaný problém. Dělá to, co má? Nedělá to něco navíc, co nebylo v zadání (pochopil pisatel kódu zadání, bylo jasné, co má dělat)? Nedělá to moc složitě (z výpočetního hlediska, struktury)? Drží to guidelines pro psaní kódu? Je kód srozumitelný, dobře okomentovaný, používá smysluplné identifikátory?

To zní, jakože na tom review musíte strávit snad stejně času jako původní řešitel, ne?

Některý commit lze schválit "formálně", když člověk na první dobrou vidí, že je vše v pořádku.

Jak tohle jste schopen posoudit na první dobrou? Nebo to je myšleno spíš v kontextu triviálních změn?

V dnešní době by ale každopádně měl první kolo code review dělat nějaký LLM, odhalí toho docela dost a rychle.

JJ, na tohle jsou ty plechovky super. U vygenerovanýho kódu jim pořád úplně nevěřím, ale review je možnost napáchat škody minimální, ale většinou něco odchytí.

[Zopper]

U LLM, ale i non-LLM kódu je potřeba hlavně pochopit, jak daný commit řeší zadaný problém. Dělá to, co má? Nedělá to něco navíc, co nebylo v zadání (pochopil pisatel kódu zadání, bylo jasné, co má dělat)? Nedělá to moc složitě (z výpočetního hlediska, struktury)? Drží to guidelines pro psaní kódu? Je kód srozumitelný, dobře okomentovaný, používá smysluplné identifikátory?

To zní, jakože na tom review musíte strávit snad stejně času jako původní řešitel, ne?

Vůbec ne. U review nemusím vymýšlet, jak to celé poskládat, nemusím si projít těch 10 slepých cestiček, a tak dál. Pokud kód následuje mě známé vzorce, tak typicky nepřemýšlím řádek po řádku, ale po blocích. Podobně jako text nečtu slovo po slově, ale jako větu (dyslektici prominou). Například ohledně zmiňované výpočetní složitosti typicky budu chytat jen vnořené smyčky, častý opakovaný un-boxing při práci s jednoduchými typy, a takové věci. Ve většině případů ale nebudu řešit, jestli tohle je o 10% rychlejší, nebo pomalejší výpočet.

Některý commit lze schválit "formálně", když člověk na první dobrou vidí, že je vše v pořádku.

Jak tohle jste schopen posoudit na první dobrou? Nebo to je myšleno spíš v kontextu triviálních změn?

Za mě tohle jsou triviální změny (ale definuj triviálnost ), nebo reviews, kde já jsem jen pro formální approval ("jo, může se použít tahle naše deprecated věc, protože to je jen přesun funkcionality z jedné třídy do druhé a existuje ticket na odstranění").

V dnešní době by ale každopádně měl první kolo code review dělat nějaký LLM, odhalí toho docela dost a rychle.

JJ, na tohle jsou ty plechovky super. U vygenerovanýho kódu jim pořád úplně nevěřím, ale review je možnost napáchat škody minimální, ale většinou něco odchytí.

+1, a z mojí zkušenosti mají větší šanci si všimnout chybějící změny, než člověk. Například přidal se nový volitelný field, to zahrnuje změnu 5 různých DTO, ale změnily se jen 4. Testy nepadají, protože se to taky přidalo jen do 4 testů z 5.

[Martin Poljak]

Co si vsimam, tak ludia co robia cez LLM si nechavaju aj code review robit cez LLM a po tyzdni sa do kodu uz vobec nepozru (a to nazyvaju sami seba programatormi).

Nevím, u nás děláme skoro jenom pomocí LLM, na code-review máme komplexní skills, ty rutinně používáme, ale stejně děláme code-review dost pečlivě i ručně. Takže to záleží. Není to žádné pravidlo. Vždycky platilo, že jsou lidé lepší a horší a bude to platit i dál. AI to neoslabuje, spíš naopak zesiluje.

..mňa by skôr zaujímalo, či vznikajú aj "názorové konflikty" medzi programátormi [...] Máte aj takú skúsenosť, že CedeReview programátori boli menej skúsení od Vás a prišlo k "ťažkej" výmene názorov ?

Naprosto. Za mě code-review velmi často totálně rozbíjí týmovou morálku. K tomu, aby to tak nebylo je třeba potkat fakt rozumné a neagresivní lidi. A takových je bohužel celkem málo.

Takže neřešíte třeba až tak úplně business logiku, jenom jestli tam nejsou nějaký úplně strašný a nebezpečný prasárny?

Záleží, na jaké úrovni myslíte. Na té nejnižší samozřejmě ano nebo pokud je tam nějaký zjevný problém. Na té vyšší ne. Napsat kód je 30 % práce vývojáře. Takže kdybych to měl řešit komplexně, vlastně budu tu práci dělat z velké části podruhé.

U LLM, ale i non-LLM kódu je potřeba hlavně pochopit, jak daný commit řeší zadaný problém. Dělá to, co má? Nedělá to něco navíc, co nebylo v zadání [...]

To zní, jakože na tom review musíte strávit snad stejně času jako původní řešitel, ne?

To má hlavně dělat ten, kdo tu AI vede a zadává ji práci.

Některý commit lze schválit "formálně", když člověk na první dobrou vidí, že je vše v pořádku.

Jak tohle jste schopen posoudit na první dobrou? Nebo to je myšleno spíš v kontextu triviálních změn?

Za mě třeba přesně tak.

[Pavel Tišnovský]

Ahoj.

Zejména (ale nejen) v souvislosti s nástupem LLM generátorů kódu mi vrtá hlavou, jak přistupovat ke code review. Jak je děláte u Vás?

Je to jenom formalita, kdy to bezmyšlenkovitě odbouchnete?
Procházíte každý řádek změněného kódu a zamýšlíte se nad tím, jako byste to psali sami?

V našem týmu se to bere spíš jako formalita vynucovaná architekty, takže si to většinou bez čtení schválíme a jdem dál. Je to norma?

Máte nějaký metody jak si code review zefektivnit či zrychlit?

Záleží, k čemu se code review dělá. Jestli je to "jen" kvůli zajištění nějaké maintainability nebo i proto, aby se junioři něco nového naučili. U nás se snažím už jen o to první, protože předávání informací k juniorům přestalo (i "díky" LLM) být funkční a ani to asi není vyžadované. Takže: zapnuté všechny možné lintery s docela přísnými pravidly, definované agent.md s poměrně přesnou speckou, jak má kód vypadat, skills pro vývojáře, první nástřel kontroly je přes LLM a potom nastupuje ruční review, které je hodně časově náročná a vlastně i nevděčná činnost.

[listoper]

Jeste je zajimave se na to podivat z jineho pohledu.
Delate code review pred merge nebo az po nem?

Protoze tzv. gatekeeping code reviews ktere nedovoluji neco mergnout dokud nebude approvnuto jsou trochu dvojsecna zbran (jako vsechno... .)
1. Vede to na to, ze lidem se nechce a zacnou delat formalni approvaly.. nebo approvujou podle toho od koho je MR...
2. Vede to na to, ze si clovek rekne ze to po nem nekdo jeste zkontroluje takze je to sdilena odpovednost a treba nedava takovej pozor... ("Stejne mi to na poprvy vratej s poznamkama tak co bych se s tim drbal")
3. Vede to na to, ze se tolik netestuje a automatizace je na druhe koleji...
4. hrozne to zpomaluje flow... (mam treba rozdelany 3 pull requesty zaroven a tlacim je prez code review 3 dny nez si lidi udelaji cas na review zapracuju pozadovany zmeny apod.. )


Pokud naopak mam takovej test suite, ze tomu verim a mam v pipeline lintery a vsechny mozny udelatka co kdyz jsou spravne nastaveny tak spravne shodi build... A pak mam spravne nastavenej deployment a observability stack kterej sam  zareaguje treba uz pri nasazeni na 10% podu ze nejaka metrika vykazuje anomalie a udela automatickej rollback...  Tak muzu mit duveru v to, ze to proste pustim az na prod. A pri prusvihu mam zase tooling abych mohl rychle jit zpet.. nebo rychle zafixovat...

Zalezi na tom... pokud mas mission critical systemy (treba vesmir a letectvi) nebo dokonce life-critical (skoro cokoliv ve zdravotnictvi) Tak se to musi hodne zvazit treba ten cas a energii do toho dat... ale jinde muze byt vyznamne levnejsi to proste "poslat"

A pak to "ex post" code review muze naopak mit velky prinos.. udela se jednou za cas meeting kde se posedi a dohromady se probere co jde spravnym smerem a co jsou veci ktery by se radsi meli opravit... a vysvetli se proc a treba se to muze zdokumentovat pokud jsou v guidelines nejaky diry...

U me na tom PoC co rozebiram v jinem tematu je zajimave jak se CR posouva do SR(Spec review).
Uplne krasny je, kdyz dokazu dodat tu specku ktera ma v sobe u kazdeho usecase tlacitko ktere spusti odpovidajici test case. Na to si uzivaci hrozne rychle zvykaji a pri dostavam krasnej feedback a kutej mi tam corner cases kde to zkousej rozbit..

[LeosB]

"Ex post" code review absolutně nefunguje, protože pak už nikdo nemá žádný důvod se mu věnovat. Jediné co funguje je vydírání tím že nebude povolený merge dokud se nedořeší code review.