16 listopada 2015

Zasady dobrego „code review”

Zrzędzenia na tłumaczenia
Jako, że pojęcie code review pochodzi z czasów, kiedy programowaniem w naszym kraju zajmowali się nieliczni pasjonaci i wysokiej klasy specjaliści, do tego pochodzący z pokolenia, które dbało o swój rodzimy język, jest to nieliczne ze sformułowań z kręgu IT, dla których mamy swój rodzimy językowy odpowiednik, który nie razi – inspekcja kodu. I wszystko fajnie – tylko, że chyba nikt już o nim nie pamięta. Raz, że inspekcja to słowo niekoniecznie istniejące w słowniku współczesnych, znających więcej słów w języku angielskim, niż rodzimym. Dwa, że jednak kojarzy się negatywnie – z nadzorem, z kontrolą celem znalezienia uchybień. I z jednej strony – słusznie, bo wszak chodzi tutaj o dodatkowy poziom zabezpieczenia, upewnienia się, że wszystko zadziała „jak należy”. Z drugiej jednak, przy pracy zwinnej („agile”), przy odchodzeniu od hierarchicznych systemów zarządzania, nie do końca to oddaje ducha tej czynności, aż prosi się o prostsze – przegląd kodu. I tego sformułowania będę się trzymał w dalszym wywodzie, posiłkując się jednakże anglosaską nazwą jako synonimem.

Na czym zatem polega ta nieodzowna część pracy programistycznej? Jak wykonywać ją dobrze, na co zwracać uwagę, a czego unikać? Na te i inne pytania postaram się opowiedzieć w niniejszym artykule.

Cel przeglądu kodu

Przegląd kodu jest bezsprzecznie jednym z najskuteczniejszych i najlepszych sposobów na minimalizację błędów w tworzonym oprogramowaniu, a zatem w efekcie – zmniejszanie ryzyka jego niepoprawnego działania i zwiększanie satysfakcji klientów z jego użytkowania. Ale pamiętajmy, że nie chodzi wyłącznie o weryfikację poprawności kodu, jak i działania aplikacji, ale i ogólne zbadanie kodu – pod kątem jego jakości, architektury czy zgodności z wewnętrznymi, jak i ogólnie przyjętymi standardami. Współcześnie sporą część przeglądu, a konkretnie czarną robotę wykona za nas komputer – osoba sprawdzająca kodu, nie powinna robić za parser, jeżeli tylko stosujemy powszechnie uznane standardy (PSR, PEP-8 czy JSLint), powinien sprawdzić je edytor czy jakieś narzędzie, które zaprzęgniemy do tego typu działań.

Widziałem wiele różnego rodzaju badań, wykresów, liczb, które wskazywały, że przegląd kodu zmniejsza o 78,39% ryzyko wypuszczenia na produkcję niepoprawnie działającego kodu, że jest o 12 i ⅓ skuteczniejszy w wykrywaniu błędów, niż manualne testy, ale… no, bądźmy poważni i odłóżmy te wyliczenia tam, gdzie ich miejsce. Na bok.

Faktem jest, że dzięki co najmniej jednej dodatkowej parze oczu przeglądającej nasz kod, zyskujemy bardzo dużo. Po pierwsze: inne spojrzenie. Szczególnie jeśli wykonuje to osoba bezpośrednio niezaangażowana w wykonanie zadania czy projektu, będąca nieco z boku. To szansa na zwrócenie uwagi na inne aspekty, wykrycie rzeczy, które gdzieś nam umknęły czy w końcu asumpt do dyskusji na temat zasadności konkretnych rozwiązań.

Po drugie aspekty edukacyjne – różnej maści. Programista z niewielkim stażem (heh, aż się prosi o użycie lekko zmurszałego: terminator) ma szansę posłuchać uwag specjalisty, osoby z większym stażem, czy to pod kątem wiedzy na temat systemu, czy szerzej – jakości wytworzonego kodu. Ale także “senior” sporo może się wiele nauczyć, czytając kod młodszych kolegów czy koleżanek. Tak czy siak, zawsze następuje przepływ wiedzy i zawsze uczy się także osoba, która przegląd kodu wykonuje. Chociażby poprzez zapoznanie się ze zmianami, jakie zachodzą w systemie. W efekcie tego wszystkiego zwiększamy jakość i bezpieczeństwo kodu, zapewniamy przepływ wiedzy, minimalizujemy ilość późniejszych błędów i zmniejszamy ryzyko.

Mój kod jest doskonały

Quidquid latine dictum sit, altum videtur – cokolwiek powiemy po łacinie, brzmi mądrze, mówi prześmiewcza sentencja. Często podobnie jest z programistami i ich przekonaniu o doskonałości własnego kodu, nieprawdaż? Wszystko działa jak trzeba, po co ktoś ma jeszcze go przeglądać? Nie ma czasu na takie rzeczy, goni nas termin, musimy wdrażać. Nie jest to myślenie, które zaprowadzi nas specjalnie daleko.

Inna kwestia, to źle rozumiane poczucie dumy i zbytnie przywiązanie do swojego kodu. Och, nie muszę specjalnie głęboko sięgać pamięcią, aby przypomnieć sobie konfliktowe sytuacje związane z code review. I myślę, że nie jestem tym specjalnie odosobniony. Wpływa na to ma wiele czynników, głównie psychologicznych. Hierarchia w zespole czy dziale, poczucie wyższości, duma z przygotowanego rozwiązania, w które włożono sporo wysiłku, nadmierna pewność siebie czy przekonanie o swojej omnipotencji. Często przyczyną bywa też zła atmosfera w dziale, jakaś dziwna rywalizacja czy, co gorsza, wyraźny podział na tych lepszych i gorszych, czego emanacją są najczęściej podziały na tych dobrych i oświeconych – architektów i szarych roboli – programistów.

Często spotykana jest także mentalność Kali robić review – dobrze; Kalemu robić review – źle. Jeszcze gorzej, jeśli osoba, która wykonuje review poczuje przypływ władzy i postara się to wykorzystać. A władza kusi, oj kusi. Takie podejście, którego zdecydowanie należy unikać i zwalczać. Nie ma tu bowiem żadnej hierarchii, zasady nieomylny mistrzu, nauczaj czy przymykania oczy na pewne rozwiązania „po znajomości”

Przykłady można by mnożyć. Tak, przegląd kodu często bywa zarzewiem konfliktów. Rolą przełożonych (oraz np. Scrum Masterów) jest likwidować tego typu zachowania i konflikty i pracować nad wytworzeniem atmosfery wzajemnej współpracy. A każdy programista musi się pogodzić z tym, że błędy były, są i będą częścią pracy deweloperskiej. Dobry programista rozumie istotę przeglądu kodu i wie, że dzięki temu ma szansę na dodatkową naukę jak i zwiększa pewność, że jego rozwiązanie nie spowoduje katastrofy „na produkcji”.

Grupowo czy solo?

Jak powinno zostać przeprowadzone review kodu – czy powinno to być dedykowane spotkanie, w którym uczestniczą wszyscy, którzy ten kod wytwarzali i gdzie kod jest wyświetlany na większym ekranie/rzutniku i wspólnie omawiany? Czy raczej osoba review wykonująca powinna dostać „diffa” (czyli plik lub częściej – zestaw plików – obrazujących różnice pomiędzy wersją oryginalną kodu, a zmienioną) i samodzielnie przejrzeć kod?

Cóż, odpowiedź nie jest jednoznaczna.

Pracując jako programista bardzo lubiłem opcję numer jeden. Przede wszystkim ze względu na bezpośrednią interakcję z osobą wykonującą przegląd, możliwość natychmiastowego odniesienia się do uwag, wyjaśnienia i przedyskutowania konkretnych rozwiązań. Ale także i przyjrzenie się fachowcom przy pracy.

Jakiś czas później, pracując jako architekt oprogramowania, wykonując review – zdecydowanie wolałem opcję numer dwa. Dlaczego? Łatwiejsza koncentracja, możliwość skupienia swojej uwagi wyłącznie na kodzie, szybkiego zerknięcia jak coś działa w aplikacji „w praktyce”, czy zajrzenia do kodu.

Z review „grupowym”, zespołowym jest jeden zasadniczy problem. Trwa ono zdecydowanie dłużej niż „solowe” i angażuje znacznie więcej osób, a zatem znacznie więcej kosztuje. Jest też znacznie trudniejsze dla osoby, która je przeprowadza – raz pod katem koncentracji, dwa pod kątem – pewnej presji autorów kodu, trzy – przeglądający kod musi znakomicie orientować się w systemie, bowiem niejako oczekuje się od niego wiedzy dostępnej „na żądanie”. Szczególnie ciężki może być problem presji grupy, konieczność bycia asertywnym, odwaga zgłaszania uwag osobom starszym stażem czy kolegom. Zgodnie z maksymą: Niech będą sądzone czyny człowieka, a nie jego godność. Nie każdy się do tego nadaje. Ponadto, nie oszukujmy się, taki przegląd jest zdecydowanie mniej dokładny i w jego efekcie wykrywane jest znacznie mniej defektów.

Niezależnie od opcji, którą wybierzesz, nie można tracić możliwości wymiany opinii i dyskusji. W związku z tym, przy opcji review solowego konieczne jest zorganizowanie spotkania podsumowującego, gdzie uwagi zostaną omówione i przedyskutowane.

Krok po kroku

Kiedy oddawać kod do przejrzenia? Jak duża porcja kodu powinna podlegać ocenie? Uwaga, uwaga – to zależy.

Przyjąć powinniśmy jednak dwie generalne zasady:

  • im mniejsze porcje tym lepiej, ale zawsze konieczny jest ogląd całości, znajomość całego kodu, jego kontekstu, a nie tylko jego wycinka,
  • przy dużych projektach – bezwzględnie cyklicznie (co sprint?) powinno odbywać się review “cząstkowe”, a na jego koniec review odbiorcze. Dobrze, jeśli za każdym razem wykonuje to ta sama osoba, unikamy wtedy sytuacji wielokrotnego tłumaczenia tego samego.

Pamiętajmy wszakże, że przegląd jest tylko finalną czynnością, zamknięciem, zatwierdzeniem danego fragmentu kodu, natomiast sprawdzanie poprawności kodu powinno być naszą codzienną rutyną. Warto zadbać o odpowiednią kulturę wzajemnego czytania kodu w zespole, a przy bardziej skomplikowanych zadaniach korzystać z dobrodziejstw programowania w parach.

Od czego zacząć

Zanim przeglądający siądzie do działania, konieczne jest aby dobrze poznał materiał, na którym będzie pracował. O co zadbać?

  • Spotkanie z autorem – poznanie założeń – co trzeba było zrobić, dlaczego przyjęto takie rozwiązania. Trzeba rozumieć kod, który powstał.
  • Dowiedzenie się, gdzie można przetestować kod, a jeśli jest to potrzebne – na jakim branchu (w systemie kontroli wersji) powstał, dzięki czemu będzie możliwość spojrzenia na pełen kontekst kodu.
  • Dopilnowanie, aby przeglądany kod miał przynajmniej 50 linii kontekstu (czyli: 50 linii kodu nad i 50 linii kodu pod pod zmienionymi liniami) – to absolutne minimum, aby cokolwiek można było zaobserwować, uchwycić szerszy kontekst.
  • Zadbanie o dobre narzędzie pracy. Dostępnych jest mnóstwo fantastycznych narzędzi, zarówno tych darmowych, jak i komercyjnych, przykładowo: Gerrit, Rietveld, ReviewBoard, Collaborator, Phabricator, Crucible.

Zasady ogólne

Przeglądający kod bierze współodpowiedzialność za jego poprawne działanie – i powinien być tego świadomy. W związku z tym koniecznie musi znać dobrze system, projekt i być na bieżąco z tym, co się w nim dzieje. O czym jeszcze trzeba pamiętać?

  • Każda linijka jest ważna, każda powinna być sprawdzona.
  • Jeżeli review będzie przeprowadzane indywidualne, należy zadbać o ciszę i spokój, tak aby możliwa była właściwa koncentracja. Konieczne jest zatem odpowiednie miejsce i dedykowany czas, tak aby uniknąć jakichkolwiek zewnętrznych dystraktorów.
  • Przegląd kodu, szczególnie większych partii, jest czynnością bardzo absorbującą i, nie oszukujmy się, trudną, niezwykle męczącą. W związku z tym nie rezerwujmy danego dnia 6 godzin na przejrzenie kodu. Bo już w połowie tego czasu będzie pękać nam głowa i nawet jeśli zmusimy się do dalszych prac, to nijak nie będzie to wykonywane z należytą starannością i z odpowiednimi efektami. Maksymalny czas jednej ciągłej sesji nie powinien przekraczać 2 godzin. Jeśli chcesz kontynuować, zrób przynajmniej półgodzinną przerwę.
  • Konieczne jest pozytywne nastawienie. I oderwanie kodu od nastawienia do jego autorów. Należy szanować autora kodu i przyjąć, że starał się wykonać swoje zadanie najlepiej jak umiał.
  • Jeżeli masz do przejrzenia większą partię kodu, zrób to w iteracjach. Przykładowo, przejrzyj kod plik po pliku, a potem zrób to raz jeszcze, próbując mocniej uchwycić kontekst całości.

Zgłaszanie uwag

Zgłaszanie uwag to często miejsce, gdzie rodzi się najwięcej problemów interpersonalnych, co najczęściej wynika właśnie z wspomnianego wcześniej mylnego poczucia władzy i swojej wielkości.

De facto jednak, aby uniknąć konfliktów wystarczy zastosować elementarne zasady savoir-vivre’u i komunikacji międzyludzkiej.

Wykonujący review powinien pamiętać, że jego odbiorcą jest inny człowiek, autor tego kodu, któremu należy się szacunek. Oraz o tym, że on sam nie jest nieomylny i nie zawsze ma rację. Nie jest wstydem dopytać autora kodu z czego wynika to rozwiązanie, co miał tu na myśli.

Uwagi należy zapisywać w sposób kulturalny, unikając zbędnych emocji i nadmiernej krytyki. Ileż to razy widziałem naniesioną w kodzie filipikę na temat niezbyt lotnego umysłu autora kodu, po czym ten przychodził mówiąc, ale przecież w tym miejscu... i… w tym momencie autor tyrady nie wiedział gdzie się schować. Tak, jeśli chcemy zwrócić uwagę w ten sposób, mocno się nad tym zastanówmy.

Z drugiej strony, w przypadku choćby najmniejszych wątpliwości, zawsze dopytajmy autora kodu, czy jest pewien danego rozwiązania – chociażby wskazując przykładowy scenariusz użycia czy testu i upewniając się, że został on rozważony i zweryfikowany jako prawidłowy.

Jedną z najbardziej nielubianych postaw jest też, wynikające z błędnie rozumianej zasady marchewki, pisanie uwag typu: zastanów się, jak można zrobić to lepiej. W ten sposób możemy w ogóle nie przeprowadzać review, a całość zamknąć w komentarzu: przejrzeć cały kod jeszcze raz i zastanowić się co można tu ulepszyć.

Z kolei autor kodu powinien walczyć w to co wierzy, ale i z godnością przyjmować to, że nie zawszę ma rację, że istnieją rozwiązania lepsze. I pamiętać, że zasada TIMTOWTDI (There is more than one way to do it – do celu może prowadzić wiele dróg) nie obowiązuje tylko w Perlu (i nie jest tak, że w Pythonie jest zawsze tylko jedna ścieżka), ale… pracując w zespole obowiązują wypracowane w nim standardy.

Kolejność sprawdzania

Zgodziliśmy się przeprowadzić przegląd, wchodzimy do odpowiedniego programu, aby zapoznać się z kodem, a tam… litania plików i katalogów, masa zmian. I od czego tu zacząć? Sugeruję, aby zawsze rozpoczynać od wszystkich nowych klas/modułów, aby dobrze zapoznać się z wszystkimi dodanymi mechanizmami, potem przejść do najistotniejszych elementów aplikacji, w których zaszły zmiany, a potem już plik po pliku, blok po bloku. A dla złapania oddechu, co jakiś czas warto sięgnąć po rzeczy prostsze – pliki „słownikowe” czy, dajmy na to, zmiany w HTML i CSS. Trzeba tylko pamiętać, że „dla złapania oddechu”, nie oznacza przejrzenia „po macoszemu”. W tych miejscach też mogą istnieć błędy, które mogą nas drogo kosztować, a łatwo je sprawdzając podejść do nich ze zbytnim luzem. I jeszcze jedna pułapka – dokumentacja i komentarze. Zawsze najpierw czytaj kod, dopiero potem komentarz czy dokumentacje, upewniając się, że zawiera ona prawdziwe i aktualne informacje. Zawsze też myśl o środowisku produkcyjnym i możliwych scenariuszach użycia, szczególnie tych najbardziej niebezpiecznych.

Na co zwracać uwagę
Kolejność mamy ustaloną, teraz pytanie na co powinniśmy zwracać uwagę? Warto przygotować sobie w ramach organizacji jakąś listę, w której mniej lub bardziej szczegółowo wypiszemy, na co powinniśmy zwracać szczególnie uwagę, na co kłaść. Poniżej przykład takiej – mocno hasłowej – listy.

  • czy istnieje dokumentacja,
  • czy we wszystkich niejasnych miejscach znalazły się stosowne komentarze, napisane w odpowiedni sposób (nie “co” dany kod realizuje, tylko “dlaczego”),
  • czy są spełnione standardy kodowania,
  • czy jest spełniona zasada KISS (przyjmijmy rozwinięcie: Keep It Sophisticatedly Simple – upraszczaj ale nie w sposób prostacki) – szczególnie w kontekście czytelności kodu i nazewnictwa zmiennych (jeżeli siedzisz 20 minut nad jedną funkcją, coś tu nie gra… no chyba że zasnąłeś),
  • czy jest to robione, tak jak robimy to w innych miejscach dobrze,
  • czy są dopisane lub uaktualnione testy jednostkowe,
  • czy są poprawnie logowane błędy,
  • czy zadbano o prawidłowe importy modułów,
  • czy zadbano o prawidłową obsługę transakcji SQL (rollback!),
  • czy kod spełnia reguły SOLID (za Robertem C.Martinem, polecam wygoogle’ować),
  • czy kod spełnia regułę DRY (Don’t Repeat Yourself – nie powtarzaj się),
  • jaki jest wpływ zmian na istniejący kod
  • jaka jest generyczność metod/reużywalność kodu
  • jaka jest przyszłościowość rozwiązania, ale… pamiętaj o zasadzie YAGNI (You Ain’t Gonna Need It – nie będziesz tego potrzebował),
  • bezpieczeństwo,
  • bezpieczeństwo,
  • i jeszcze raz bezpieczeństwo,
  • czy zapewniono prawidłową obsługa błędów, a w sytuacji gdy błąd wystąpi, informacje od nim są odpowiednio odnotowywane w logach,
  • czy rozwiązanie spełnia odpowiednie kryteria wydajnościowe.

Mając taką listę, możemy w trakcie czy po przeglądzie rzucić na nią okiem, upewniając się, że sprawdziliśmy kod pod kątem najważniejszych jego aspektów.

Na koniec

Co powinno się zadziać po skończonym review? Przede wszystkim, o czym już wspomniałem wcześniej, należyta komunikacja z autorem (autorami kodu) i wspólne omówienie kodu i zgłoszonych uwag (no, chyba, że review odbywa się grupowo). Ale de facto na tym nie kończy się robota przeglądającego. W przypadku bowiem, gdy już na etapie dokonywania poprawek, po wgłębieniu się w kod, okaże się, że niektóre uwagi są niesłuszne, lub wręcz przeciwnie – nabrały nieoczekiwanego sensu – konieczne jest, aby dokonujący poprawek omówił to z przeglądającym wcześniej kod.

Oczywiście jeśli zgłoszonych błędów było dużo, lub wymagają one dużych zmian w samym kodzie, konieczne jest przeprowadzenie kolejnego przeglądu…

Na koniec… naprawdę

I to zasadniczo byłoby na tyle. Dogłębnie, w różnych wymiarach omówiliśmy cały proces przeglądu kodu. Czytelnik powinien traktować jednakże niniejszy artykuł wyłącznie jako zbiór wskazówek i spostrzeżeń, a nie jedynie słuszną wiedzę objawioną. W przypadku jakichś uwag, zapraszam do komentowania artykułu lub bezpośredniego kontaktu.

#technikalia#ZespółDeweloperski