Přejít k navigační liště

Zdroják » Různé » Jak na code review

Jak na code review

Články Různé

Co je code review a jak jej provádět? Kdo poskytuje review? Jak review probíhá? Jak probíhá diskuse kolem?

Nálepky:

Text vyšel původně na autorově webu.

Code review je kontrola změn v kódu před tím, než jsou začleněny do projektu. V praxi dnes code review vypadá nějak takto:

  • Uršula chce udělat změnu v projektu. Změny provede na své větvi kódu a vytvoří Pull Request, tedy žádost o začlenění těch to změn.
  • Uršula najde vhodného člověka, který by jí mohl udělat code review. Kdo je vhodným člověkem, záleží na tom, jaké jsou procesy nebo zvyklosti ve firmě či na tom daném projektu. V tomto případě to je Věnceslav. Pokud Uršula používá GitHub, může tam Věnceslava přímo naklikat jako někoho, od koho chce review.
  • Věnceslav si přečte popis Pull Requestu. Díky tomu zjistí, čeho se týká a co je vlastně účelem změn. V tomto kontextu pak prohlédne kód. Může komentovat jednotlivé části a když je hotov, poskytne nějaký verdikt – buďto PR schválí, nebo zamítne.
  • Uršula diskutuje s Věnceslavem o změnách. Nakonec jde a opraví nalezené chyby (nejčastěji tím, že přidá další commity do téže větve, tedy i do téhož PR).
  • Jestli začlenění schválených změn (merge) nakonec provede Uršula nebo Věnceslav, je opět na dohodě.

GitHub dnes člověka celkem pěkně tímto procesem vede. Kolem této základní kostry, jež je všem společná, je ovšem spousta věcí, které si rozhodujeme sami. Kdo poskytuje review? Jak review probíhá? Jak probíhá diskuse kolem PR?

Jedna z věcí, které jsou od počátku zcela zakořeněny v softwarové kultuře Apiary, jsou právě code reviews. Natolik, že pokud se výjimečně stane, že začleňuji kód, který review nedostal, cítím se stejně provinile, jako bych použil gotoeval, nebo jinou věc, která programátory od mala budí ze spaní. Jak to tedy funguje u nás?

Kdo kontroluje

Když nás bylo málo a neměli jsme firmu rozdělenou na týmy, dělal každý zrovna to, co bylo potřeba. Každý rozuměl každé části produktu, a uměl komukoliv udělat code review. Velmi brzy po tom, co jsem nastoupil, jsem dělal review ostříleným mazákům a oni mně. V code review neexistovala žádná hierarchie, která by říkala, že zkušenější dělají review méně zkušeným. Díky tomu se znalost codebase a schopnost code review hezky rozprostřela. Také nikdo neměl pocit, že by byl chytřejší než ostatní, nebo neomylný. Rovní s rovnými. Ono totiž pokud vám dělá review začátečník a nepochopí váš kód, není to jeho chyba. Napsali jste příliš komplexní a nepochopitelný kód. Až tam bude chvíli ležet a vy se k němu po čase vrátíte, budete ho také číst jako začátečník.

Když jsme vytvořili týmy kolem produktových oblastí (bounded contexts), změnilo se jenom to, že reviews si dělají lidé převážně v rámci týmu.

Co se kontroluje

Code review se dělá zcela na všem, co má být začleněno do hlavní větve. I ve chvíli, kdy jsem ve tři ráno musel v rámci služby vstát a opravit produkci kvůli nějakému průšvihu, nechal jsem si udělat code review. Samozřejmě ne zcela vždy (někdy na to prostě nebyl čas) a také pomůže, když máte i ve tři hodiny polovinu firmy vzhůru, protože jsou v San Franciscu :-D

Neexistuje, že bych začal novou knihovnu na zelené louce a tvořil bych to tam sám, bez code review. Kdo pohlídá, že ji navrhuji správně? Kdo bude schopen ji udržovat, když pojedu na dovolenou? Díky code review jsou zaručeny alespoň základní kvality kódu. A také existují minimálně dva lidé ve firmě, kteří danému kódu rozumí.

Kdy se kontroluje

Toto nemáme úplně pevně dáno, ale slušností je udělat code review co nejdřív, kdy má dotyčný Věnceslav kousek volného času. Nevytrháváme se tedy z programátorského flow, ale v zásadě se očekává, že review proběhne aspoň do 24 hodin. Jestliže se totiž na review čeká dlouho, začně to příliš blokovat Uršulu. Někde (zdroj jsem nedohledal) jsem četl, že reviews by se měly dělat ihned, protože neblokovat někoho, kdo má práci hotovou, má vyšší hodnotu než chodit po špičkách kolem flow někoho, kdo programuje. Svým způsobem mi to smysl dává.

O čem se (ne)diskutuje

Zde záměrně smíchám to, co ve firmě děláme všichni, a to, co dělám já. Nebo co bych si přál, aby bylo běžné. Když přijdu k PR jako někdo, kdo má dělat review, postupuji následovně:

  • Přečtu si popis PR. Pokud jej nemá nebo nevysvětluje, proč jsou změny prováděny, PR zamítnu.
  • Podívám se, zda prošly testy v Continuous Integration. Pokud ne, PR zamítnu. Autor mě měl pozvat k review až ve chvíli, kdy je vše zelené. Mám-li dobrou náladu, podívám se proč testy spadly a napíšu komentář, který autorovi pomůže s vyřešením problému.
  • Jestliže se změna týká něčeho, co ovlivňuje API nebo chování produktu, podívám se na změny v dokumentaci. Pokud žádné změny nejsou, PR zamítnu. Pokud jsou, přečtu si dokumentaci a snažím se z ní pochopit, co změna dělá. V kontextu tohoto porozumění potom dělám review všeho ostatního. Jestli se něco od popisu liší, je to zásadní problém.
  • Jako první hned po dokumentaci se podívám na testy. Testy jsou nejdůležitější část PR. Pokud chybí, PR zamítám. Důkladně čtu testy a přemýšlím nad tím, zda to, co testují je opravdu to, čeho chtěl autor změn dosáhnout a co je v popisu změn. Není nic horšího než testy, které testují nesmysly.
  • V tuto chvíli už přesně vím, jak má věc fungovat. Přečetl jsem totiž dokumentaci a testy. Mohu tedy pokračovat k vlastnímu kódu.

Začínání testy považuji po čase jako jednu z nejdůležitějších kvalit toho, kdo dělá code review. Začlenění špatných testů totiž dává falešný pocit bezpečí, že něco funguje tak, jak má. Tikající bomba! To už je lepší nemít žádné testy, protože potom aspoň víte, že o funkčnosti daného kódu nevíte vůbec nic, a jednáte s opatrností.

Jak se hodnotí

Po celou dobu review si v hlavě připomínky dělím na tři kategorie:

  • nitpick – Něco, co není vůbec důležité, ale OCD mi nedovolí o tom nenapsat komentář. Většinou věci ohledně závorek, mezer, velikosti písmen, atd.
  • blocking – Zásadní problém, kvůli kterému nemůže být PR začleněn a proč mu dávám zamítavý verdikt. Kód zapomněl na nějaký edge case a vybouchne, je tam výrazný performance nebo security problém, apod.
  • Ostatní připomínky jsou někde mezi. K diskusi, ke zvážení. Kód bude fungovat správně i bez nich. Architektura, pojmenovávání věcí, struktura kódu, použití jiné knihovny, aj.

Připomínky podle toho jasně označím. V negativním závěřečném hodnocení zopakuji, co přesně bylo tak problematické, že jsem kvůli tomu PR nepustil dál. Nejsou-li ale v kódu žádné blocking připomínky, dám mu zelenou. Je na autorovi, zda má čas se se mnou vybavovat, zda chce opravovat drobnosti, nebo jestli naopak musí mít tento kód nutně zítra na produkci a raději udělá okamžitě merge. Běžně se stává i to, že autor souhlasí s připomínkami, ale kód potřebuje co nejdříve začlenit, takže to udělá a k připomínky později opraví v dalším, dedikovaném PR, nebo pro ně založí issues – aby se na ně nezapomnělo.

Mnoho nitpick připomínkám se lze vyhnout, pokud se používá nějaký coding style guide a vynucuje se pomocí linteru (např. PEP8/pycodestyleAirbnb/ESLint). Přitom s linterem doporučuji neotravovat při lokálním vývoji – tam ať si ho každý nainstaluje a zapne/vypne dle libosti – ale v Continuous Integration. Při prototypování totiž linter spíše překáží. Pokud ale neprojde linter v CI, nemůže být kód začleněn do společné codebase. Díky linteru se také lépe udržuje téma diskuse v PR. Nebavíme se o mezerách a závorkách, ale o tom, jestli kód dělá to, co dělat má. A o jeho architektuře.

Jak se diskutuje

Code review je psaná komunikace mezi dvěma lidmi a jako taková trpí množstvím příležitostí k tomu, aby se dotyční začali nenávidět. Stručně doporučím dvě věci:

  • Pravidlo číslo jedna je odosobnit se od svého kódu. Pokud někdo píše, že váš kód je špatně, neznamená to, že chtěl ranit vaše city, že neumíte psát dobrý kód, že vás vyhodí z práce, že jste idiot, nebo že celý váš život nestojí za nic. Nejspíš chtěl jen napsat, že váš kód je špatně. Stejně tak při review nemyslete na toho, kdo kód psal, a koho volil do parlamentu, ale soustřeďte se na to, zda bude daná věc fungovat.
  • Přečtěte si Humanizing among coders. Nebo si to pusťte na YouTube.

Přemýšleli jste v minulém odstavci nad tím, jakou má funkci označovat připomínky jako nitpick, když mají při verdiktu stejnou váhu jako cokoliv, co není blocking? Je to proto, aby dal Věnceslav chudince Uršule najevo, že i když k jejímu PR napsal 60 komentářů, tak 55 z nich jsou „jen takové hlouposti, neboj“.

Kdo začleňuje

Kdo začlení schválený PR? To záleží na tom, co je navázáno na začlenění kódu do hlavní větve. Často merge způsobí, že se někde automaticky nasadí nová verze aplikace, nebo že se vydá nová verze knihovny. Co když ji ještě vydat nechceme? Co když tato změna nejdříve vyžaduje nějaké změny na produkční databázi? Tyto věci ví autor kódu, tedy Uršula. Proto by měla udělat merge ona. Věnceslav akorát „dá zelenou“.

Kromě této ale existuje ještě jedna praxe, která říká, že v momentě, kdy je PR přijat, tak má být i začleněn. Pokud je při tom potřeba udělat něco speciálního, má to být napsáno v popisu PR a Věnceslav to má udělat a změny hned nasadit. Pokud Uršule napsal nějaké neblokující připomínky, ta je může adresovat v novém PR. Tento přístup zrychluje nasazování kódu na produkci, protože se odstraňují některá čekání mezi Uršulou a Věnceslavem. Zároveň to klade nároky na Věnceslava – má zodpovědnost za nasazení aplikace, takže si bude dávat pozor, aby review udělal dobře.

Deset nejčastějších chyb

Když jsem si s lidmi vyprávěl o code review, narazil jsem mnohdy na to, že jej dělají, ale vždy v nějaké z mého pohledu „jednonohé“ formě. Vždy tomu něco chybělo k tomu, aby to fungovalo jako ta pěkná kontrolní mašinérie, jakou máme v práci. Rozhodl jsem se tedy o naše zvyklosti z Apiary a moje vlastní zkušenosti podělit.

Není to vyčerpávající, ale to ani být nemělo. Na Zdrojáku na téma code review už dříve vyšly tři články a mimo český internet je určitě také spousta zdrojů, jak se v kontrole kódu zdokonalit. Jeden z nejlepších článků na toto téma je za mě jednoznačně Top ten pull request review mistakes, kde Scott Nonnenberg rozebírá časté chyby při review a musím přiznat, že minimálně v rámci mojí praxe jsou to zásahy do černého.

Jak děláte code review vy? Schválili byste tento článek? A věděli jste, že k němu taky můžete udělat code review? :-)

Komentáře

Subscribe
Upozornit na
guest
10 Komentářů
Nejstarší
Nejnovější Most Voted
Inline Feedbacks
View all comments
Hryzo

Ake nastroje pouzit v komercnej sfere, kde nemame k dispozicii GitHub? GitLab? Na planovanie pouzivame JIRA.

k

Proc bych nemel mit jeden z trojice GitHub, GitLab, Bitbucket?

Jarda

Proč nemáte k dispozici GitHub? Pracuju v „komercnej sfere“ už dlouhá léta a GitHub používám docela dost. Možná je rozdíl v „komercnej sfere“.

Lemming

U nás používáme Gerrit. Ale tedy nic moc, vyžaduje specifické zásahy do commit procesu. Předtím jsme používali Crucible, sice to bylo post-commit, ale používalo se to mnohem lépe.

Petr

Používáme phabricator na vlastním stroji a maximální spokojenost. Pro code-review tu je jiný postup než v případě githubu, je tu jiné názvosloví, možnosti nastavení jsou obrovské.. Rebase se dělá pouze lokálně a prostě vše funguje… Doporučuji min vyzkoušet, ať už na vlastním železe, nebo placený, nebo free verzi pro pět lidí.

https://www.phacility.com/phabricator/

honzikec

Pokud používáte JIRA, tak GitLab je asi dobrá volba, protože je taky od Atlassianu. Já s ním mám zkušenosti dobré, takže můžu s klidným srdcem doporučit…

Jarda

To se trochu pleteš, GitLab není od Atlassianu: https://about.gitlab.com/about/

Bitbucket je od Atlassianu, ale je to jedno, Jira umí pracovat se vším. Dřív jsem jí používal s Bitbucketem, teď s GitHubem a nevidím v té integraci žádný velký rozdíl.

Vít Kotačka

Já jsem docela spokojenej s RhodeCode. Mají i community verzi.

Luděk Rous

Pull request k libovolnému redaktorskému článku jsem doteď neviděl a přijde mi to jako skvělý nápad. Poslední dobou mi totiž přijde, že si články (sport, politika, … na různých serverech) po sobě nečte ani autor sám.

TonyVlcek

Kategorie nitpick mi přijde skvělá:
Mám zkušenost z jednoho projektu, který se sice nepodařil, ale spoustu věcí mě naučil. Tam jsme tuhle kategorii komentářů neměli, a když se na to dívám zpětně, tak by nám byla bývala opravdu pomohla. Na projektu jsme všichni dělali ve volném čas, a občas se nám PR zasekl třeba i déle než na týden, protože byl někde překlep v komentáři, mezi závorkami chyběla mezera, atp… Kdybychom si to označili zvlášť a potom jednou za čas udělali PR „Cosmetic fixes“ (nebo něco na ten způsob), tak věřím, že bychom se v projektu dostali dál. Mimo to se občas v záplavě těchto komentářů i ztratil důležitější komentář, který ve skutečnosti poukazoval na funkční nedostatek kódu.

Díky za zajímavý článek! :)

Enum a statická analýza kódu

Mám jednu univerzální radu pro začínající programátorty. V učení sice neexistují rychlé zkratky, ovšem tuhle radu můžete snadno začít používat a zrychlit tak tempo učení. Tou tajemnou ingrediencí je statická analýza kódu. Ukážeme si to na příkladu enum.

Pocta C64

Za prvopočátek své programátorské kariéry vděčím počítači Commodore 64. Tehdy jsem genialitu návrhu nemohl docenit. Dnes dokážu lehce nahlédnout pod pokličku. Chtěl bych se o to s vámi podělit a vzdát mu hold.