Code review checklist

Nedávno jsem v práci prezentoval, jaké přínosné věci používáme na aktuálním projektu. Vyzkoušeli jsme si spoustu zajímavých nástrojů a praktik a v podstatě to byla taková laboratoř, kdy ty funkční záležitosti použijeme na dalším projektu. Mind mapa níže shrnuje přehled prezentovaných témat.

Jedním z nejcennějších realizovaných konceptů pro mne je, že se nám podařilo naimplementovat funkční a efektivní code review. (Doufám, že kolega Banter o tom brzy napíše článek.) A co čert nechtěl, po zmiňované prezentaci se nejvíc diskutovalo právě code review. Jedním z výstupů téhle diskuze je, že by bylo dobré mít nějaký code review checklist.

Já takový checklist nemám, protože ke code review přistupuju intuitivně (což ale neznamená, že nevím, co přesně chci, naopak). Nicméně pro potřeby diskuze jsem si sesumíroval, co by v takovém checklistu mohlo být.

Co je to code review?

Ač se pojem code review používá v oblasti softwarového inženýrství dosti zhusta, má celkem nejednoznačný obsah. Pro někoho to je výsledek nástrojů jako je SonarQube, PMD, FindBugs ad. Tyto nástroje řeší tzv. statickou analýzu kódu a jsou výbornými pomocníky při udržování kvalitního kódu.

Ale code review, tak jak ho chápu já, začíná tam, kde tyto nástroje končí. Prostě tam, kde stroje selhávají, či nestačí, přichází ke slovu “stará dobrá ruční práce”. Dalo by se to také nazvat jako asynchronní peer review.

Co je to code review checklist?

Checklist ((kontrolní) seznam bodů) slouží k tomu, abychom na něco nezapomněli. Třeba koupit chleba a mlíko cestou z práce. V případě code review jde o to, nezapomenout projít některý z aspektů, které chceme v rámci review kontrolovat.

Hlavní oblasti

Věci, které tak nějak intuitivně kontroluji při code review by se daly shrnout do těchto základní oblastí:

• Konvence
• Design
• Best-practices
• Závislosti
• Pokrytí testy

Konvence

Kdekoliv dochází k nějaké sociální interakci, jsou přítomny konvence. Buď již existují, nebo se začnou vytvářet. V dnešní době, kdy je vývoj software téměř vždy týmovou prací, je taková sociální (u některých programátorů a adminů spíše asociální) komunikace nevyhnutelná. Z hlediska code review, bych vypíchnul dva body, pro které je dobré konvence nastavit a dodržovat/kontrolovat:

• Formátování zdrojového kódu napomáhá jeho čitelnosti, pochopitelnosti, orientaci v něm atd. Tahle oblast se dá z větší části kontrolovat pomocí statické analýzy kódu (např. v Javě nástroj Checkstyle), ale některé věci zkrátka nejde nacpat do (automatických) pravidel. Domluvte se na nich, dodržujte je a váš reviewer vás bude mít rád ;-)
• Pojmenování. Věci by měly mít správná jména. Bude pak jasné, k čemu slouží, když se o nich budeme bavit, budeme více méně na stejné platformě a kdokoliv nový to lépe a rychleji vstřebá. Typicky, je dobré mít jmennou konvenci pro komponenty, balíčky, třídy, metody a proměnné. A cokoliv dalšího, co dává smysl a bude se vyskytovat ve více instancích.

Konvence jsou velmi rozsáhlé téma. A stejně jako u spousty dalších věcí, o kterých budu psát, dochází k jejich přesahu do jiných oblastí. Berme to rozřazení do základních kategorií jako velmi volné.

Design

Tohle je moje oblíbené téma, a tak zde budu mít nejvíc položek. Je to taky z toho důvodu, že kontrola designu je pro mne jedním z hlavních cílů code review. Kdybych si měl vybrat jenom jeden aspekt, který revidovat, byl by to jednoznačně design.

• Konceptuální diskuze. Důvod, proč často zamítnu reviewovaný kód je, že zavádí nějakou konceptuální změnu designu, která nebyla předem diskutovaná. Tohle má dvě složky. Jedna je subjektivní — mám určité designové preference a jelikož jsem většinou zodpovědný za architektonická rozhodnutí, tak je to moje právo a zodpovědnost. Druhá složka je týmová — pokud někdo “partizánsky” propašuje změnu, která bude ovlivňovat ostatní členy týmu, je to jasný důvod k zamítnutí. (Jen pro jistotu, partizánský zde má negativní konotace.) Obojí se dá jednoduše řešit zavedením designových review, kterých se účastní celý tým a kde se řeší design ještě před implementací.
• Testovatelnost. Nejsem TDD evangelista (v dnešní době?!), ale koncept a zkušenosti s unit testy mne jako vývojáře hluboce ovlivnily. Myslím si, že největší přínos a benefit unit testů je, že mají pozitivní vliv na design produkčního kódu. Kód, který je obtížně testovatelný, je prostě špatný.
• Konzistence. Systém/aplikace by měl být konzistentní napříč různými vrstvami, tj. odpovědnost jednotlivých vrstev/komponent, přístup ke zpracování výjimek, používané datové typy (třeba by pomohl kanonický datový model), přístup k logice (objektově, funkcionálně) atd.
• Znovupoužitelnost. Na úrovni knihoven, komponent, tříd, metod.
• SOLID. Systém/aplikace by měl respektovat dané/zvolené paradigma. V případě OOP by měl být “SOLIDní”. Takže: Single responsibility, Open-close, Liskov substitution, Interface segregation, Dependency inversion. A objektový. Atd.
• Logování by mělo být smysluplné, odpovídající a se správnou severitou a formátováním. Občas mě zaráží, jak málo vývojáři přemýšlí u logování nad tím, že aplikace poběží většinu svého životního cyklu na produkci.
• Vyvarovat se: duplicity, komplexity, zanořené logiky (cykly, podmínky), věcí napevno napsaných v kódu (hardcoded). A smrtelně nebezpečné choroby DIY.

Best-practices

Best-practices asi není úplně nejlepší název pro tuto kategorii. A určitě není vyčerpávající a jistě mi leccos propadlo sítem.

• Kód by měl být čitelný a srozumitelný. Čitelný znamená, že po něm “oko dobře plyne”, čemuž můžou napomoci konvence. A srozumitelný ve smyslu, že business logika by měla být jednoduše pochopitelná.
• Externalizace. Některé věci by v kódu neměly být vůbec: konfigurace, internacionalizace, to co patří do properties, řetězce literálů. Často je něco řešeno konstantama, místo použití enumů.
• Okomentovaný kód. Jestli je v kódu Javadoc se dá zkontrolovat statickou analýzou kódu. Jestli jsou ty komentáře aktuální, smysluplné a říkají to, co by měly, to už nám žádný nástroj neřekne. Pokud je kód čitelný a pochopitelný, mělo by v komentáři být popsaný hlavně výjimečné, či překvapující chování.
• Zakomentovaný kód. Jednoznačně vyhodit! Už nikdy se nepoužije a bude tam hnít roky.
• Neadresné TODO. Podobně jako zakomentovaný kód. Pokud mají vaši vývojáři potřebu si psát do kódu TODO, ať se tam aspoň podepíší. Stejně už se k tomu nejspíš nikdy nevrátí. Možná je to moje úchylka, ale nesnáším (měsíce, či roky staré) TODO v produkčním kódu.
• Komity do VCS by měly být malé, časté, smysluplné a měly by řešit pouze jedinou věc. A měly by mít rozumný komentář, ideálně nastavený konvencí. Když vidím komit/changeset, kde někdo opravil “půlku internetu”, otevírá se mi imaginární kudla v kapse.

Závislosti

Ve zkratce, měli bychom si dát pozor, co nám kdo do aplikace/systému zatáhne. To se týká hlavně externích knihoven, ale také interní závislostí mezi jednotlivými vrstvami a komponentami.

Není to tak dávno, co jsem si tuhle říkal “proč je ta (Java EE) aplikace tak veliká?". Vypíšu si strom závislostí a ona je tam přibalená půlka Springu?!? Uf.

Pokrytí testy

Přiznám se, jednou jsem dělal na aplikaci, která měla 96% pokrytí testy. Ale jinak, nejsem žádný fanatik přes testy. Nicméně “rozumné” a “dostatečné” pokrytí testy by aplikace měla mít. Zejména business logiky. Naopak, není potřeba testovat platformu, či frameworky.

A kde je ten checklist?

Jak jsem psal v úvodu, tento článek je zamyšlením, co by v code review checklistu být mohlo. Možná, kdybych přemýšlel dost dlouho, tak bych dal dohromady i nějaký reálný checklist. Ale nechci. Mám rád, když jsou nastavená nějaká pravidla, ale musí umožňovat dostatek volnosti. Aby se dalo dýchat, aby nepotlačovaly invenci a motivaci. Diskuze je daleko důležitější, než mít nějaký papír na odškrtávání.

Mind map