iTechArt logo

Code review zgodny z wszystkimi zasadami: checklista od programistów iTechArt

Development & QA

Co się stanie, jeśli przypadkowo lub celowo zapomnimy o code review? Czy możemy powierzyć to zadanie komuś innemu i czy istnieje uniwersalny, skuteczny algorytm do sprawdzania kodu? Ilja Gumbar i Władisław Naruckij, Software Engineers w firmie iTechArt, doskonale znają odpowiedzi na te pytania.

NASI ROZMÓWCY
ilya.gumbar.png

Ilja Gumbar

Software Engineer w firmie iTechArt

Jego zdaniem code review pomaga obniżyć koszty tworzenia dobrej jakości aplikacji, a przecież na tym zależy klientom.

uladzislau.narutsky.png

Władisław Naruckij

Software Engineer w firmie iTechArt

Uważa, że code review pozwala zadbać o to, aby kod przekazywał intencje programisty i był zrozumiały nawet dla kogoś, kto nie śledzi na bieżąco danego zagadnienia.

Jak ważny jest code review? Co się stanie, jeśli go pominiemy?

ILJA GUMBAR: Nie ma wątpliwości, że code review jest ważnym elementem pracy zespołowej. Jeśli procesy są skuteczne, pozwala on:

  • kontrolować dług techniczny projektu;
  • zwiększać czytelność i łatwość utrzymania kodu;
  • ograniczać liczbę usterek/błędów;
  • promować dzielenie się wiedzą.

W rezultacie code review może pomóc w obniżeniu kosztów tworzenia dobrej jakości aplikacji, co przecież bezpośrednio leży w interesie klienta.

Przegląd kodu powinien być obowiązkowym krokiem nawet w przypadku awaryjnych poprawek. W dłuższej perspektywie zignorowanie tego etapu prowadzi do wydłużenia czasu realizacji projektów, a co za tym idzie — do wzrostu kosztów.

WŁADISŁAW NARUCKIJ: Myślę, że etap code review jest ważny i powinno się go traktować w odpowiedzialny sposób. Jeśli zostanie pominięty lub jeśli nie będzie traktowany z należytą uwagą, może pociągnąć za sobą koszty w postaci dodatkowego czasu pracy, których bezwzględna wartość będzie zależeć od różnych czynników (takich jak szybkość budowania i wdrażania produktu w różnych środowiskach). Z własnego doświadczenia mogę powiedzieć, że jeszcze na etapie code review nierzadko udaje mi się wykryć różnego rodzaju błędy — np. błędy w logice biznesowej czy nieprawidłową obsługę parametrów wejścia/wyjścia funkcji. Oczywiście te błędy tak czy inaczej zostałyby później wykryte przez inżynierów ds. QA, ale, jak wspomniałem wcześniej, pociągnęłoby to za sobą dodatkowe koszty. Programista musiałby ponownie uruchomić projekt, naprawić go, potem (być może) przeprowadzić kolejny code review, a na koniec ponownie przekazać gotowy kod do środowiska QA. A to wszystko wymaga czasu, który można zaoszczędzić dzięki dokładnemu przeglądowi.

Code review ma też inny, równie ważny cel, jakim jest utrzymanie „czystego” kodu i architektury. Nie będę się tu rozwodzić nad samą definicją „czystości” — na ten temat powstało już wiele książek (np. „Czysty kod” i „Czysta architektura” R. Martina) — ale powiem jedną, być może oczywistą rzecz: poprzez swój kod programiści tak naprawdę nie pracują z maszynami, tylko z ludźmi. To właśnie dlatego podczas code review upewniam się też, czy kod komunikuje intencje programisty i czy jest zrozumiały nawet dla kogoś, kto nie zna dogłębnie kontekstu. Istnieje bowiem duże prawdopodobieństwo, że w przyszłości ten kod będą musiały utrzymywać inne osoby. A jeśli nie będzie on łatwy do zrozumienia, po raz kolejny poniesiemy koszty czasowe.

Jak dużo pracy wymaga code review? Ile czasu trzeba na niego poświęcić?

ILJA GUMBAR: Wszystko zależy od projektu i wprowadzanych zmian. Z reguły większość zespołów stara się kontrolować wielkość poprawek, przestrzegając pewnych zasad:

  • PR (pull request) musi zawierać zmiany, które dotyczą tylko jednego przypadku użycia;
  • PR nie powinien zawierać jednocześnie np. nowej funkcjonalności i refaktoryzacji.

Jakość weryfikacji jest odwrotnie proporcjonalna do wielkości sprawdzanych zmian. Oznacza to, że im większy PR, tym większa szansa, że coś przeoczymy. W związku z tym czas poświęcony na code review podlega kontroli zespołu, ale zazwyczaj nie przekracza on 30 minut.

WŁADISŁAW NARUCKIJ: Moim zdaniem jest to dość wymagający proces, ponieważ jednym z wyzwań jest zrozumienie toku myślenia drugiej osoby. Zdarzają się sytuacje, kiedy wiesz już dokładnie, jak rozwiązać problem, a potem, na etapie sprawdzania, widzisz zupełnie inne rezultaty. W takich momentach ważne jest, aby nie ulec pokusie stwierdzenia, że każda decyzja, która nie pokrywa się z naszą własną, jest zła. Musimy wziąć pod uwagę alternatywne podejście i zrozumieć je — być może okaże się lepsze. Może to być trudne, ponieważ obraz właściwej decyzji powstał już w naszej głowie.

Kolejną trudnością może być złożoność zadania, które mamy wykonać. Podczas implementacji złożonej logiki biznesowej lub przepływu biznesowego zdarza się, że wpływają one na wiele plików w różnych częściach aplikacji. Sprostanie takiemu wyzwaniu może być trudniejsze i będzie wymagało poświęcenia więcej czasu na code review. Musimy też zwrócić uwagę na to, czy zmiana wpływa na jakiekolwiek elementy wielofunkcyjne. Jeśli tak, trzeba ocenić, czy wszystko działa prawidłowo.

Gdybym miał wskazać konkretną szacunkową ilość czasu, powiedziałbym, że code review może zająć od 5–10 minut do 1–2 godzin — zależnie od wielkości i złożoności zadania.

Komu powierzać code review?

ILJA GUMBAR: W różnych zespołach stosuje się różne strategie. Jakość i liczba oceniających może się też różnić w zależności od obszaru, w którym są wprowadzane zmiany. Zwykle w zatwierdzeniu zmian biorą udział co najmniej dwie osoby: Tech Lead i programista, który był w jakimś stopniu zaangażowany w daną funkcjonalność.

Jeśli chodzi o dzielenie się wiedzą, to im więcej osób przegląda PR-y, tym lepiej. Dlatego jeśli PR wprowadza zmiany w rdzeniu projektu lub gdy rozszerzana jest biblioteka współużytkowanych komponentów, warto informować o tym cały zespół (lub choćby jego część).

WŁADISŁAW NARUCKIJ: Myślę, że code review można powierzyć każdemu odpowiednio doświadczonemu członkowi zespołu, który jest dobrze zaznajomiony z tematem i wymaganiami biznesowymi produktu, a także architekturą projektu lub systemu. Taka osoba powinna też umieć analizować różne podejścia i rozwiązania, a także oceniać ich wady i zalety, aby wybierać najprostsze i najlepsze z możliwych.

Autor kodu a recenzent — jakie obowiązki ma każda z tych osób?

WŁADISŁAW NARUCKIJ: Moim zdaniem obowiązki rozkładają się mniej więcej następująco. Autor kodu musi znaleźć najprostsze i najlepsze w danej sytuacji rozwiązanie, dbając przy tym o „czystość” kodu (o czym była już mowa wcześniej). Rolą recenzenta jest z kolei potwierdzenie, że autor wykonał te zadania. Jeśli nie, recenzent powinien przekazać mu konstruktywną informację zwrotną i zasugerować alternatywne rozwiązanie. Do obowiązków recenzenta należy również przeanalizowanie tego, czy wszystkie przypadki zostały rzeczywiście wzięte pod uwagę. Oznacza to więc nie tylko sprawdzenie, czy napisany kod jest poprawny, ale także upewnienie się, czy nie jest potrzebny żaden dodatkowy kod.

ILJA GUMBAR: Obie te osoby powinny być przyjazne, uprzejme i szanować się nawzajem w komunikacji. Musimy też pamiętać, że mają one ten sam cel, jakim jest usprawnienie danego aspektu systemu.

Obowiązki autora kodu:

  • przygotowanie PR-ów do przeglądu i upewnienie się, że są one zgodne z przyjętymi zasadami (PR powinien być mały, odnosić się do wykonywanego zadania, zawierać opis wprowadzanych zmian i stosowanego podejścia oraz testy zmian, a styl kodu powinien być zgodny ze standardami itp.);
  • przeanalizowanie każdego komentarza i udzielenie odpowiedzi: zdarzają się sytuacje, w których pewnych komentarzy nie można uwzględnić natychmiast (naprawa awaryjna); w takim przypadku autor jest zobowiązany do uwzględnienia tego w długu technicznym, aby skorygować problem, gdy sytuacja pozwoli na dokonanie zmian;
  • wprowadzenie wymaganych zmian lub uzasadnienie, dlaczego dany komentarz nie może zostać uwzględniony.

Obowiązki recenzenta:

  • dokładna analiza proponowanych zmian według następujących kryteriów: architektura, funkcjonalność, złożoność, testy, styl kodu, najlepsze praktyki itp.;
  • uruchomienie przez recenzenta zmian na swojej maszynie (ostatni etap) i zweryfikowanie ich zgodności z założeniami;
  • pozostawienie listy komentarzy opartych na analizie zmian — istotne jest, aby zmiany faktycznie przyczyniły się do poprawy jakości kodu w jakimś aspekcie.

Tam, gdzie istnieją ugruntowane procesy, jesteśmy w stanie unikać konfliktów. A jeśli już do nich dojdzie, sposoby ich rozwiązywania są dokładnie uregulowane.

Jakich usług można użyć podczas code review? Jakie są ich największe zalety i wady?

ILJA GUMBAR: Z reguły większość usług zarządzania projektami ma pewne wbudowane funkcje code review. Nie miałem okazji przeprowadzić analizy porównawczej każdej z nich, ale mogę wskazać funkcjonalność, która w taki czy inny sposób musi być wdrożona, aby zapewnić maksymalną wygodę:

  1. komunikacja z backlogiem;
  2. komunikacja z systemem CI/CD;
  3. komunikacja z systemem dokumentacji/wiki;
  4. możliwość komentowania linii, bloku lub całego PR;
  5. funkcja tworzenia zadań na podstawie komentarzy i łączenia ich z PR-ami;
  6. możliwość proponowania zmian (jako diff);
  7. opcja śledzenia i monitorowania statusu edycji;
  8. opcja automatycznego przypisywania recenzentów za pomocą elastycznego systemu reguł;
  9. obsługa zaawansowanej składni projektowania PR i komentarzy (odniesienia, obrazy, listy kontrolne itp.).

Przykłady: GitHub, Azure DevOps, Atlassian.

WŁADISŁAW NARUCKIJ: Najczęściej przeprowadzałem code review w BitBucket i GitHub, dlatego mogę porównać te dwie usługi i zwrócić uwagę na kilka rzeczy:

  • BitBucket jest prostszy w użyciu, ponieważ wyświetla strukturę plików i folderów, co ułatwia nawigację w porównaniu z GitHub;
  • GitHub pozwala z kolei na łatwiejsze komentowanie i podświetlanie składni, co upraszcza cały proces.

Słyszałem też, że niektóre narzędzia Git (a dokładniej klienty desktopowe — np. GitKraken) obsługują przeglądanie PR-ów bezpośrednio z poziomu aplikacji. Myślę, że to wygodniejsze niż code review w przeglądarce, ale do tej pory nie miałem okazji osobiście ich wypróbować.

Google ma swój własny standard poprawnego code review. Stosujecie go w swojej pracy?

ILJA GUMBAR: Nasze obecne procesy pod wieloma względami pokrywają się z tymi opisanymi w Google’s Engineering Practices. Stosujemy je, ale z uwzględnieniem specyfiki naszych zadań.

WŁADISŁAW NARUCKIJ: Niektóre z reguł Google są nieodłącznie związane z moją pracą. Staram się na przykład unikać osobistych preferencji w pewnych aspektach i zawsze omawiam z programistami szczegóły, których nie rozumiem — tak, aby zagłębić się w tok myślenia autora kodu. Dbam też o to, aby code styles były przestrzegane i sam się ich trzymam.

Jakie są wasze osobiste praktyki związane code review?

WŁADISŁAW NARUCKIJ: Moja praktyka obejmuje zwykle następujące kroki:

  1. Zatrzymaj się i pomyśl/przypomnij sobie, czego dotyczy pull request i jakich zmian od niego oczekujesz. To ułatwia rozpoczęcie pracy nad nowym zadaniem.
  2. Przejrzyj zmiany w plikach, które zawierają wielofunkcyjne rozwiązania. Jednocześnie należy ocenić, czy zmiany te mogą ujawnić się w innym miejscu lub coś zepsuć.
  3. Przejrzyj zmiany w plikach specyficznych dla danego zadania.
  4. Pobierz pull request, uruchom i przetestuj podstawową funkcjonalność oraz konkretne przypadki.
  5. W zależności od wyników uzyskanych w powyższych punktach możliwe są następujące rezultaty:
    • jeśli są wymagane poprawki, przekazuję informację zwrotną (zostawiając komentarz w pull request lub dzwoniąc do programisty i omawiając to wspólnie z nim) i proszę o wprowadzenie zmian — następnie code review jest powtarzany, ale sprawdzane są tylko te pliki, które uległy zmianie;
    • jeśli wszystko jest w porządku lub potrzebne są tylko bardzo drobne zmiany kosmetyczne (które mogę wprowadzić w 1–2 minuty), zamykam pull request i kończę code review.

ILJA GUMBAR: Oto lista kontrolna, z której sam korzystam:

Kontrola zgodności ze standardami

  • Czy zmian jest dużo i czy może to utrudnić weryfikację?
    • Czy można je podzielić na części? Czy byłoby lepiej, gdyby były to dwa osobne PR-y?
    • Czy zmiany zawierają zarówno nową funkcjonalność, jak i refaktoryzację (nie zawsze jest to dobre rozwiązanie)?
    • Czy nazwy i opisy commitów są wystarczająco dobrze sformułowane?
  • Czy metody/funkcje i klasy mają rozsądne rozmiary?
  • Czy stałe wymagają przeniesienia z kodu do konfiguracji albo zadeklarowania w osobnym miejscu w celu ułatwienia modyfikacji?
  • Czy istnieją jakieś opcjonalne komentarze lub skomentowany kod?
  • Czy istnieje „martwy” kod (tj. taki, który nigdy nie jest wywoływany/używany i nie wpływa na działanie programu)? Aby to sprawdzić, najprawdopodobniej trzeba będzie przeanalizować zmiany z poziomu własnej maszyny.
  • Czy podczas kompilowania pojawiają się nowe ostrzeżenia?

Wsparcie

  • Czy konieczne jest dodatkowe rejestrowanie/monitorowanie? Czy dostępne będą wystarczające informacje diagnostyczne, aby przeanalizować potencjalne problemy po implementacji?
  • Czy potencjalne błędy i wyjątki są obsługiwane prawidłowo?
  • Czy kod jest łatwy do zrozumienia i utrzymania? (KISS, YAGNI) (KISS — Keep It Simple, Stupid; YAGNI — You Ain’t Gonna Need It)
  • Czy zmiany są wstecznie kompatybilne z obecnym kodem? Zmiany w kontraktach (API lub komunikatach) muszą być wersjonowane, jeśli nie są wstecznie kompatybilne.
  • Czy występują jakieś potencjalne problemy z wydajnością i skalowaniem?
  • Czy są jakieś problemy z bezpieczeństwem? Czy kod wykorzystuje do działania dane osobowe użytkownika? Czy sprawdzana jest poprawność danych wejściowych?

Jakość

  • Czy zmiany są zgodne z wymogami? W razie wątpliwości należy skonsultować się z autorem.
  • Jak łatwy do odczytania jest kod? Czy tok myślenia autora jest zrozumiały?
  • Czy nazwy używane w kodzie są zrozumiałe?
  • Czy zasady SOLID są naruszane?
  • Czy daty i godziny są obsługiwane prawidłowo? Czy dana funkcjonalność została przetestowana pod kątem działania w różnych strefach czasowych?

Testowanie

  • Czy test jednostkowy wystarczy do sprawdzenia zmian?
    • Czy istnieją jakieś fragmenty kodu, które nie są objęte testami? Należy przeanalizować, jak trudne będzie ich przetestowanie.
    • Czy nazwa testu odzwierciedla funkcjonalność, której dotyczy?
    • Czy kontrakty zostały przetestowane (jeśli jest to wymagane)?

Wydanie

Czy wydanie zmian jest bezpieczne? Jeśli nie, należy przenieść je do osobnej gałęzi albo odpowiednio oznaczyć.