7 błędów code review

Code review może bardzo poprawić jakość kodu. Jest tylko jeden warunek -  by wykorzystać ten potencjał, trzeba je odpowiednio przeprowadzić. Bez zbędnego gadania przechodzimy do listy najczęstszych błędów, które powodują, że marnujemy tę okazję.


Skupienie się na stylu, składni


Ten typ review widziałem wiele razy: mnóstwo uwag wokół tego jak coś zostało zapisane, a kwestie bardziej istotne bez pojedynczego komentarza. Popełnia się ten błąd głównie dlatego, że najłatwiej jest takie rzeczy wyłapać. Oko programisty przyzwyczajone jest do wyszukiwania zbędnych spacji, niepotrzebnych nowych linii czy rozjazdów we wcięciach. To robimy z automatu.


Tak powinno być:


Skoro programista robi coś z automatu, to znaczy, że automat może zrobić to jeszcze lepiej. By tak się stało należy ustalić w zespole konwencję stylu (może być jedna z publicznie dostępnych) i używać narzędzi do automatycznego poprawiania kodu pod tym względem. Konwencja ta musi również zostać odzwierciedlona w środowisku developerskim każdego programisty z firmy - niezależnie od tego czy używa Emacsa, VIM-a, Visual Studio czy intelliJ. Narzędzia do statycznej analizy kodu wyłapują wiele nieprawidłowości w kodzie i robią to o wiele skuteczniej niż człowiek. Zarówno wymuszenie konwencji stylu jak i analiza kodu powinny być warunkiem wstępnym do recenzowania kodu.


Review powinno się zaczynać od poziomu uproszczeń składniowych, których nie są wstanie wykonać narzędzia automatyczne. Tu z kolei łatwo się skupić uproszczeniach składniowych czy literówkach. Jednak celem code review jest zrozumienie całości zmian i poprawa ich jakości. Dlatego gdy nie rozumiemy to nie klikamy “approve”, ale staramy się dowiedzieć o co chodzi.

Pomijanie testów


Widzisz w review dużo testów, przelatujesz tylko przez nie wzrokiem tak by jak najszybciej przejść do implementacji. Uwierz mi - rozumiem Cię :) Recenzowanie testów jest nudne. Ciągle tylko setup, assert, teardown. Ta powtarzalność szybko usypia naszą czujność i łatwo założyć, że skoro pierwsze kilka testów jest ok, to reszta też będzie ok. Niestety, często tak nie jest. Skąd się biorą te testy, które nie przechodzą raz w miesiącu (zwykle 30 albo 31)? A te zależne od wyniku poprzedniego testu? Albo te, które sprawdzają tylko trywialne rzeczy? To teoretycznie nie powinno przejść przez code review.


Tak powinno być:


Testy to też kod i zasługują na porządne review. Tu jest dobra szansa na upewnienie się, że kod jest dobrze otestowany. Wymaga to dużego skupienia i dokładnego przemyślenia proponowanej zmiany - dlatego nie jest łatwe. Cały team musi zwrócić na to uwagę i bez wsparcia wszystkich członków zespołu będzie to walka z wiatrakami. Przemyślcie co i jak chcecie testować, co w Waszej aplikacji jest kluczowe. Wymagajcie od siebie nawzajem wypełniania tych ustaleń.


Patrzenie tylko na dodany kod

Dużo większą uwagę zwracasz na te zielone linijki niż na te czerwone? Tak, w większości przypadków to, co się dodaje, jest istotniejsze, ale nieopatrznie usunięta linia może narobić niezłego ambarasu o ile nie masz dobrze przetestowanego systemu. Co jeżeli usunięta metoda była jeszcze gdzieś potrzebna? Co jeżeli usunięte zostało ostatnie wywołanie?


Tak powinno być:


Przy wszelkich zmianach w liniach usuniętych będzie widać zamysł poprzedniej implementacji. Dzięki temu można porównać nowe i stare, dopytać skąd się wzięły różnice o ile to nie jest oczywiste.


Dodatkowo jeżeli w changesecie została usunięta metoda, to na code review obowiązkiem jest sprawdzenie (lub zapytanie) czy wszystkie wywołania również zostały usunięte. Z drugiej strony warto sprawdzić, czy przypadkiem usunięcie wywołania nie było ostatnim użyciem danej metody w kodzie. Może można ją usunąć i wyczyścić tym samym codebase.

Złe wyczucie momentu


Pół godziny przed demem to bardzo zły moment na code review. Zwykle wygląda to tak, że razem z powiadomieniem o review dostajesz też wiadomość od autora “klepnij mi to, bo za 15 minut robimy deploy na demo”. Na porządną recenzję potrzeba czasu i spokoju, a taka zrobiona na szybko nie zapewni jakości. Takie sytuacje często też biorą się ze złego zarządzania, przez co programiści czasem nie czują się za to odpowiedzialni. Bez względu na to, kto jest winien, marny kod został wcielony do nieodpowiedniej gałęzi i prawdopodobnie tam pozostanie.


Tak powinno być:


Dobrym zwyczajem jest zamykanie zadań sporo przed prezentacją, releasem czy jakimkolwiek deadlinem. Tylko to zapewni nam, że będzie wystarczająco dużo czasu i na review i - przede wszystkim - na późniejsze poprawki.


Czasem jednak zdarza się duża obsuwa, a pokazanie nowego killer-feature’a to być albo nie być. Wtedy zamiast pospieszać review lepiej jest zbudować demonstracyjną paczkę z innej gałęzi niż ta, do której trafia zrecenzowany kod. Trzeba pomyśleć o takiej możliwości zawczasu i przygotować odpowiednio proces budowania lub deploymentu.


Niewnikanie w design

Kolejny błąd wynikający ze zbyt płytkiego wejścia w rolę recenzenta. Łatwiej jest stwierdzić, że kod będzie działać i nie ma w nim rażących uchybień niż ocenić, czy jego design jest dobry. To dlatego, że trzeba dokładnie przemyśleć, jaką rolę w systemie pełni nowy kod i czy dobrze wpasowuje się w to, co już jest napisane. Prześledzenie zgodności z dobrymi praktykami np. OOP wymaga dużo skupienia.


Tak powinno być:


Z jednej strony należy przeanalizować czy nowe komponenty/zmiany wytrzymują zderzenie z zasadami i dobrymi praktykami. Czy same w sobie są dobrze zaprojektowane? Kolejnym krokiem jest wyjście na poziom wyżej - wpływu na architekturę. Większość zmian będzie dla niej kompletnie obojętna. Jednak te, które zmieniają architekturę są bardzo istotne i trzeba dobrze przemyśleć, czy to dobry kierunek. Można, a nawet powinno się, skonsultować taką zmianę z większą liczbą osób.

Zbyt duże changesety


PR z tytułem “Major system refactoring” i statystykami w stylu “60 files changed, 1740 insertions(+), 1202 deletions(-)” to jest bardzo zły pomysł. O ile prawdopodobnie przy odpowiednim wysiłku samo przejrzenie zmian jest możliwe, to umieszczenie tych zmian w szerszym kontekście - procesów biznesowych czy aplikacji - będzie baaardzo ciężkie. Dlatego też jakość takiego review będzie z natury dość niska. Przy takiej ilości zmian recenzent może się skupić co najwyżej na kwestiach składni.


Tak powinno być:


Bardzo ważne jest dzielenie pracy na małe kawałki. Po pierwsze daje to lepszą kontrolę nad postępami prac, po drugie znacznie ułatwia review. Oczywiście tu może dużo pomóc odpowiednio kumaty PM, który już przy planowaniu zwróci uwagę na ten problem.


Duży changeset może powstać w wyniku przejścia do nowszej wersji podstawowej biblioteki (np. frameworku) i często jest to nieuniknione w dużych aplikacjach. Tu zawsze warto minimalizować zakres zmian i nie popaść w manię aktualizacji. Dodatkowo, jeżeli zmian będzie bardzo dużo, można otworzyć review wcześniej i robić je stopniowo - pewnie dzięki temu uniknie się powtarzania części błędów. W takim wypadku jednak dokładne przejrzenie finalnej wersji i tak będzie potrzebne.


Niejasne komentarze

Opatrzenie linijki, która nam nie pasuje komentarzem “Fix plz”, to częsty błąd. Skąd druga osoba ma wiedzieć, o co konkretnie nam chodzi? Raczej nie wyczyta tego z naszych myśli. Może się domyśli, o ile będzie to coś oczywistego, a może nie ma pełnej wiedzy by to poprawić, albo ma w tym konkretnym wypadku rację. Nie dowiesz się, dopóki nie napiszesz jasno, co jest nie tak z danym fragmentem.


Tak powinno być:


Code review jest też miejscem gdzie można konfrontować różne pomysły na kod. W końcu nie zawsze inaczej znaczy źle. Jednak żeby stworzyć pole rozmowy trzeba jasno definiować problem, a także proponować rozwiązanie. Bez tego nie ma podstaw do dialogu czy zmiany zdania. Poza tym, dzięki szerszym opisom code review staje się też miejscem, gdzie programiści mogą się od siebie nawzajem uczyć i to jest bardzo cenne.


Code review to świetne narzędzie, jedno z tych bardziej użytecznych. Może posłużyć do znacznej poprawy jakości kodu, konfrontowania idei czy przekazywania wiedzy. Może być też procesem dla procesu. Według mnie warto się postarać, żeby wycisnąć z niego maksimum.