neděle 1. května 2011

Stručný průvodce code review

Nejsem žádný velký příznivce code review, k tomu už jsem se kdysi dávno přiznal, na druhou stranu neznám jiný proces pro zachycení těch nejpalčivějších problémů, které by na vás vypadly dříve nebo později. Za těch pár let co jej dělám jsem si vypracoval seznam oblastí, na který se vždycky vyplatí podívat a pár pravidel, které se snažím dodržovat.

Code smells a OOP design

Tohle je obecné pravidlo, které vychází z toho, že kód, který je správně objektově navržen není tak náchylný k problémům. Speciální pozornost věnuji třídám, které vystupují v roli Centrálního Mozku Lidstva, bezpečně je poznáte podle toho, že mají hromadu procedurálního kódu. Návrhový vzor Business facade nebo Service facade neznamená, že do jedné třídy napěchujeme všechnu logiku, jak si bohužel mnoho programátoru myslí.

Když už mluvíme o smraďoších v kódu, tak se mnohdy stačí podívat na třídy, které mají největší velikost případně největší testy a nebo nemají testy vůbec. Povětšinou platí pravidlo, že kód, který se těžko testuje nebývá obvykle dobře navržený a jsou v něm problémy.

Osobně při code review koukám také na to jestli je kód dostatečně defenzivní. Co všechno musí být ještě public, proč něco není final. Všechno co smrdí duplicitou musí okamžitě pryč. Stejně tak jakýkoliv zakomentovaný kód, pokud je potřeba nějaká ukázka šup s ní do javadocu. Když tu mluvim o kódu mám na mysli i všechnu infrastrukturu okolo tj. Maven, Spring atd.

Synchronizace a bezpečnost z hlediska vláken

Nejproblematičtější částí co do možných problémů představuje kód, který je špatně proveden z hlediska paměťové konzistence při vícevláknovém přístupu. Pokud dneska programujete jakoukoliv webovou aplikaci případně obsluhu messagingu tak jste tomuto problému vystaveni aniž byste si to možná uvědomovali.

Programátoři si dobře uvědomují problém vzájemného vyloučení vláken pomocí zámků, ale problémem je uvědomit si, že synchronizační primitiva zároveň zaručují i viditelnost dat mezi vlákny. To je problém pokud si objekty předáváte mezi různými kontexty, které jsou obsluhovány různými vlákny. Na tohle konto zbývá dodat: a bude hůř například díky Servlet 3.0 a jeho podpoře asynchronicity.

Zaměřte se proto na objekty, které drží nějaký stav. Důležité je také zkontrolovat chování cizích knihoven, které používáte. Jestli byly navržené na vícevláknové použití, které odpovídá vašemu setupu. Obecně řečeno, nejméně problémů si způsobíte pokud budete programovat bezstavové služby s tím, že stav budete předávat v imutabilních objektech.

Zámky/Deadlocky způsobéné voláním sebe sama a jiné špinavosti

Hodně problémů bývá v různých typech zámků ať již paměťových a nebo v databází. První věcí je ověření, že zámek vůbec může fungovat. Hodně často je zámek dobře použitelný v lokálním provedení, v jedné běžící instanci JVM. Pokud máte cluster deployment je nutné si ověřit, že zámek bude fungovat i v clusteru.

Pokud už potřebujete zámek, který ma lídově řečeno fakčit v clusteru, tak bude pravděpodobně navržen nad databází. Zde je potřeba pečlivě projít uvolňování zámku, nejlépe na příkladu spadlé instance JVM. Mimochodem když už mluvíme o zámcích podobnou sortu problémů představuje obecně kód, který nějakým způsobem závisí na pořadí inicializace. Opět věc, která může dělat problémy v clusteru.

Samostatnou kapitolou je využívání zdrojů, které jsou poskytované z poolu například databázová připojení, HTTP vlákna atd. Zde je potřeba zkontrolovat, že v rámci jednoho volání nedochází k alokaci více než jednoho zdroje. Pokud ano, dříve nebo později vás s velkou pravděpodobností čeká deadlock. K tomu stačí, aby byl počet vláken přistupujících k poolu stejný nebo vyšší jako počet zdrojů v poolu.

Uvolňování zdrojů a jejich alokace

Pokud alokujete zdroje, které je explicitně potřeba vrátit zpět. jako je paměť, file descriptory atd. zkontrolujte, že se tak děje v try/finally blocích. Opravdu záludným problémem bývá vícenásobné přiřazení do proměnné, která ukazuje na zdroj, který je potřeba vrátit. Docela dobrým pravidlem bývá zkontrolovat, že se používají final proměnné. To tento typ problému minimalizuje.

Při alokaci zdrojů je potřeba prověřit jakým způsobem dochází k jejích uvolnění. Pozor je potřeba dávat na vše vázané nějakým způsobem na transakce. Pokud může nastat situace, že budeme mít dlouhotrvající transakci je potřeba ověřit, jakým způsobem a jestli vůbec se po dobu jejího běhu uvolňují alokované zdroje. Vedlejším efektem transakcí totiž bývá alokace paměti či zamykání dalších zdrojů (řádky, tabulky databáze).

Pár postřehů

Představa, že během code review odhalíte všechny problémy, které tam někdo nasekal za N dní prace je nesmyslná. Code review by nám mělo pomoci odstranit ty největší boty, všechno ostatní je nad plán. Pokud problémy nehodláte odstranit hned a nebo na ně nevznikne nějaký defekt, ke kterému se později vrátíte, tak code review nemá smysl snad kromě sdílení vědomostí o kódu.

Zaměřte se na další detaily jako: jak dlouho trvá exekuce testů, o kolik se prodlouží start aplikačního serveru, o kolik víc paměti kód sežere, jak dlouho se to bude buildovat. To jsou všechno otázky, které vám během code review pomohou udělat si celkový obrázek o tom co máte před sebou.