20. července 2014

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.

Pozitivní věci na projektu (code review je fialový)

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.

Zdroj: Dilbert.com

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



4 komentáře:

  1. Ten požadavek na checklist je pitomost ... a pouze to o čemsi svědčí.

    To, co Ty jsi sepsal, je spíš takovej Programming Guide, kterej by se hodil (když už Tě donutili to sepsat;-)) jako základ pro určitou edukaci (resp. nastavení pravidel - viz konvence) toho, jak psát "dobrej kód".

    A s tím, jak se u dotyčných zvýší jejich programátorský schopnosti, tak se u nich i přirozeně vyvine/zvýší jejich schopnost poznat dobrej/špatnej kód a dobrý/nevhodný řešení.

    M.

    OdpovědětVymazat
    Odpovědi
    1. No, já bych nebyl takovej příkrej, s tím soudem. Záleží na kontextu - pokud chci nastavit nějakou vývojářskou kulturu mezi cca 50 lidma, tak se bez nějakých více, či méně formálních pravidel neobejdu. To je náš případ - kód review se dělá, ale každý to dělá nějak jinak. Sice svoboda, ale má to velkou režii.

      A jestli tomu říkám Programming Guide nebo code review checklist je jedno, jsou to dvě strany téže mince - pro vývojáře guide, pro reviewera checklist. Podstatný je, když už něco takového použiju, že si tam vyberu věci, které jsou pro mne fundamentální, checklist/guide na čtyři stránky je k ničemu.

      Vymazat
    2. Možná jsme si jen špatně rozuměli:-)

      Já jsem to pochopil tak, že publikum té prezentace Tě zaúkolovalo: "tak nám na to udělej checklist (kuchařku)":-)

      To s těma pravidlama pro jednotnou vývojářskou kulturu - to je jasný, v tom se shodnem. Já měl na mysli spíš to, že když se code review smrskne na mechanickou kontrolu nějakýho checklistu dodanýho "shora" - bez zapojení vlastního rozumu, tak je to cesta do pekel. To už se to pak může rovnou nahradit nějakým nástrojem na statickou analýzu kódu (kterej bude ve finále i rychlejší a spolehlivější;-)).

      To je ale jen názor, výsledek ukáže až praxe:-)

      M.

      Vymazat
  2. I'm glad to be reading this article, I simply want to offer you a huge thumbs up for your great information.
    Tableau Guru
    www.sqiar.com

    OdpovědětVymazat