Ten artykuł jest jednym z dwóch artykułów z miniserii dotyczącej tematyki pull requestów oraz code review. W tym artykule omówię jak zrobić DOBRE code review. W drugim artykule z tej serii możesz przeczytać artykuł z przeciwnej perspektywy – jak przygotować DOBRY pull request.

Mam nadzieję, że termin code review nie jest Ci obcy. Jeśli jednak widzisz go po raz pierwszy, to spieszę z pomocą. Code review to proces analizy kodu przeprowadzany przez kogoś innego niż autor kodu. Główne cele code review to:

• Analiza kodu pod kątem poprawności, jakości i bezpieczeństwa zaimplementowanych zmian;
• Detekcja potencjalnych błędów;
• Dzielenie się wiedzą w zespole. Pod kątem dzielenia się wiedzą działa to w dwie strony. Uczyć się z code review można z komentarzy, które ktoś zostawi przy naszym kodzie. Można też uczyć się, czytając kod kogoś innego i samemu robiąc code review. Na początkowych etapach swojej kariery starałem się robić tyle code review, ile się dało. Nawet jeśli ostatecznym rezultatem miało być magiczne LGTM 😉

Kilka par oczu wychwyci więcej niż jedna. Dodatkowo pracując nad zadaniem samodzielnie, zamykamy się na tylko naszą perspektywę. Świeże spojrzenie kogoś innego potrafi niekiedy całkowicie „wywrócić stolik”.

Empatia

Moim zdaniem empatia to najważniejszy aspekt dobrze przeprowadzonego code review. Pewnie nie jest Ci obcy poniższy mem.

Mam kilka wskazówek, które mogą pomóc w tym, by code review nie wyglądało tak, jak na przedstawionym obrazku:

• Pierwsze i najważniejsze — nie bądź 🍆 (tu oryginalnie miało być brzydkie słowo). Zostawiając komentarz, zastanów się, czy sam chciałbyś/chciałabyś taki otrzymać.
• Code review służy do weryfikacji jakości kodu/rozwiązań, a nie oceniania autora. Niedopuszczalne na code review są wszelkie przytyki, osobiste wycieczki i uwagi odnoszące się do autora kodu (jego wiedzy, umiejętności).
• Zgłaszając coś, upewnijmy się, że uwagi są merytoryczne. Podajmy merytoryczną i rzeczową argumentację sugestii swojej zmiany oraz linki, źródła, dokumentację, jeśli są dostępne. O faktach rozmawia się lepiej niż o opiniach. Uwaga na zasadzie „zróbmy to w sposób X, bo moim zdaniem to jest lepsze” jest jak dyskusja o wyższości jabłek nad gruszkami (jabłka lepsze).
• Tryb rozkazujący w code review zazwyczaj jest fuj. Pytanie zamiast żądania jest mniej ofensywne. Twarde stwierdzenia zostawiłbym na przypadki, które nie pozostawiają wątpliwości.
• Code review to nie konkurs „kto znajdzie więcej”. Nie ma nic złego, by nawet w dużym pull requeście po prostu zostawić approve.
• Szanuj czas autora pull requesta. Nie każ mu czekać kilka dni na code review. Warto też zastanowić się, czy to, co zgłaszamy, faktycznie warto naprawiać. Być może zaproponowane przez nas rozwiązanie jest lepsze, ale jest nieopłacalne. Robiąc code review, warto mieć w głowie reguły DRY, KISS i YAGNI. Co z tego, że nasze rozwiązanie da większe korzyści przy milionie połączeń do serwera, jeśli obecny ruch do serwera to tysiąc połączeń i dodatkowe trzy dni pracy autora?

W komunikacji podczas code review warto również zwrócić uwagę na inne aspekty:

• Warto ostrożnie korzystać ze słów „zawsze”, „nigdy”, „wszyscy”, „nikt” itp.
• Unikałbym ogólników np. „to jest źle”, „to można zrobić lepiej”.
• Komentarze powinny być rzeczowe i jednoznaczne. Wiąże się to z punktem poprzednim. Jeśli mamy jakieś uwagi, to warto upewnić się, że zostały przekazane w sposób klarowny. Nie należy też przeginać w drugą stronę. Ciężko czyta się wypracowania, gdzie kluczem są dwa zdania w środku tekstu.
• Zachęcam, by nie tylko wyłapywać rzeczy do poprawy, ale też by wskazywać, jak można poprawić zgłaszany problem. Osobiście niezbyt leży mi podejście „to jest źle, ale dlaczego to już musisz sam na to wpaść” lub „to można poprawić, wymyśl jak”. Wiem jednak, że są też zwolennicy tego podejścia. Argument za tym podejściem jest taki, że zmusza ono do myślenia.
• Jeśli nie jesteśmy czegoś pewni, to warto zostawić komentarz w formie pytania. Zostawienie błędnej sugestii marnuje czas autora pull requesta na weryfikację i ewentualną dyskusję.
• Warto dać znać, jak istotny jest zgłaszany problem. Warto rozróżnić, pytania, sugestie i drobiazgi od tematów ważnych i krytycznych.
• Blokując merge, upewnij się, że jest czas i przestrzeń na polerowanie kodu. Czasami sytuacja niestety wymaga zaakceptowania nieidealnego kodu. Gdy produkcja leży, nie ma czasu na oglądanie pull requesta przez lupę 😉
• Widzisz jakieś fajne rozwiązanie? Warto to docenić! Komentarze w code review służą nie tylko do negatywnego feedbacku. Pozytywny feedback jest równie wartościowy.
• Miej cierpliwość do mniej doświadczonych członków zespołu. Błędy w kodzie zwykle nie wynikają ze złej woli lub lenistwa. Warto uważać z założeniami, że ktoś coś wie lub powinien wiedzieć.
• Jeśli jest okazja, by pokazać komuś jakiś ciekawy wzorzec, podejście czy rozwiązanie, to warto to zrobić. Code review służy również wymianie wiedzy. Nawet jeśli ostatecznie sugestia nie zostanie wykorzystania, to autor kodu może dowiedzieć się czegoś nowego.
• Dla zduplikowanych problemów zostaw jeden zbiorczy komentarz. Nie ma sensu zostawiać kilkudziesięciu identycznych komentarzy.
• Zachęcam do zapoznania się z zagadnieniem Komunikacji Bez Przemocy (NVC) i wypróbowania tego podejścia w praktyce. Jeśli interesuje Cię temat NVC w codziennej pracy, to koniecznie daj znać w komentarzu.

Kiedy robić code review?

Tu można odpowiedzieć krótko — jak najszybciej. W benchmarku przeprowadzonym przez LinearB dla ogółu badanej populacji metryka Review Time kształtowała się następująco:

• Elite — poniżej 3 godzin;
• Good — 3-13 godzin;
• Fair — 14-24 godziny;
• Needs Focus — 24 godziny.

Oczywiście czas na review zależy od rozmiaru zespołu, projektu, obłożenia pracą i miliona innych czynników. Nawet sam rozmiar pull requesta może mieć wpływ na to ile czasu zajmie jego sprawdzenie. Najdłuższe code review robiłem powyżej 3 godzin. Sam do tego typu metryk podchodzę sceptycznie, jednak zachęcam, by spróbować wyciągnąć z nich wnioski. Podobnie jak autorzy badania, uważam, że czekanie powyżej 24 godzin na code review jest czymś wymagającym poprawy.

Oczekiwanie na code review często blokuje inne osoby. Czasami na code review potrzeba przegadać propozycje pewnych rozwiązań. Przez to np. dalsze prace nad zadaniem mogą być zablokowane. Może też być tak, że ktoś potrzebuje zmian z jednego PR w innym zadaniu, np. bugfix, który blokuje jakieś inne zadanie. Warto wykazać się empatią i upewnić się, że nikogo nie blokujemy przez zwlekanie z code review.

Na co zwracać szczególną uwagę a na co niekoniecznie?

Do rzeczy, na które moim zdaniem należy zwrócić szczególną uwagę, należą:

• Decyzje architektoniczne — jakie były czynniki decyzyjne, co zadecydowało o wyborze danego rozwiązania, jakie były alternatywne rozwiązania, czy jest jakiś ślad po decyzji np. w formie ADR (Architecture Decision Record).
• Schludność i estetyka kodu — poprawne i logiczne nazwy zmiennych i metod, optymalizacja warunków, czytelność i prostota kodu.
• Poprawność kodu i implementacji — czy kod robi to, do czego został stworzony.
• Wydajność kodu — tego czasami nie da się wyłapać na etapie czytania kodu. Jednak można czasem trafić na kod, po którym od razu widać, że jest nieoptymalny. Może to być łatwa do zmniejszenia złożoność algorytmiczna, wykorzystanie nieefektywnych metod, niepotrzebne operacje w kodzie, sekwencyjne wykonywanie operacji, gdy można je zrównoleglić itp.
• Testy — nie chodzi mi tu absolutnie o pokrycie kodu testami. Do sprawdzania pokrycia kodu służą dedykowane narzędzia, a nie człowiek. Swoją drogą code coverage uważam za metrykę o marginalnej użyteczności. Chodzi mi o sprawdzenie, czy:
  • pokryte zostały istotne przypadki;
  • opis testów jest poprawny;
  • testy są czytelne i zrozumiałe;
  • faktycznie testują to, co powinny;

  Zachęcam też do wypróbowania podejścia polegającego na rozpoczęciu code review od testów. Pozwala to spojrzeć na strukturę proponowanego rozwiązania okiem klienta. Łatwo wtedy zrozumieć intencje i oczekiwania autora kodu, jak kod powinien być użyty.

Przeprowadzając code review, warto też spojrzeć na zmiany w szerszym kontekście. Mam tu na myśli analizę tego, jak dana zmiana wpłynie na cały system. Warto zastanowić się, czy dodana zmiana nie będzie miała negatywnych efektów w innych miejscach systemu. Przykładowo, w funkcji do prostych obliczeń znajduje się błąd i zwraca błędny wynik. Okazuje się jednak, że wiele miejsc w systemie na niej polega i wyniki zwracane w tych miejscach są zgodne z oczekiwaniami. Czy w takim przypadku naprawienie logiki w tej funkcji jest poprawne?

Pomocna w zrozumieniu sprawdzanego kodu jest również znajomość istniejącego kodu. Jeśli zmiana doszła w już istniejącym pliku, to pomocne może być sprawdzenie wcześniej istniejących zmian. Doda to więcej kontekstu do analizowanego kodu i może pomóc wypracować lepsze rozwiązania.

Piramida priorytetów code review

Moim zdaniem priorytety code review świetnie określa poniższa piramida.

Bez poprawności kodu nie ma sensu analizować jego pozostałych aspektów. Wdrożenie poprawnego, ale niebezpiecznego jest proszeniem się o kłopoty. Idąc dalej, w dłuższej perspektywie utrzymanie nieczytelnego kodu będzie trudne. Mając czytelny kod mamy pole do dyskusji nad jego elegancją i potencjalnymi usprawnieniami. Finalnie, gdy nasz kod jest dopięty na ostatni guzik, możemy pomyśleć o innych miejscach w kodzie oraz np. o zasadzie skauta.

Lokalne uruchamianie i pobieranie kodu

W tym aspekcie mam mocne „to zależy”. Zwykle nie pobieram i nie testuję kodu na lokalnej maszynie, gdy robię code review. Do testowania poprawności działania kodu są testy i wolę skupić się na weryfikacji poprawności testów niż ręcznie sprawdzać, czy coś działa. Dodatkowo wierzę, że to czy testy przechodzą i kod się uruchamia, powinien sprawdzać CI, więc po co robić to manualnie.

Jednak analizując zmiany w np. wybranych flow E2E czy zmiany na warstwie UI warto rozważyć faktyczne pobranie i ręczne „przeklikanie” zmian. Testy E2E są powolne i dość kosztowne w rozbudowie i utrzymaniu. Mogą przez to nie pokryć wszystkich istotnych przypadków. Podobnie z testami UI. Wątpię w sens np. pisania testów dla layoutów dla wszystkich popularnych rozdzielczości ekranu.

Inną sytuacją, gdzie moim zdaniem warto pobrać zmiany i je przetestować lokalnie, są zmiany wpływające na developer experience, czy zmiany związane z toolingiem developerskim. Jeśli sprawdzamy kod toolingu, którego będziemy w przyszłości używać, warto sprawdzić, czy jest dla nas komfortowy i spełnia nasze oczekiwania. W przypadku takich zmian może też zwyczajnie nie być testów. Wtedy ręczne testowanie zmian jest wręcz wskazane.

Nitpicks

Nitpickami określa się wszystkie drobne rzeczy, które można poprawić, ale które nie powinny wpływać na werdykt code review. Słownik DIKI świetnie tłumaczy nitpicking jako „czepiać się” lub „szukać dziury w całym”. Moim zdaniem tym określeniem można określić takie rzeczy jak:

• Drobne błędy interpunkcyjne, ortograficzne, nadmiarowe lub brakujące znaki np. średniki, gdy nie jest to konieczne; Tego typu błędy powinny być zgłaszane i najlepiej automatycznie naprawiane przez lintery i formatery, a nie w trakcie code review. Jeśli w trakcie review wychodzą takie rzeczy, to problemem jest brak lub niewłaściwa konfiguracja narzędzi, a nie każdy pojedynczy średnik czy brakująca spacja.
• Rzeczy o marginalnym znaczeniu dla jakości kodu np. kolejność metod w klasie, parametrów w metodzie, korzystanie z syntax sugar.
• Nazewnictwo w kodzie POD WARUNKIEM, że obecna nazwa jest OK, ale mamy w naszej opinii lepszą propozycję. Absolutnie nie nazwałbym nicpickiem sytuacji, gdy nazewnictwo jest niezrozumiałe lub niespójne np. gdy w projekcie posługujemy się terminem User, a ktoś nagle zaczyna używać Customer.

Dla nitpicków osobiście traktuję dwie strategie:

• Zostawiam komentarz zaczynający się od nitpick: lub nit: z opisem problemu, a najlepiej z blokiem suggestion, który autor PRa może paroma kliknięciami dodać do swojego kodu. Czasami zostawiam pojedynczy komentarz z ogólnym opisem problemu, jeśli miejsc do poprawy jest dużo. Jeśli widzę, że wynika to np. z błędnej konfiguracji toolingu, to zakładam osobny ticket na naprawienie tego.
• Zdarza się też, że świadomie ignoruję takie rzeczy i zakładam, że szkoda czasu na zajmowanie się tym. To jednak mocno zależy od kontekstu konkretnej zmiany. Przykładowo literówkę w lokalnej zmiennej formater (zamiast formatter) prawdopodobnie bym olał. Jednak tę samą literówkę w publicznym API zgłosiłbym jako błąd do poprawy.

Pair code review

Podobnie jak można programować w parze, tak można również robić code review. Takie podejście sprawdzi się świetnie szczególnie przy wdrażaniu zmian na poziomie architektury, implementacji kodu związanego z biznesem czy trudniejszych bugów. Podejście, gdzie autor kodu opowiada o dodanych zmianach, jest analogiczne do metody kaczki wykorzystywanej przy debuggowaniu. Tak przeprowadzone code review pozwala skupić się na najważniejszych aspektach zmian. Dodatkowo czas code review może zostać mocno skrócony. Jeśli autor zmian je tłumaczy, to reviewer nie musi poświęcać czasu na ich samodzielne zrozumienie. W ten sposób można też uniknąć „ping-ponga”, gdzie rozmówcy odbijają wielokrotnie swoje komentarze.

Code review w parze sprawdzi się zarówno, przy jednym komputerze jak i zdalnie z udostępnianiem ekranu. Zachęcam jednak, by faktycznie miało to miejsce w parze. W większym gronie moim zdaniem produktywność i efektywność tego podejścia mocno ucierpi.

Po skończeniu pair code review warto spisać wnioski i zostawić komentarze, tak by w historii został ślad, dlaczego konkretne zmiany zostały wdrożone.

Podsumowanie

Czy robię idealne code review? Nie. Czy chciałbym robić idealne code review? Chciałbym, jest to nierealne, ale jest to kierunek, w którym warto podążać. Ciebie zachęcam do tego samego i mam nadzieję, że ten artykuł Ci w tym pomoże.

Artykuł przygotowałem na bazie moich doświadczeń. Jeśli Twoje doświadczenia są inne, to zostaw komentarz. Chętnie poznam Twoją perspektywę. Zachęcam również do sprawdzenia artykułu o perspektywie osoby tworzącej pull request jako uzupełnienie tematu.

Na zakończenie nieco humoru:

Źródła i materiały dodatkowe

• Google Engineering guide – How to do code review
• LinearB – 2025 Software Engineering Benchmarks Report
• Adam Fałek – 33 najlepsze praktyki Code Review
• Steve On Stuff – Nitpicky Code Reviews Are a Drag
• Front Cave – Poznaj 4 Powody Dlaczego Przy Code Review Kodu Warto Pobrać Lokalnie Brancha Ze Zmianami
• Code Reviews 101 – The Basics
• The Software House – Code review z dwóch perspektyw – #33 Taby vs spacje
• Oskar Dudycz – A few words on communication
• JAK PRZYGOTOWAĆ SIĘ DO CODE REVIEW – zrób to zanim pokażesz swój kod drugiej osobie
• Szymon Przedwojski – Jak robić code review? 1/2: Przygotowanie do review
• Szymon Przedwojski – Jak robić code review? 2/2: Czas na review + bloopers
• Pavneet Singh Saund – Rituals of Shaming in the Software Industry
• Pavneet Singh Saund – Quit blaming others. It’s your fault!
• Daniel Veihelmann – 5 Best Practices for Code Reviews
• Carlo Sales – The Importance of Being a Code Reviewer
• Charles-Axel Dein- Maslow’s pyramid of code review
• Vadim Kravcenko – Healthy Code Reviews
• Magdalena Malinowska-Berggren – Porozumienie bez Przemocy – podstawowe informacje
• 2017 – Sebastian Malaca – Code Review – how to do it better?
• Gergely Orosz – The Software Engineer’s Guidebook str. 153
• Code Review Anxiety Workbook
• Joel Kemp – Giving better code reviews
• Evan Smith – Kind Engineering How To Engineer Kindness
• Sean Hammond – Code Review in Remote Teams
• Philipp Hauer – Code Review Guidelines for Humans
• Jake Zimmerman – Code Review from the Command Line
• Michael Lynch – How to Do Code Reviews Like a Human (Part One)
• Michael Lynch – How to Do Code Reviews Like a Human (Part Two)

Zaobserwuj mnie na LinkedIn!

LinkedIn jest głównym medium, poza blogiem, gdzie publikuję swoje przemyślenia. Zaobserwuj mój profil na LinkedIn, by być na bieżąco!
Zachęcam też do zaobserwowania devszczepaniak.pl na Facebooku, gdzie oprócz informacji o nowych artykułach publikuję linki do ciekawych treści.

Udostępnij wpis

Szukasz taniego i dobrego VPS? Skorzystaj z Mikrusa i przy zakupie odbierz dodatkowy miesiąc usługi za darmo!

📖 Koniecznie sprawdź ofertę księgarni Helion oferującej tysiące książek dla programistów w dobrych cenach! Kupując z tego linku, pomagasz w rozwoju bloga.

Chcesz docenić moją pracę? Postaw mi kawę!