Ten artykuł został oryginalnie opublikowany w numerze 112 (marzec/kwiecień 2024) magazynu Programista.

Robienie dobrych code review to umiejętność, którą warto opanować. W artykule omówimy, jakie są zalety i wady tego procesu i w jakich projektach warto go stosować. Zastanowimy się, jakie podejście warto przyjąć, robiąc review kodu, w jaki sposób najlepiej to robić, na jakich aspektach kodu możemy się skupić i wreszcie – w jakiej formie pisać komentarze z naszymi uwagami do kodu, aby służyły one dla dobra projektu, a cały ten proces był okazją do owocnej komunikacji między członkami zespołu, a nie źródłem konfliktów.

Dostałeś/aś do zrobienia swój pierwszy code review i nie wiesz, jak się za to zabrać? Na co zwracać uwagę, przeglądając czyjś kod? W jakiej formie pisać komentarze? A może kod, który ty piszesz, będzie podlegał review innych osób i nie wiesz, czego się spodziewać ani jak zareagować? Jeżeli tak, to ten artykuł jest dla ciebie! Nawet jednak jeżeli jesteś już doświadczonym programistą, znajdziesz tu zestaw porad, z którymi być może się zgodzisz, a być może nie, ale z pewnością dadzą one do myślenia.

Mówimy tu oczywiście o procesie w wytwarzaniu oprogramowania, w którym jeden programista dostaje do przejrzenia kod innego programisty i ma za zadanie zgłosić swoje komentarze oraz zatwierdzić go, jeżeli wszystko jego zdaniem wygląda OK. Można by po polsku określić go na przykład jako „przeglądanie kodu”, ale ja pozwolę sobie używać angielskiego terminu „code review” ze względu na to, że jest on powszechnie używany.

To praktyka może przybierać różne formy w różnych organizacjach i zespołach. Może to być nieformalne zawołanie kolegi z biurka obok, aby spojrzał przez ramię na kod wyświetlony na ekranie i powiedział, co o nim myśli. Może też być to, z drugiej strony, w pełni sformalizowany proces, który używa specjalnego narzędzia w ramach systemu kontroli wersji (np. „pull request” na GitHubie albo Perforce Swarm) i wymaga od osoby przeglądającej zatwierdzenia, aby odblokowana została możliwość zrobienia merge czy też submit.

Będziemy tu omawiali code review jako proces w ramach pracy w danej organizacji, w danym zespole, nad danym projektem. Wszystko to samo może jednak dotyczyć także prowadzenia projektów open source, gdzie uwagi zgłaszane przez różnych użytkowników np. za pomocą „issues” i „pull requests” na GitHubie też stanowią tak jakby code review danego (publicznie dostępnego) kodu.

Kiedy i po co robić code review?

O tym, czy w ogóle warto wprowadzić praktykę code review w danym projekcie, należy zdecydować indywidualnie, biorąc pod uwagę charakter projektu oraz znając wady i zalety tej praktyki. Spośród zalet pierwsze do głowy z pewnością przychodzi podniesienie jakości kodu. Dodatkowa para oczu skierowana na każdy nowy czy modyfikowany fragment kodu to okazja, aby ocenić go z innej perspektywy, a więc być może znaleźć i zgłosić ewentualne błędy, zanim trafią do repozytorium. Mogą to być błędy logiczne, nieprawidłowo obsłużone „przypadki skrajne” (np. kiedy dane wejściowe są puste), wycieki pamięci, problemy z wydajnością czy nawet luki bezpieczeństwa.

Istnieją też jednak inne, mniej oczywiste zalety wykonywania code review. Wprowadzenie tej praktyki sprawia, że każdy fragment kodu widziały i znają w mniejszym lub większym stopniu co najmniej dwie osoby. Prowadzi to do pożądanego w każdym zespole rozpowszechnienia wiedzy, zamiast zamykać każdego inżyniera w „silosie” tematu, którym się zajmuje. Dzięki temu być może ktoś inny będzie mógł w przyszłości – w razie potrzeby – łatwiej wrócić do pracy z danym kodem. Innymi słowy, zwiększa się tzw. „bus factor”. Wreszcie, sama świadomość, że kolega czy koleżanka z zespołu przeczyta nasz kod, a nie wyłącznie kompilator, może nas zmotywować do większej staranności w jego pisaniu tak, aby nie tylko działał poprawnie, ale i był dobrze przemyślany i czytelny.

Możemy też oczywiście wymienić wady wykonywania code review. Pierwszą i najbardziej oczywistą jest cenny czas programistów, jaki należy na to poświęcić. Każdy czas pracy spędzony na przeglądaniu i komentowaniu cudzego kodu to czas, którego nie poświęcamy na pisanie albo debugowanie swojego kodu. Z punktu widzenia osób zarządzających zasobami i czasem programistów może to się wydawać wręcz jego marnowaniem, a przynajmniej czynnikiem obniżającym produktywność pracy. Nie bez znaczenia jest też konieczność oderwania się od swoich zadań i skierowania uwagi na inny fragment programu, co bywa obciążające mentalnie.

Argumentem często podnoszonym przez przeciwników włączania code review do procesu jest stwierdzenie, że błędy w kodzie powinny raczej być znajdowane przez dobrze napisane i regularnie wykonywane testy – jednostkowe, integracyjne itd., ewentualnie ręczne testowanie programu przez dział QA, a także inne automatyczne narzędzia, jak statyczna analiza kodu. Jest to oczywiście prawda, tak jak prawdą jest, że człowiek czytający kod nie „przetestuje w głowie” tylu możliwych przypadków, ile sprawdzą dobre testy.

Tym niemniej jedno nie wyklucza drugiego. Można powiedzieć wręcz, że testy automatyczne i code review świetnie się uzupełniają. Reviewer czy reviewerka może zwrócić uwagę na jakiś przypadek, który nie jest pokryty testami. Poza tym testy w ogóle nie sprawdzają istotnych aspektów kodu, takich jak jego struktura, czytelność, trafność nazw zmiennych i funkcji, komentarze czy dołączona dokumentacja, które mogą być kluczowe w przyszłym utrzymaniu kodu.

Jeszcze inną potencjalną wadą code review jest wydłużenie procesu. Konieczność zatwierdzenia każdej zmiany w kodzie, podobnie jak konieczność przejścia testów automatycznych, może spowodować, że każda, nawet najmniejsza zmiana trafia do repozytorium co najmniej po jednym lub kilku dniach od napisania. Taka sytuacja może być akceptowalna zależnie od charakteru projektu i tego, jak szybko należy iterować jego kod. Na efektywność procesu review ma też oczywiście wpływ organizacja tego procesu. Jeżeli wystarczy, że dowolny inny członek zespołu zatwierdzi zmianę, wszystko to może się dziać szybciej, niż kiedy każde review zatwierdza jeden lider zespołu, który w dodatku jest bardzo zapracowany lub akurat poszedł na urlop.

Podsumowując tę część, można powiedzieć, że to, czy warto robić code review, zależy od charakteru projektu. Jeżeli do napisania jest prototyp, jakieś narzędzie do użytku wewnętrznego czy też rozwiązanie tymczasowe, to być może nie warto tego robić, bo jakość kodu nie musi być jak najwyższa. Wówczas bardziej liczy się szybkość jego tworzenia, modyfikowania oraz zwinność całego procesu.

Z drugiej strony, jeżeli projekt dotyczy rozwoju i utrzymania systemu, który trafia do klientów, być może stanowi główny produkt naszej firmy, bądź też jest platformą, na bazie której działa inne oprogramowanie (np. kompilator, system operacyjny), to jakość kodu powinna być priorytetem. Zawsze jednak o tę jakość w jakimś co najmniej minimalnym stopniu warto dbać – nawet jeżeli nie wymuszają tego rozbudowane procesy. Wiemy w końcu, że „prowizorki bywają najtrwalsze”. Podsumowaniem tego rozdziału może być Tabela 1.

Niekoniecznie	Tak
Projekt mniej ważny, np. narzędzie pomocnicze	Projekt ważny, np. główny produkt naszej firmy
Liczy się czas – szybkie iterowanie	Liczy się jakość – jak najmniej błędów
Projekt tymczasowy, np. prototyp	Projekt długotrwały – istnieje lub będzie utrzymywany przez długi czas
Projekt dla wąskiego grona użytkowników, np. do użytku wewnętrznego	Projekt dla szerokiego grona użytkowników, np. udostępniony publicznie w Internecie
Bezpieczeństwo i niezawodność nie są kluczowe, np. gra single player uruchamiana lokalnie	Bezpieczeństwo i niezawodność są kluczowe, np. serwer sieciowy, platforma dla innego oprogramowania

Tabela 1. Czy warto robić code review w danym projekcie?

Jak podchodzić do sprawy?

W tym artykule nie padną żadne konkretne przykłady kodu, konstrukcji w językach programowania, które należy promować lub których należy unikać. Toteż artykuł może być dla ciebie przydatny niezależnie od tego, w jakim języku programujesz. Zamiast tego zajmiemy się ogólnym podejściem do robienia review. Zacznijmy jednak od początku…

Podczas robienia code review, tak samo, jak podczas programowania, obowiązują dobre praktyki programistyczne. Z pewnością znasz wiele z nich, bardziej lub mniej sformalizowanych, obojętne, czy przechodziłeś/aś jakieś kursy w tym kierunku, studiowałeś/aś informatykę czy może jesteś samoukiem i praktykiem. Mam na myśli wszelkie zasady prawidłowego, wydajnego, jak i prostego używania dostępnych konstrukcji w danym języku i bibliotekach. Wszystko, co mielibyśmy stale z tyłu głowy, pisząc swój kod, warto też brać pod uwagę, przeglądając czyjś kod.

Obowiązują też oczywiście zasady narzucone w danej organizacji, w danym zespole czy projekcie. Jeżeli mamy spisany dokument typu „coding standard”, który narzuca określony sposób nazywania zmiennych (CamelCase czy snake_case?) albo obsługi błędów w kodzie (rzucamy wyjątki, czy zwracamy numeryczny kod błędu?), to przestrzeganie takich zasad warto sprawdzać, robiąc review kodu. Taki dokument warto mieć, ponieważ może on rozwiązywać wiele niejasności i stanowić punkt odniesienia. Nawet jednak, kiedy żadnego formalnego standardu nie mamy spisanego, to w każdym zespole, który działa nie od dziś, z pewnością obowiązują pewne niepisane zasady (tzw. „wiedza plemienna”), które warto odkryć i się do nich dostosować.

Wreszcie, podczas robienia code review obowiązuje kultura osobista. Koniec końców komentarze w review to komunikacja człowieka z człowiekiem. Nie piszemy ich do kompilatora ani do ChatGPT, ale do żywej osoby. Wrócimy jeszcze później do tematu pisania komentarzy w review bardziej szczegółowo, ale warto zaznaczyć już teraz, że obowiązuje w nich (jak to się dawniej określało) „netykieta”, czyli zasady uprzejmości i dobrego wychowania, które powinny obowiązywać w Internecie, a tak naprawdę zawsze i wszędzie. W przypadku code review szczególnie może się przydać znajomość sztuki dawania i otrzymywania „feedbacku”, czyli informacji zwrotnej. Warto doczytać na ten temat.

W pewnym dużym projekcie obowiązywała zasada, że każda zmiana w kodzie musiała przejść review jednego programisty-leada, który trzymał pieczę nad całym kodem. Nazwijmy go „Wielki Ben”. To on tworzył podwaliny tego projektu wiele lat temu, a teraz – kiedy jego utrzymanie zostało powierzone nowemu zespołowi – nadal pilnuje, aby wszystko było zgodne z jego pierwotną wizją. Każe on pisać rozwlekłe komentarze objaśniające nawet najbardziej oczywiste rzeczy wynikające z samego kodu. Zabrania używać inteligentnych wskaźników i innych nowoczesnych mechanizmów w języku programowania. Wszystko musi być nazwane tak, jak on chce. Nie ma z nim dyskusji na temat szczegółów ani miejsca na własną opinię. Nietrudno sobie wyobrazić, co programiści w tym zespole myślą o nim, jak i pracy w tym projekcie.

Powyższa historia jest całkowicie zmyślona, a wszelka zbieżność z realnymi osobami czy projektami jest przypadkowa. Jednakże nikt nie chciałby trafić do takiego projektu. Tym bardziej my, robiąc review czyjegoś kodu, nie chcemy być takim Wielkim Benem. Dlatego warto zacząć od podstaw i zastanowić się, jakie mentalne podejście byłoby dobre podczas robienia code review.

Jako pierwszą i najważniejszą zasadę można polecić, aby uczestnicząc w review po którejkolwiek ze stron, pozbyć się własnego ego. Brzmi to może jak duchowe mądrości jakiegoś guru z dalekiego wschodu albo hipisa, który wziął za dużo LSD, ale w praktyce oznacza rzecz bardzo prostą: aby zbytnio nie przywiązywać się do swojego kodu, a uwag na jego temat nie brać do siebie. Można powiedzieć: „Pamiętaj, twój kod to nie jesteś ty!”

Napisany przez nas kod nie istnieje sam dla siebie, ale ma czemuś służyć. Doświadczony programista wie, że często bardziej wartościowa dla projektu, w porównaniu z dodaniem całych setek linii nowego kodu, bywa modyfikacja jednej kluczowej linii. Jeszcze bardziej wartościowe, a przy tym satysfakcjonujące, bywa usuwanie tysięcy linii kodu, które stały się niepotrzebne. Krótszy kod to w końcu mniej zakamarków, w których mogą czaić się nieodkryte jeszcze błędy. Nie warto więc nadto przywiązywać się do linijek kodu przez nas dodanych.

Tu pojawia się pytanie o odpowiedzialność. Jeżeli nie powinniśmy przywiązywać się do napisanego przez nas kodu i zaciekle go bronić, to czy w ogóle nie powinniśmy go nazywać „swoim kodem”, tylko traktować cały kod jako wspólny? Myślę, że przesadzić można zarówno w jedną, jak i w drugą stronę. Traktując każdą linijkę kodu jako wspólną, a nie napisaną przez nas, można zrzucić z siebie odpowiedzialność za wszelką jakość, za błędy, za czytelność i dalsze utrzymanie tego kodu, a więc nie dbać o niego, podobnie jak ludzie nie dbali o mienie w epoce komunizmu, kiedy wszystko było „wspólne”, „państwowe”, czyli „niczyje”, co zniechęcało też do starannej pracy.

Moim zdaniem, aby znaleźć balans między tymi skrajnościami, warto podchodzić do code review jak inżynier albo mechanik, a nie jak artysta. Niedobrze jest traktować swój kod jako dzieło, które podyktowała nam „wewnętrzna inspiracja”, i teraz niczego nie wolno w nim zmienić, wszystko ma zostać dokładnie tak, jak jest, aby inni mogli go podziwiać. Z takim podejściem każdy komentarz na temat kodu można potraktować jako osobisty atak.

Podchodząc jak mechanik natomiast, możemy myśleć o zmianach w kodzie jak o naprawie samochodu w warsztacie. Ów samochód może jest brudny, stary, nawet trochę zardzewiały, ale my zrobiliśmy w nim naprawę jednego układu i jesteśmy zadowoleni z tego, jak teraz działa. Podpisujemy się pod tą naprawą i bierzemy odpowiedzialność za to, że wykonaliśmy ją starannie i zgodnie z dobrymi praktykami. Być może trzeba było iść na pewne kompromisy, coś zamocować na taśmę i „trytki”, ale wiemy dokładnie, co, jak i dlaczego tak zrobiliśmy. Koniec końców chodzi o to, aby samochód jeździł, a klient był zadowolony.

Z tematem odpowiedzialności za konkretne części kodu wiąże się pytanie o to, kto powinien robić code review? Nie ma tutaj jednej reguły. Ta decyzja może wynikać ze specyfiki danego projektu i zespołu albo może nawet narodzić się naturalnie w toku nieformalnej komunikacji między programistami. Powierzanie każdego review jednemu lub kilku osobom najbardziej doświadczonym i piastującym najwyższe stanowisko to jedna opcja. Istnieje tu jednak niebezpieczeństwo, że taka jedna osoba mająca autorytet może szkodzić projektowi, narzucając swoje sztywne czy staroświeckie poglądy, jak wspomniany wyżej Wielki Ben. Inną opcją jest posiadanie przez każdy moduł, plik czy katalog swojego „właściciela”, który dany fragment kodu napisał, zna go najlepiej i sprawuje nad nim opiekę.

Paradoksalnie pewnym rozwiązaniem może być też powierzanie review różnym członkom zespołu łącznie z „juniorami”. Ci młodsi stażem, nawet jeżeli nie znają jeszcze dobrze naszego projektu ani nie mają dużego doświadczenia w programowaniu w ogóle, mogą wnieść do dyskusji nieco świeżości, patrząc na kod z nowej perspektywy, swoimi pytaniami rozpocząć ciekawe dyskusje, a na pewno sami wiele się nauczą, robiąc takie review.

Jak wykonywać code review?

Mając omówione ogólne podejście do sprawy, przejdźmy teraz do konkretów. Mamy do wykonania review czyjegoś kodu. Jak się za to zabrać? Od czego zacząć? Nie skupiamy się w tym artykule na konkretnych narzędziach ani językach programowania, ale zakładamy, że mamy do dyspozycji jakieś wygodne narzędzie, które pozwala przeglądać kod i wyraźnie zaznacza wprowadzone zmiany – linijki dodane, zmienione i usunięte.

Zmiany wprowadzone do kodu można przeglądać z różnych perspektyw – na poziomie bardziej ogólnym lub skupiając się na szczegółach. Ilustruje to Rysunek 1. Na poziomie ogólnym patrzymy, jakie pliki, klasy, być może całe katalogi i moduły zostały dodane. Próbujemy zrozumieć ich architekturę – logiczne znaczenie każdego z tych elementów, a także powiązania między nimi. Przypomina to naukę każdego nowego kodu, z którym przychodzi nam pracować. Jedyną różnicą jest tutaj fakt, że w code review mamy wyraźny podział na te funkcje czy klasy, które są nowe (dodawane przez autora kodu, który przeglądamy), oraz te, które w kodzie pozostają niezmienione. Tych ostatnich możemy nawet nie widzieć w widoku porównującym same zmiany, ale czasem musimy do nich sięgnąć, aby mieć pełny obraz całej architektury.

Rysunek 1. Różne poziomy, na jakich można przeglądać kod

Kiedy czujemy, że cały ten obraz już „nie mieści nam się w głowie”, czasem warto sobie rozrysować diagram klas na kartce papieru, w notacji UML czy jakiejkolwiek innej, być może naszej własnej. To pomaga zwizualizować sobie i zrozumieć powiązania między klasami – relacje dziedziczenia, zawierania i inne. Innym podejściem do zrozumienia ogólnej struktury kodu jest jego skompilowanie i uruchomienie w debuggerze, aby krok po kroku prześledzić przepływ sterowania i zrozumieć, która funkcja wywołuje które inne, pod jakimi warunkami itd.

Z drugiej strony natomiast, zmiany w kodzie można przeglądać z perspektywy szczegółów, a więc czytając kod linijka po linijce. Mając do czynienia z review, w którym zmieniane jest tylko kilka linijek, oczywiście będziemy patrzyli na niego wyłącznie z tej perspektywy. Czytając każdą linijkę, możemy w myślach sprawdzać wiele rzeczy. Czy dane wyrażenie arytmetyczne albo logiczne jest poprawne? Czy wskaźnik albo referencja, do której się odnosimy, w tym miejscu na pewno nigdy nie będzie pusta, powodując „crash” programu? Czy zmienna jest nazwana zgodnie z coding standardem, a jej nazwa odzwierciedla jej przeznaczenie?

Oba te aspekty są ważne i wzajemnie się uzupełniają. Komentarze zgłoszone w review mogą dotyczyć zarówno ogólnej struktury kodu, jak i konkretnych instrukcji. Analizując wyłącznie ogólną strukturę, może nam umknąć wiele błędów w konkretnych instrukcjach. Z kolei sprawdzając kod linijka po linijce, możemy nie zwrócić uwagi na niezbyt dobrą organizację całości w funkcje i klasy. Jednak trudno jest równocześnie myśleć o jednym i drugim tym aspekcie.

Dlatego review bardziej rozbudowanej zmiany w kodzie warto robić w kilku „przebiegach”. Najpierw przeglądamy całość szybko i pobieżnie – „przewijamy”, patrząc jak dużo kodu mamy do przejrzenia, których części projektu dotyczy zmiana oraz próbując „wyłapać” nazwy kluczowych klas i funkcji, które zostały dodane, zmienione lub usunięte. Zaczynając w ten sposób, możemy lepiej zdecydować, na których częściach kodu dobrze będzie skupić się bardziej szczegółowo w pierwszej kolejności, zamiast przeglądać go od góry do dołu.

Wiedza o nowym dla nas kodzie buduje się stopniowo. Nawet znając dobrze dany projekt i jego dotychczasowy kod, powinniśmy pamiętać, że nie znamy nowego kodu ani zamysłów jego autora od pierwszych chwil po otwarciu review. Dlatego nie warto spieszyć się z wprowadzaniem komentarzy. Coś, co może nam się wydawać w pierwszej chwili błędem, być może okaże się prawidłowe i celowe, kiedy to my – robiąc review – lepiej zrozumiemy ogólny obraz proponowanej zmiany. Pomysły na uwagi do przeglądanego kodu warto sobie notować gdzieś na boku i wprowadzić je oficjalnie do review dopiero na koniec, kiedy już cały kod przejrzeliśmy i dobrze zrozumieliśmy. W międzyczasie wiele z takich uwag, które przyszły nam do głowy, może ulec zmianie albo okazać się błędne.

Wiemy, że liczba linii kodu nie jest wyznacznikiem jakości projektu ani wydajności pracy programisty. Podobnie liczba komentarzy zgłoszonych w review nie powinna być celem samym w sobie ani metryką, na podstawie której oceniamy cokolwiek lub kogokolwiek. Jedna zmiana w kodzie może w oczach osoby przeglądającej wyglądać idealnie, spełniać wszelkie kryteria jakości i nie wymagać żadnych poprawek, a inna przeciwnie – wzbudzać wątpliwości w bardzo wielu miejscach. Żartobliwie mówi się, że miarą jakości kodu jest liczba „WTF na minutę”, jakie padają podczas jego review. Ta liczba zależy jednak od obu stron – autora kodu, jak i osoby go przeglądającej.

Istnieje też powiedzenie: „Daj programiście review na 10 linii, a zgłosi 10 komentarzy. Daj mu review na 1000 linii, a napisze tylko LGTM (Looks Good To Me)”. Ta druga sytuacja świadczyłaby o tym, że osoba przeglądająca nie przyłożyła się do szczegółowego przeanalizowania całości nowego kodu. Trzeba jednak przyznać, że łatwiej przegląda się zmiany małe. Lepiej więc duże zadania i duże ilości nowego kodu rozbijać na wiele małych zmian dodawanych i przeglądanych etapami. Można to robić na przykład na osobnym branchu, aby wykonać merge do głównego brancha dopiero, kiedy cała nowa funkcjonalność jest skończona.

Na co zwracać uwagę?

Kiedy przeglądamy kod, do głowy przychodzą nam różnego rodzaju uwagi, które możemy zgłosić autorowi. Warto zdawać sobie sprawę, że nie wszystkie są tak samo ważne i warte tego, aby o nie walczyć, jeżeli druga strona się nie zgadza i ma inne zdanie. Inaczej można też podzielić je na te bardziej obiektywne (np. błędy logiczne, które mogą doprowadzić do przerwania programu) i te subiektywne, które mogą być kwestią gustu (np. jak najlepiej nazwać daną zmienną). Sytuację tę zilustrowano na Rysunku 2.

Rysunek 2. Rodzaje możliwych uwag do kodu posortowane wg ważności

Ten podział nie jest jednak ścisły i nigdy nie powinniśmy być w stu procentach pewni swego. Coś, co może nam się wydawać oczywistym błędem, być może wcale nim nie jest, bo autor zapewnił, że dany wskaźnik czy referencja w danym miejscu nigdy nie jest null-em przez inny fragment kodu, na który nie zwróciliśmy uwagi. Z drugiej strony, kwestia nazwania zmiennej może być jednak ważna, jeżeli zaproponowana nazwa jest nieczytelna albo wręcz myląca.

Ogólnie rzecz biorąc, warto jednak autorowi kodu pozostawić pewną swobodę i nie wymuszać na nim czy to struktury całego kodu, czy to nazywania poszczególnych jego elementów dokładnie tak, jak my chcemy. Pozwólmy mu „wyrazić siebie”. Koniec końców to on jest autorem kodu, a my tylko reviewerem. Warto mieć w projekcie spisany „coding standard”, być może też używać narzędzia do automatycznego formatowania, jak Clang Format. Wtedy mniej drobiazgów w kodzie pozostawionych jest subiektywnej ocenie.

Zawsze jednak będą się pojawiały takie pytania, na które nie ma jednej obiektywnie najlepszej odpowiedzi, np.: „Czy ta nazwa funkcji jest tutaj najlepszą możliwą?”. Warto zadać sobie przy tym meta-pytanie: „Czy to jest ważne?”. Komentarze do spraw mniej ważnych i będących raczej kwestią gustu można oczywiście zgłaszać, ale tylko o te obiektywne i istotne (jak błędy w kodzie) warto się spierać i nalegać na ich zmianę.

Jednym z przykładowych aspektów kodu, na które można zwracać uwagę, analizując go na poziomie ogólnym, jest podział na klasy, ich semantyczne znaczenie i relacje między nimi. Warto przemyśleć, czy zaproponowana architektura jest najlepszą możliwą dla danego zastosowania, czy nie jest na przykład nadmiernie skomplikowana. Niektórzy programiści, zafascynowani klasyczną książką „bandy czworga”, próbują wszędzie znaleźć miejsce do zastosowania wzorców projektowych, aby ich kod był jak najbardziej ogólny i przygotowany na modyfikacje w przyszłości. Często nie biorą oni pod uwagę, że w przyszłości kod prawdopodobnie trzeba będzie zmienić w sposób, którego teraz nie mogą przewidzieć, a im bardziej będzie skomplikowany, tym ta zmiana będzie trudniejsza.

Na średnim poziomie szczegółowości przeanalizować możemy wydajność zaproponowanego algorytmu. Kwestia wydajności jest czasem zbywana cytatem spopularyzowanym przez Donalda Knutha „premature optimization is the root of all evil” („przedwczesna optymalizacja jest źródłem wszelkiego zła”). Ten cytat jednak jest jednym z najbardziej nadużywanych i opacznie rozumianych w całej informatyce, a także wyjętym z szerszego kontekstu. Autor miał na myśli poprawienie drobnych nieefektywności i mikro-optymalizacje, które mogłyby uczynić kod mniej czytelnym i trudniejszym w utrzymaniu, np. przez przepisanie go na asembler.

To prawda, że zajmując się optymalizacją wydajności, warto najpierw użyć profilera, zmierzyć tę wydajność i odkryć, na jakich częściach kodu należy się skupić – które stanowią wąskie gardło systemu. Projektując algorytmy i struktury danych, warto jednak przez cały czas mieć z tyłu głowy dobre praktyki dotyczące wydajności, tak samo jak i bezpieczeństwa. Warto takie aspekty oceniać też w review. Koniec końców algorytm o gorszej złożoności obliczeniowej, jak O(n²), może działać dobrze na danych testowych, ale dopiero, kiedy rozmiar danych będzie większy, algorytm okaże się tak nieefektywny, że użytkownik zgłosi, jakoby program się całkiem zawiesił (choć dla małych danych, mieszczących się w pamięci cache, taki algorytm może być czasem nawet szybszy).

Skupiając się na najniższym poziomie szczegółowości, można oceniać trafność nazw zmiennych czy funkcji, a także komentarze w kodzie. Wiele osób twierdzi, że komentarze w ogóle nie powinny być potrzebne, bo i bez nich kod powinien być czytelny sam w sobie. To moim zdaniem zbyt radykalne podejście, ale faktycznie dobre komentarze i czytelny kod są ze sobą ściśle związane.

W Listingu 1 pokazano przykłady komentarzy, które możemy nazwać złymi oraz jak można je poprawić. Użytym językiem programowania jest tu C++. Wybór tych konkretnych przykładów może być oczywiście dyskusyjny, ale ogólnie można chyba powiedzieć, że złe są takie komentarze, które zastępują czytelny kod. Źle jest powtarzać w komentarzu, co robi kod, jeżeli wprost jest to widoczne. Źle jest objaśniać komentarzem przeznaczenie zmiennej czy funkcji zamiast nadać jej odpowiednią nazwę. Źle jest wreszcie rysować „szlaczki” w stylu ASCII Art z komentarzy, aby podzielić kod funkcji na kilka części, zamiast podzielić go na osobne funkcje.

Listing 1. Przykłady kiepskich komentarzy i poprawionych wersji

return buffer_size; // Returns buffer size.
-->
return buffer_size;

// 7% is the percent of users to be selected for discount.
// Formula calculates number of users to receive discount.
AssignDiscount( GetTotalUserCount() * 7 / 100 );
-->
constexpr int percent_users_with_discount = 7;
int users_with_discount = GetTotalUserCount() * percent_users_with_discount / 100;
AssignDiscount( users_with_discount );

void RunProgram()
{
    ///////////////////////////////////////////////////////
    // Initialization
    …
    ///////////////////////////////////////////////////////
    // Calculations
    …
    ///////////////////////////////////////////////////////
    // Writing the results
    …
}
-->
void RunProgram()
{
    Initialize();
    Calculate();
    WriteResults();
}

Komentarze mogą być jednak przydatne do objaśniania kwestii, które trudno jest wyrazić samym kodem. Dobry komentarz to na przykład taki, który przed nieoczywistym algorytmem wyjaśnia, co ten algorytm robi, jak się nazywa lub skąd został zaczerpnięty. Podobnie komentarz może określić założenia dotyczące jakichś zmiennych czy parametrów funkcji, np. czy dany wskaźnik może być null-em i co to oznacza (np. kiedy dany parametr jest opcjonalny), inne specjalne wartości (np. zwrócony indeks -1 oznacza, że elementu nie znaleziono), jaki zakres wartości może przyjmować zmienna liczbowa (np. powinna być w zakresie 0…100) albo w jakich jednostkach jest wyrażona (np. w metrach, milisekundach, procentach).

Osobną kategorię stanowią komentarze dokumentujące interfejs jakiejś funkcji, klasy czy też całej biblioteki. Tutaj warto opisać każdą funkcję i jej parametr oraz każdą strukturę i jej pole, aby użytkownik mógł się z takich komentarzy nauczyć, jak używać tych elementów bez studiowania ich wewnętrznej implementacji. Takie komentarze też warto sprawdzać w review i patrzeć na nie jak na część dokumentacji, być może obok plików Worda czy stron na Confluence opisujących dany kod jeszcze ogólniej. Komentarze i dokumentacja to może być zresztą jedno i to samo, kiedy używamy rozwiązań takich jak Doxygen czy Sphinx, w których dokumentację można generować z komentarzy napisanych w odpowiednim formacie.

Powyżej wymienione zostały tylko wybrane przykładowe aspekty kodu, na które można zwracać uwagę, wykonując review. Nie sposób opisać ich wszystkich, szczególnie nie skupiając się na konkretnym języku programowania. Dlatego robienie dobrego code review to umiejętność, której można się nauczyć przede wszystkim przez praktykę.

W jaki sposób pisać komentarze?

Na koniec zastanówmy się, w jakiej formie wpisywać komentarze w review będące uwagami do przeglądanego kodu. Tak jak wspomnieliśmy wcześniej, przede wszystkim obowiązuje uprzejmość i kultura osobista. Z jednej strony to się może wydawać oczywiste. Z drugiej jednak może się okazać niełatwe. W końcu komentarze w review to asynchroniczny sposób komunikacji przypominający komentarze pod artykułami, postami w mediach społecznościowych i inne dyskusje w Internecie, a w tych – jak wiemy – nawet miłe i poważne osoby zamieniają się niekiedy w ostatnich chamów.

Pisząc komentarze w review, powinniśmy więc szczególnie uważać, aby nie dać się ponieść emocjom. Dobrą regułą jest nie pisać niczego, czego byśmy nie powiedzieli drugiej stronie prosto w twarz. Przed wysłaniem warto swój komentarz przeczytać jeszcze raz. To pozwoli wyłapać literówki i inne błędy w tekście, ale też da chwilę do namysłu nad jego treścią. Dodatkowo, robiąc review w ramach pracy jako programista, jesteśmy w sytuacji zawodowej, a więc powinniśmy się zachowywać i komunikować profesjonalnie. Żarty i dyskusje nie na temat może lepiej prowadzić na firmowych kanałach Discorda czy Slacka.

Mówiliśmy już o tym, aby pisząc kod, pozbyć się swojego ego i nie brać uwag na jego temat do siebie. Ta zasada działa też w drugą stronę: Zgłaszając komentarze do czyjegoś kodu, warto skupić się na kodzie, a nie na jego autorze. Taką sytuację zilustrowano na Rysunku 3. „Jesteś kiepskim programistą, bo napisałeś/aś ten kod w taki sposób.” – to przykład bardzo złego komentarza. „Ten kod jest napisany w nienajlepszy sposób.” – brzmi już lepiej. Jeszcze lepiej byłoby napisać: „Moim zdaniem ten kod nie jest napisany najlepiej, ponieważ (…) Czy nie lepiej byłoby to napisać tak: (…)?” W tej ostatniej wersji kryje się kilka dobrych praktyk. Omówimy je teraz po kolei.

Rysunek 3. Warto skupić się na kodzie, a nie na autorze

Po pierwsze, robiąc review, najważniejsza jest pokora. Nigdy nie powinniśmy być pewni swego na 100%, ale zawsze zakładać, że jednak możemy się mylić. Dlatego dobrze jest zacząć od stwierdzenia: „Moim zdaniem…”, „Według mnie…”, „Wydaje mi się, że…” itp. Koniec końców wyrażamy tutaj swoje zdanie – szczególnie jeśli coś jest kwestią subiektywną (np. „według mnie lepiej byłoby umieścić tę funkcję w klasie X”), ale również, kiedy coś wydaje nam się oczywistym błędem („według mojej wiedzy ta funkcja nie zadziała poprawnie z takimi parametrami”).

Po drugie, stwierdzenie „ponieważ (…)” to miejsce na uzasadnienie. Swoje zdanie warto poprzeć jakimiś argumentami. Jeżeli widzimy błąd, napiszmy przykład danych wejściowych czy wartości zmiennych, dla których wynik będzie nieprawidłowy. Jeżeli wywołanie jakiejś funkcji jest naszym zdaniem niezgodne ze specyfikacją danej biblioteki, wklejmy adres dokumentacji, która objaśnia jej prawidłowe użycie. W przypadku języka C czy C++ i jego biblioteki standardowej może to być np. adres konkretnej strony z serwisu cppreference.com. Jeżeli dany kod wydaje nam się niezgodny z ogólnie przyjętymi dobrymi praktykami w programowaniu, może nawet warto wkleić adres jakiejś przykładowej strony wyszukanej w Internecie na poparcie swojej tezy, że dana konstrukcja językowa jest niezalecana albo jak wygląda zalecana.

Po trzecie, omawiany dobry komentarz zawiera konkretną propozycję zmiany: „Czy nie lepiej byłoby to napisać tak: (…)?”. Warto być pozytywnym i nie tylko wskazać problem, ale i zaproponować możliwe rozwiązanie lub rozwiązania. Zamiast samego: „Nazwy zmiennych i, j są nieczytelne” dodajmy też: „Może lepiej nazywać je group_index, element_index?”

Wreszcie, sugestia „Czy nie lepiej byłoby to napisać tak: (…)?” nieprzypadkowo ma formę pytania. Warto stosować taką formę, bo nie tylko brzmi bardziej uprzejmie, ale też zachęca do odpowiedzi i dalszej dyskusji. Jeżeli okaże się, że jednak nie mieliśmy racji, czegoś nie wzięliśmy pod uwagę, kod jest jednak poprawny albo z jakichś innych powodów jego autor się z nami nie zgadza i nie chce go zmienić, naturalne będzie dla niego wpisanie odpowiedzi na takie pytanie, np. „Nie, ponieważ w moim kodzie (…) i dlatego wg mnie tak jest lepiej to zaimplementować.”

Wracając do roli autora kodu, który odpowiada na zgłoszone w review uwagi, warto jasno zasygnalizować, czy zgadzamy się z osobą przeglądającą kod i wprowadzimy lub już wprowadziliśmy żądane poprawki, czy też nie zgadzamy się i chcemy z nią polemizować (oczywiście podając przy tym argumenty na poparcie swojego zdania). Zależnie od zwyczajów panujących w zespole to może być bardziej formalne stwierdzenie typu „OK, akceptuję, poprawię to” albo nawet tylko emotka „łapki w górę” 👍. Nie zaszkodzi też podziękować za komentarz.

Podsumowanie

Na pytanie, czy warto robić code review, krótko i ogólnie można odpowiedzieć tylko: to zależy. Jeżeli jednak pracując w danym projekcie, podjęliśmy decyzję lub ktoś inny taką decyzję podjął, aby code review było częścią procesu, to warto wykonywać je starannie i trzymać się dobrych praktyk, jak opisane w tym artykule. Wtedy review będzie służyło dla dobra projektu, a nie tylko zabierało cenny czas.

Wykonując code review, dbajmy też o to, aby ten proces nie stanowił źródła konfliktów między ludźmi, ale przeciwnie – łączył we wspólnej sprawie, bo na code review można też i tak patrzeć – jako na dodatkowy kanał komunikacji między ludźmi, gdzie pretekst stanowi wspólne pochylenie się nad kodem.

Adam Sawicki
Luty 2024