Code reviews v praxi

Máte ošklivý kód, kterému nikdo nerozumí? Máte problém se zaučováním nových členů týmu? A rádi byste to změnili? Jedním z řešení může být používání code reviews.

Máte ošklivý kód, kterému nikdo nerozumí? Máte problém se zaučováním nových členů týmu? A rádi byste to změnili? Jedním z řešení může být používání code reviews. V SUSE je pro část projektů používáme povinně přes dva roky a v tomto článku bych se rád podělil o postřehy a zkušenosti, které jsme za tu dobu nasbírali. Povíme si, co to code reviews jsou, proč je dělat, jak při nich postupovat, jak zvládnout komunikaci s nimi spojenou a jaké nástroje vám s nimi pomohou.

Článek vychází z přednášky (slajdy, video), se kterou jsme společně s Davidem Majdou vystoupili na několika konferencích a meetupech.

Co je to code review?

Code review je proces, který zajistí, že každou změnu v kódu uvidí další osoba a může ji připomínkovat. Tento process není závislý na tom, kdy a jak přesně se kód dá k připomínkování a jakým způsobem se zaznamenají komentáře ke kódu. Někdy stačí požádat kolegu v kanceláři, aby se na změnu podíval, jindy je vhodnější formálnější přístup, jako je třeba podepisování commitů.

Proč dělat code reviews?

V SUSE jsme začali s code reviews hlavně proto, aby se zlepšila kvalita kódu, zejména jeho čitelnost. Ukázalo se, že s tím code reviews opravdu pomáhají. Téměř každý se podvědomě snaží psát lepší kód, když ví, že se na něj podívá i někdo další a bude ho hodnotit. Připomínky často obsahují vylepšení, jak kód napsat jednodušeji, případně jak použít již existující funkce. Kód je tak méně duplicitní.

Ukázaly se také další výhody. Když přišel nový člen do týmu, tak při kontrole změn dostával hodně tipů na vylepšení a pomohlo to s rychlostí adaptace. Zlepšila se také zastupitelnost členů týmu. Když je někdo na dovolené, jeho zástupce je typicky ten, co mu dělal nejvíce reviews. Při zastupování řeší urgentní problémy a znalost změn, co proběhly, mu usnadňuje odhadnout, kde by mohl být problém.

V SUSE máme code reviews rádi. Pokud vy také, a k tomu chcete pracovat na zajímavých projektech, přidejte se k nám.

Postup při code review

Code review má typicky dva účastníky – autora změny a reviewera. Náčrtek níže ukazuje, jak funguje celý cyklus.

Na začátku autor udělá svoji změnu. Když je změna připravena, tak reviewer provede review. Výsledkem review a případné diskuze s autorem je buď předělání změny (v tom případě celý cyklus proběhne znovu) nebo je změna přijata (v tom případě se začlení).

image00

Pojďme se nyní podívat na to, jak code review vidí každý z účastníků a jaké problémy potřebuje vyřešit.

Pohled autora změny

Z pohledu autora jsou důležité tři otázky:

  • Jak poznat, že je změna připravena na review?
  • Jak si nechat zkontrolovat kód?
  • Kdy změnu začlenit?

Jak poznat, že je změna připravena na review?

Změna by měla být izolovaná, tedy měla by fungovat bez nutnosti dalších změn kódu v daném repozitáři. Testy musí procházet a i všechny potřebné další části, jako je dokumentace, by měly být hotové. Pokud změna není kompletně hotová, tak hrozí, že se nedodělá nikdy (jistě i váš kód obsahuje spoustu FIXME a TODO). V neposlední řadě by to měla být minimální změna, co implementuje požadovanou funkcionalitu nebo opravuje řešený problém. Velké změny se hůře kontrolují, snáze se něco přehlédne a efektivita code review klesá.

Praxe

Nejtěžší je udržet změny malé. Někdy se ale nelze vyhnout větší změně. V tom případě je rozumný kompromis dělat code review průběžně po menších kusech, i když změna není finální.

Jak si nechat zkontrolovat kód?

Přesný způsob, jak si nechat zkontrolovat připravenou změnu, závisí na konkrétním projektu. Obvyklý postup je úpravu nahrát na dohodnuté místo a buď někoho oslovit, nebo počkat, až někdo bude mít čas a review vám udělá bez ptaní sám.

Praxe

Mně se nejlépe osvědčilo dělit změny na spěchající a nespěchající. V prvním případě oslovím konkrétního člověka. Ve druhém případě se snažím ostatní nerušit. Časem si někdo změny všimne a podívá se na ni. Pokud se déle nic neděje, ve vhodný okamžik se připomenu. Tento postup je sice náročnější na uhlídání všech změn, ale poskytuje kompromis mezi vyrušováním a nutností začleňovat změny.

Kdy začlenit změny?

Hlavní rozhodnutí je, jestli změny začleňovat před nebo po review. Dále je dobré se v rámci projektu dohodnout, kdo začlení kód po úspěšné kontrole. Možnosti je několik:

  • autor změny
  • reviewer
  • osoba odpovědná za vydání nebo nasazení kódu

Praxe

V otázce, jestli začleňovat kód před nebo po review, praxe jednoznačně ukázala, že je lepší po review. Na začátku jsme review dělali až po začlenění, ale spousta námitek nebyla řešena a někdy ani k review nedošlo.

Co se odpovědnosti za začlenění týče, osvědčilo se mi začleňování osobou, která má odpovědnost za navazující změny. Pokud změna vyžaduje zvláštní kroky od administrátora serveru, tak se hodí, pokud dotyčný administrátor kód začlení ve chvíli, kdy je to pro něj vhodné i s ohledem na další úpravy a případné problémy (jistě není třeba říkat, co si o vás bude myslet správce serveru, pokud má padla a kvůli vaší změně v kódu se musí připojit, aby řešil případný výpadek). Pokud v projektu nebývají potřeba žádné navazující změny, potom na tom, kdo má odpovědnost za začlenění, příliš nezáleží.

Pohled reviewera

Teď se podíváme na druhou stranu. Dostali jste změnu a máte ji zkontrolovat.

První krok je jednoduchý – musíte kódu rozumět. Pokud nerozumíte tomu, co kód dělá, tak je chyba na straně autora a je to důvod pro přepracování.

Pro co nejefektivnější a zároveň konzistentní review je dobré mít svůj seznam věcí ke kontrole. Tento seznam by měl obsahovat kombinaci věcí, které jsou důležité pro kvalitu kódu, a dále specifické věci důležité pro daný projekt (např. zpětná kompatibilita, efektivita, nebo dokumentování změn).

Pro inspiraci tu popíšu svůj osobní seznam. Mám ho rozdělený na tři části podle úrovně detailů, kterými se v dané části zabývám:

  • přehled, kdy poměrně rychle vidím, jestli je problém s celou myšlenkou změny
  • jednotlivé části změny
  • detaily na úrovni jednotlivých řádků

Přehled

Testy

Při review obvykle začínám tím, jestli má změna testy. Pokud je nemá, tak si rozmyslím, jak náročné je danou část testovat a jestli to má smysl. Pokud by bylo testování náročné, tak to většinou značí problém v designu kódu.

Někdy samozřejmě nemá smysl testovat automaticky, protože opakovaný manuální test je rychlejší, než stále opravovat křehký test. Dobrý případ jsou změny v UI, případně změny ve wrapperu nad API, které se často mění.

Pokud testy jsou, tak se koukám, jestli opravdu testují, co mají, a jak jsou odolné proti budoucím změnám. Divili byste se, kolik testů závisí na konkrétním počítači, nebo jsou tak intenzivně mockovány, až ve skutečnosti nic netestují.

API

Další částí, co kontroluji, jsou změny v API, protože API je to, čím spolu jednotlivé komponenty komunikují a API dělá rozdíl mezi dobře čitelným kódem a shlukem volání podivných metod. Ideální je vidět API v akci. K tomu dobře poslouží testy, protože test je první fungující kód, co dané API používá. Pokud volání v testech vypadá divně nebo vyžaduje spoustu mockování, pak to značí, že API není příliš dobré. K dobrému návrhu API bylo napsáno mnoho, tak nebudu zabíhat do detailů.

Místo změny

S API souvisí i další bod – je změna dělána na správném místě? Pokud například potřebuji zajistit zpětnou kompatibilitu API, kdy se změnila vstupní data, tak je lepší udělat metodu, co převede stará data na novou strukturu, než mít všude v kódu podmínky na detekci staré struktury. Špatně umístěná změna zhoršuje udržovatelnost, ztěžuje budoucí změny a zhoršuje čitelnost kódu.

Izolovanost změny

Nakonec zkontroluji, jestli změna ovlivňuje i další části kódu a jestli se změnou nerozbijí. Kvůli tomu se hodí znát daný systém nebo přizvat k review i lidi, které změna může ovlivnit (pokud například píšete knihovnu, tak je ideální mít v záloze jednoho až dva uživatele dotyčné knihovny, kteří se můžou ke změně vyjádřit).

Části

Don’t repeat yourself (DRY)

První věc, co kontroluji, je, jestli jsem podobný kus kódu už neviděl. Pokud ano, pak je duplicitní a zhoršuje budoucí údržbu. Typické řešení je přesunout sdílenou část do metody, třídy nebo komponenty.

Rozšiřitelnost

Pokud změna rozšiřuje funkcionalitu, tak je dobré zkontrolovat, jestli usnadňuje nebo naopak ztěžuje případné další rozšíření. Příklad dobré změny je změna složitého switche na polymorfismus. Špatná změna naopak je onen switch rozšířit a přidat ještě další speciální podmínku pro novou funkcionalitu.

Boy scout rule

Při každé změně je dobré aplikovat pravidlo zvané boy scout rule. Toto pravidlo se nazývá podle amerických skautů. Když někde táboří, snaží se zanechat při odchodu místo ve stejném nebo lepším stavu, než bylo předtím. Aplikováno na kód to znamená, že pokud změna se týká kódu v mizerném stavu, neměla by ho zhoršovat, ale naopak ho aspoň lehce vylepšit. Často stačí lépe pojmenovat metodu, zavést lepší abstrakci, případně přidat unit test. Bez tohoto pravidla se kód postupem času dostane do stavu, kdy ho nebude možné udržovat a nezbude než ho kompletně přepsat.

Detaily

Ošetření chybových stavů

Zde zkoumám, jestli je kód správně, a jestli řeší chybové stavy. U jazyků bez výjimek to znamená zkontrolovat, zda jsou zachyceny všechny návratové hodnoty oznamující chyby. U výjimek je situace jednodušší, ale stále je třeba zapřemýšlet, kde přesně výjimky zachytávat a řešit.

Leakování

Z podobné kategorie jsou i leaky. U jazyků bez automatické správy paměti jde především o dealokování paměti. Ale i u jazyků, které automatickou správu mají, můžou leakovat prostředky jako otevřené soubory nebo síťová spojení.

Při kontrole změn kódu jsou tyto problémy poměrně jednoduše vidět, protože pokud např. přibyla alokace bez související dealokace, jde s velkou pravděpodobností o problém.

Coding conventions

Poslední věc, co kontroluji, je dodržování coding conventions. Někteří si možná říkají, že to není důležitá věc, ale já si dovolím nesouhlasit. První věc, kterou vidí člověk, který kód nezná, je úprava kódu. Pokud každý soubor vypadá úplně jinak nebo se dokonce v jednom souboru dělá jedna věc třemi různými způsoby, tak to zanechá špatný první dojem. Nadšení dělat něco s kódem tím dost utrpí. Nelze také nezmínit teorii rozbitého okna.

Jak komunikovat

Pro práci s kódem mají programátoři talent a stále se v ní zlepšují (ostatně proto čtete tento článek), ale v oblasti sociálních dovedností mají často mezery. U code reviews, kde z podstaty věci spolu komunikují alespoň dva vývojáři, tak často dochází k třenicím. Je proto podstatné, aby všichni účastníci věděli, co je cílem review –  důležité je výsledné dílo a ne si dokazovat, kdo je lepší.

Buďte konstruktivní

Hlavní zásada je jednat konstruktivně a ne konfliktně. Pokud budete používat konfliktní tón, je pravděpodobné, že se situace vyhrotí a místo konstruktivní diskuze povedete zákopovou válku, při níž ani o píď neuhnete.

Pokud nějakou část změny kritizujete, tak kromě důvodu proč je dobré i napsat, jak by daná věc šla udělat lépe. Těžko se diskutuje možné zlepšení, když pouze jedna strana dává návrhy a druhá jen kritizuje – neberte si příklad z našich politiků, kdy jedni tvoří, druzí jen kritizují, a pak se to vymění.

Nebuďte osobní a pátrejte

Vždy se bavte o kódu a nikdy neřešte schopnosti druhého. Cílem je společným úsilím dojít ke kvalitnímu kódu, ne se poměřovat. Dobré je také spíše se ptát než rovnou kritizovat. Je totiž velmi těžké znát všechny souvislosti a důvody, které vedly k napsaní daného kusu kódu, a je větší šance, že autor to ví lépe než vy. A i když jste si jisti, je lepší se zeptat, jaký je důvod této úpravy, než rovnou říct, že je zbytečná. Ono to dojde i druhému, a pak to bere částečně i za svůj nápad a hlavně nemá pocit, že je mu něco vytýkáno.

Nebojte se pochválit

Poslední tip a možná ten nejdůležitější je: nebojte se chválit a ocenit dobré myšlenky. Pokud autor elegantně vyřeší problém, použije vám neznámou funkci, která ale perfektně sedí k danému kódu a nebo je návrh dobrý, tak to napište a nenechte si to pro sebe. Pochvala nikdy neurazí.

Nástroje

Společně s rozšířením používání code reviews se zlepšují i nástroje, které usnadňují celý proces. Nejvýraznější krok v open source komunitě za poslední dobu jsou webová review na GitHubu. Změny je možné komentovat přes webové rozhraní a pak případně rovnou i začlenit. Existují i nástroje specializované přímo na code review, které nezávisí na zvoleném systému správy verzí nebo formě komunikace. V praxi jsem je nezkoušel, tak pouze odkážu na dva populární: Gerrit a Review Board.

Pokud používáte stejně jako SUSE GitHub, můžou se vám hodit některé nástroje, které se na něj dají navázat pomocí API. Nejznámější je Travis CI, který umí přímo do pull requestu doplnit informaci, jestli kód i po změně prochází testy na všech podporovaných platformách. To umožní zachytit neprocházející testy ještě před review. Na Travis je možné připojit například Coveralls, který automaticky do pull requestu přidává informaci o změně v pokrytí testy. Zajímavý projekt je i Code Climate, který pomocí jednoduché metriky měří, jestli patch zlepšuje nebo zhoršuje kvalitu kódu.

Často kladené otázky

Na našich přednáškách se často opakovaly některé dotazy, které bych tu rád zopakoval a zodpověděl. Je na nich mimochodem  vidět, že programátoři mají větší problémy se sociální částí než s tou technickou.

Jak přesvědčit šéfa/kolegy/podřízené

Nejčastější dotaz byl, jak přesvědčím své okolí, aby se reviews ve firmě zavedly. Nejlepší je použít argumenty uvedené v sekci Proč. Případně přidat argumenty o technickém dluhu, pokud není vyvíjený program jednorázový. Dále lze také najít dostatek referencí na úspěšné firmy (např. Google, Facebook, Microsoft nebo Adobe), které code reviews už používají, a tím ukázat, že to není pouze teorie, ale i praxí ozkoušený postup.

V případě kolegů či podřízených se osvědčila taktika rozděl a panuj, kdy se vyplatí získat na svoji stranu buď vůdčí osobnost týmu, nebo postupně získávat na svoji stranu progresivnější členy týmu s tím, že konzervativnější se postupně přidají.

Dlouho otevřené reviews

Z týmu, co již code reviews používá, se ozval dotaz, jak řešit “hnijící” pull requesty, kdy se nikomu nechce je kontrolovat. Tady pomůže změna politiky vyžádání review z pasivní na aktivní, kdy o review přímo někomu řeknete. Samozřejmě je možné i použít sociální tlak, který jsem například já aplikoval tak, že na týmový mailing list se automaticky posílá připomenutí, v kterých repozitářích jsou neaktivní pull requesty.

Závěr

Doufám, že vás článek přesvědčil, že code reviews není žádná černá magie. Některé z technik jste možná již sami používali a pokud začnete s code reviews, budete je “jen” používat systematicky a jednotně. V SUSE se nám zavedení code reviews tak osvědčilo, že někteří lidé si bez něj život neumí představit, a proto je používají i na soukromé projekty.

Snad jsem aspoň část z vás inspiroval k vyzkoušení.  A pokud code reviews nasadíte, případně již máte nasazené, budeme rádi, když se v komentářích s námi podělíte o své zkušenosti.

Review článku provedli David Majda a Martin Vidner.

Matfzyzák, horolezec, vodák a k nezastavení. Již přes šest let pracuje u SUSE na různých projektech a rukama i hlavou mu prošel kód od asembleru přes C++ a Perl až po Ruby. Příznivec dobré diskuze.

Zdroj: https://www.zdrojak.cz/?p=13483