Code review nie ma sensu?

Myślę, że każdy kto przeczytał tytuł tego tekstu łapie się za głowię i zadaje sobie pytanie „czy ten człowiek zwariował?!”. Jednak ten temat wyniknął z jednego powodu – na Twitterze pojawił mi się wpis Andrzeja Krzywdy, w którym Andrzej udostępnił wpis Justina Searlsa. W tym wpisie, Pan Justin pisze, że code review jest stratą czasu i tak naprawdę alternatywą do niego i najlepszą opcją jest pair programming, tylko taki prawdziwy, a nie taki gdzie jedna osoba siedzi i pisze kod, a druga tylko patrzy.

I w tym wpisie chcę podyskutować o tym jak wygląda code review i jakie mogą być z tym procesem problemy powodujące, że moglibyśmy go określić mianem bezsensownego.

Czym jest code review?

Ogólnie mogę powiedzieć, że jestem fanem code review. Mając do wyboru brak code review, lub code review w projekcie – jestem całkowicie za tą drugą opcją, bo nie da się być nieomylnym, więc sprawdzenie kodu przez kogoś innego ma jak najbardziej sens. Może na początek warto jednak wyjaśnić w ogóle, czym code review jest, bo wiem, że część osób, która odwiedza mojego bloga, jest osobami początkującymi i dopiero poznają wszelkie zagadnienia związane z programowaniem.

W dużym skrócie, wyobraź sobie, że napisałeś ten swój świetny kod, jest gdzieś na Twoim branchu, czy w jakimś innym miejscu i czeka na merge z główną gałęzią. Tutaj, zanim faktycznie to połączenie nastąpi, robisz STOP i bierzesz kolegę lub koleżankę z zespołu i on lub ona ma za zadanie przejrzeć ten kod. To jest właśnie proces code review. Chodzi o to, żeby ktoś inny spojrzał na kod świeżym okiem osoby, która nie spędziła ostatnich tygodni na tymi kilkoma linijkami kodu. W tym procesie chodzi o to, żeby sprawdzić, czy ten kod ma sens, trzyma się kupy, założeń projektu itd.

I to jest forma code review, która działa całkiem ok. Po pierwsze, dlatego, że jest to robione przez jedną, maksymalnie dwie osoby, więc nie trzeba znajdować czasu całego zespołu. Druga sprawa, to powinno być zrobione bardzo szybko, co w takiej formie ma miejsce. Przykładowo, u nas w zespole była taka zasada, że pierwszą rzeczą, którą powinniśmy zrobić danego dnia jest code review. Przychodząc do pracy, pierwsze zadanie, które bierzemy z góry, to code review, tak żeby było gotowe od razu.

Patologia i kult spotkań

Jednak sytuacja, o której pisze Pan Justin to code review, gdzie wszyscy się razem zbierali, puszczali sobie kodzik na projektorze i oglądali go, patrzyli z każdej strony, oceniali, krytykowali publicznie. I jest to sytuacja w której absolutnie rozumiem, że jest to marnowanie czasu, do tego jest to formalne spotkanie, czyli marnowanie chęci do życia zespołu, po trzecie jest to publiczne krytykowanie, które jest demotywujące dla wielu osób, a po czwarte, przez to, że musi być to zaplanowane wcześniej, żeby móc zrobić spotkanie, to takie review jest robione bardzo rzadko, raz na jakiś czas, bo nie da się mieć spotkania co godzinę. Taka forma code review, gdzie musimy zaangażować więcej osób, a nawet cały zespół, i która nie może być zrobiona w miarę natychmiastowo, a więc w przeciągu paru godzin, faktycznie nie ma sensu.

Jeśli code review sobie leży przez tydzień nie sprawdzone, potem musi być zatwierdzone przez dwie, trzy, pięć osób, do tego jeszcze pewnie konkretnych, wybranych w zespole, to jest to patologia i taka forma, jak już wspomniałem, nie ma sensu, bo to powoduje, że chęć do wrzucania jakiegokolwiek kodu maleje, bo dostajesz feedback po tygodniu, lub później, już dawno nie pamiętasz co robiłeś w tym kodzie, poza tym, ponieważ robi to zawsze jedna osoba, to staje się ona takim królem kodu i ciężko z nią dyskutować, bo jako jedyna ma pełną wiedzę o kodzie w projekcie i ostateczne zdanie.

Mniej formalnie

W jednym z tekstów, które napisałem dla naszej firmy, napisałem coś w stylu „im bardziej formalne code review, tym bardziej zniechęca do wrzucania kodu i sprawdzania tego kodu”. Im więcej formularzy czy pól, które trzeba uzupełnić akceptując code review, pobierania masy rzeczy, odhaczania kilkunastostronicowej checklisty, tym bardziej odrzucający jest cały proces code review. Jeśli sprawdzenie zmian, które jedynie poprawiają jakaś literówkę, wymaga ode mnie żmudnego procesu przechodzenia przez formularze, pobieranie kodu, uruchamianie ręcznie wszystkich testów, robienia opisów itd. to już sama myśl o tym jest dla mnie męcząca i będę odkładał takie zadanie jak najpóźniej.

Opisana przed chwilą forma code review, jak najbardziej nie ma sensu i takie code review, które jest odkładane w czasie i wywołuje niechęć w zespole będzie tylko powodować coraz większe opóźnienia. Zwłaszcza, że w większości przypadków code review blokuje proces akceptacji pull requesta, a więc też dostarczenia całej funkcjonalności.

Blokowanie pull requestów przez code review, w połączeniu z odkładaniem procesu code review na później, sprawia, że funkcjonalności później znajdą się na produkcji, albo w testach i będą dostarczane rzadziej niż by to było możliwe. Do tego, jeżeli feedback z code review dostajesz po tygodniu, to Twój mózg już dawno się przełączył na inne zadania i nie pamiętasz od razu dlaczego coś w taki sposób zrobiłeś i co było w ogóle w treści zadania. Musisz więc poświęcać kolejne godziny na przypominanie sobie tego wszystkiego.

No dobrze, to jak robić dobrze?

W miarę przyzwoite code review, którego nie nazwałbym bezsensownym będzie wtedy, kiedy zadania, nad którymi pracujemy są względnie nieduże, albo raczej pull requesty, które wrzucamy są niewielkie, dzięki temu ilość kodu, który mamy do przejrzenia w ramach code review jest niewielka, a więc można to zrobić szybko. Po drugie, code review powinno być priorytetem, czymś co bierzemy w pierwszej kolejności jak tylko zabieramy się za nowe zadanie, tak żeby pull requesty nie „wisiały” w nieskończoność. Kolejna sprawa, to osoby odpowiedzialne za code review – powinna móc to robić większość zespołu, zamiast wybranych pojedynczych osób, dzięki temu unikniemy patologii trzymania wiedzy u jednej osoby, lub konfliktów wynikających z tego, że wszyscy muszą się podporządkować tej jednej osobie, która przegląda jako jedyna każdy kod. Następny istotny punkt to brak nadmiernych formalności, code review powinno mieć wytyczone pewne wskazówki, ale każdy powinien móc dopasować code review do sytuacji.

Poza tym poprawny proces przeglądania kodu wymaga, żeby też inne elementy projektu były skonfigurowane poprawnie. Jakie to elementy? Przede wszystkim automatyczne uruchamianie testów dla każdego pull requesta i również automatycznie uruchamiane sprawdzanie zgodności kodu ze standardami, jakie przyjęliśmy w projekcie, robione chociażby przez narzędzia takie jak SonarCube. Dzięki temu czas robienia code review maleje drastycznie, bo odpada nam sprawdzanie wszystkich elementów, które są powtarzalne i mogą być zrobione mechanicznie, a więc przeglądając kod możemy się skupić jedynie na logice i wysokopoziomowej czytelności, a więc chociażby na wzajemnym ułożeniu elementów kodu.

Dobry proces code review gwarantuje, że absolutnie nie będziemy chcieli z niego rezygnować, bo nie będzie stratą czasu, ale sposobem na utrzymanie, lub wręcz podniesienie jakości kodu w naszym projekcie i metodą szerzenia wiedzy wśród członków zespołu.