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



7. července 2014

Jak dělám Java pohovor II: proč nedávám testy?

Image courtesy of Michal Marcol
FreeDigitalPhotos.net
Je to už nějaký pátek, co jsem napsal (úspěšný) článek Jak dělám Java pohovor. Byl to pro mne výsledný stav určitého vývoje a shrnutí zkušeností z vedení technických (převážně Java) pohovorů, kterých jsem měl tehdy za sebou pár desítek.

Hned od počátku jsem měl štěstí, že mi nikdo nemluvil do toho, jak má interview vypadat. A jsem za tu důvěru vděčný. Taková svoboda mi vyhovuje, takže jsem si jednotlivé kroky pohovoru sestavil a vymyslel podle sebe.

Není to úplně jednoduchá věc, začít takhle z ničeho - není na to žádný mustr, informací je pomálu, není se moc koho zeptat, protože HR vám nejspíš neporadí a ten kdo dělal technický pohovory před váma, už nejspíš ve firmě nepracuje.

Proč nedávám testy

Jednu věc jsem věděl jistě - nechci používat žádné testy. Je s podivem, jak moc jsou testy na pohovorech rozšířené. Protože když uvážíme, jak malou mají, v daném kontextu, vypovídací hodnotu (osobně bych dokonce řekl mizivou) a jak velkou vyžadují režii na údržbu, aby aspoň k něčemu byly; člověk by řekl, že to za to nestojí.

Trochu to chápu. Když už jednou někdo ty testy vytvoří, tak je může dát kandidátovi klidně slečna z HR. On už to pak někdo vyhodnotí. Takže na první pohled se to zdá jako jednoduchý, efektivní a objektivní způsob ohodnocení kandidáta. Ani jedno z toho není pravda.

V první řadě, testy testují něco, na co kandidát téměř jistě nebyl připravený. Je téměř jisté, že ten obskurní syntaktický příklad, který v testu máte, nikdy v životě v praxi nepotkal. Ať už vám ta "chytrá" knížka, nebo internet, podle kterých jste to sestavili, tvrdí cokoliv. A nejde jen o to, že jsou příklady vycucané z prstu, problém je, že se na ně nedá nijak připravit - když si chci udělat certifikaci, vím, co se potřebuju naučit, jaký bude rozsah, co se dá použít ke studiu apod. Když jdu na pohovor, nevím nic - dostanu (nejspíš nekvalitní) test a záleží jen na štěstí, nakolik se moje zkušenost z obrovského Java kontinentu kryje s tématy v testu.

Šíře Java landscape je další problém. Co chcete vměstnat do rozumné délky testu? Dejme tomu, že by test měl trvat hodinu, předpokládaný čas na otázku je tři minuty (takže kandidát nad tím nebude moc přemýšlet a jen vysype z hlavy, na co si vzpomene), což vychází na 20 otázek. Kolik otázek věnujete Java SE? Kolik Java EE, kolik Springu? A co něco souvisejícího, třeba databáze, nebo skriptování? Taky máte dojem, že to jen škrábete po povrchu?

Možná si říkáte, zaměříme se jen na technologie, které používáme. OK, používáme na projektech Spring, tak budou testy o Springu. Právě jste se připravili o polovinu schopných lidí, kteří by u vás mohli pracovat. Na druhou stranu, třeba to tak chcete - jednodruhové, vyšlechtěné, single-malt vývojáře.

Dalším problémem testů je jejich aktuálnost. Jak často vychází major verze nějakého frameworku/platformy? Když vezmu v potaz Java EE (verze 5 až 7), tak za poslední čtyři roky bych takový test musel předělat dvakrát až třikrát (beru v úvahu, že reálné využití se nekryje s vydáním specifikace). Kdybych řešil jen Spring (verze 2.5-4.0), jsem na tom podobně.

A zmíním ještě jednu věc. Máte uni-sex testy "one-size-fits-all", které dáváte všem, bez ohledu na úroveň seniority? Máte pro každou senioritu zvláštní test? Jestli chcete dělat testy v rozumné kvalitě, budete s tím mít dost práce.

Co nějaká výjimka?

V tom, proč nedávat testy bych se mohl pitvat ještě dlouho, ale myslím, že pár zdatných (a dostačujících) důvodů jsem uvedl. Na druhou stranu, přeci jenom, nenašel by se nějaký případ, kdy testy u pohovoru dávají smysl? Přiznám se, nedávno jsem testy taky jednou použil. Či spíše přesněji: akceptoval je a podílel se na nich.

Byl to ale velmi specifický případ. Měl jsem možnost podílet se na nabírání Java vývojářů na Filipínách. Nabírat vývojáře na druhé straně světa není úplně jednoduché. Jádrem přijímacího procesu byl osobní pohovor, kdy za stranu zaměstnavatele se na něm podílely osoby z Česka (já), Francie a USA. Když už si to konečně naplánujete interně, že se sejdete na druhé straně glóbu, je podstatný, aby vám na pohor přišli už jenom relevantní lidé.

První filtrování udělalo místní HR. Jako druhý filtr jsem navrhoval klasický phone screen, ale vzhledem k časovému posunu (Česko-Filipíny +7 hodin) jsme se nakonec shodli právě na testech - hlavně z důvodu jejich asynchronicity. Napsal jsem tedy (s vydatnou a převážnou pomocí kolegy Bantera) test, který by byl takovým "rozumným" filtrem. Opravdu jenom filtrem, nic víc - pokud někdo neprošel testem, nemělo smysl se s ním vidět osobně.

Na tomto testu jsou podstatné tři věci, které ospravedlňují jeho existenci: kontext, účel a trvanlivost. Kontext je, myslím, jasný - pohovory na jiném kontinentu zkrátka normálně nedělám. Účel jsem už také zmínil, šlo o pouhý filtr, při celkovém hodnocení kandidáta jsem k testu nijak nepřihlížel. No a "trvanlivost" - šlo o jednorázovou záležitost, ten test jsem od té doby neviděl, vidět nechci a kdyby náhodou, bylo potřeba řešit najímání vývojářů v Jižní Americe ;-)  začal bych ze stejné pozice: testy raději ne.

Výzva

Jestli děláte pohovory a dáváte na nich testy - zkuste se nad těmi testy zamyslet. Dávájí vám to, co od nich očekáváte? Je nějaká jiná cesta, jak dosáhnout stejného výsledku? Troufám si tvrdit, že bez testů se vám podaří najmout kvalitnější lidi. Zkuste to, jen tak se posunete dál.

Mind Map



Související články