Code Review

Code Review (w skrócie CR) – czyli inspekcja lub przegląd kodu. Systematycznie sprawdzanie i badanie całości, bądź fragmentu kodu, przez członka zespołu, zarówno pod względem ogólnej poprawności, jak i funkcjonalności.

Cel Code Review

Cele Code Review:

• Dla klienta / produktu:
  • znalezienie i naprawa błędów,
  • poprawa jakości kodu (spójność, utrzymywalność, zastosowanie wzorców projektowych i dobrych praktyk).
• Dla zespołu:
  • zwiększenie wiedzy i umiejętności,
  • wymiana wiedzy, zarówno domenowej, jak i technicznej / programistycznej.

Dla kogo jest Code Review

W procesie Code Review może uczestniczyć każdy członek zespołu. W szczególności:

• tester może przeglądać kod innych testów,
• developer może przeglądać kod testów,
• tester może przeglądać kod developerów,
• developer może przeglądać kod innych developerów.

Do Code Review nie jest wymagana znajomość danego języka! Mozna wspomagać się Checklistami (o nich będzie później), które zawierają listę rzeczy, na które trzeba zwracać uwagę podczas przeglądania kodu.

Na Code Review możemy zwracać również uwagę na takie aspekty jak:

• literówki i złe formatowanie,
• wartość na twardo w kodzie,
• niejasne komentarze,
• duplikacje kodu,
• niepełne pokrycie testami (tester może analizować automaty napisane przez developera oraz jakie przypadki zostały w nich pokryte!),
• błędne umiejscowienie kodu,
• błędne zmiany w konfiguracjach.

Również z Code Review płynie wiele korzyści, o których piszemy w dalej części tego artykułu.

Co można przeglądać podczas Code Review

Co można przeglądać podczas (Code) Review poza kodem testów/aplikacji? Tak naprawdę – wszystko 😉 A dokładniej:

• przypadki testowe,
• wymagania,
• wszelkiego rodzaju dokumenty (Manuale, dokumentacja),
• dane testowe,
• strategię testów,
• skrypty testowe/wspomagające,
• definicje budowania/releasów (.gitlab-ci.yml, Jenkinsfile etc.),
• definicje plików dockera.

Typy Code Review

Wyróżniamy kilka typów Code Review. Najważniejsze to…

Formalne

Bazuje na formalnym procesie, którego jedną z popularniejszych implementacji jest Fagan inspection, podczas którego uczestnicy spotykają się aby porozmawiać i analizować wspólnie kod.

Zalety:

• skuteczność,
• udział uczestników o różnych rolach (także np. ekspertów domenowych),
• precyzyjny opis znalezionych defektów (położenie, istotność, typ, moment pojawienia się), dobre statystyki.

Wady:

• pochłania sporo czasu (nawet 1/3 czasu pracy developera), wymaga wielu przygotowań,
• potrzeba przynajmniej 3-6 uczestników, sali, projektora, wydruków.

Nieformalne

Zalety:

• proste – nie wymaga przygotowań,
• nie pochłania tyle czasu co formalne Code Review,
• wykorzystanie narzędzi jak GitLab, GitHub, Crucible itp.

Wady:

• nie da się zapewnić, że CR zostało (w pełni) zrobione,
• brak „gwarancji” naprawienia błędów,
• niewygodne przygotowanie kodu do przeglądu i samo przeglądanie.

Przez ramię

Zalety:

• proste – nie wymaga przygotowań,
• przekazywanie wiedzy, interakcja, nawiązywanie współpracy, budowanie relacji.

Wady:

• brak statystyk, raportu
• łatwo zapomnieć o znalezionych błędach, lub wyciągniętych wnioskach,
• nie ma możliwości weryfikacji, czy znalezione bugi zostały naprawione,
• prowadzone przez autora, nie przez recenzenta, to autor narzuca tok rozumowania i sposób przeglądu kodu,
• recenzent otrzymuje na bieżąco wyjaśniania dotyczące kodu, co utrudnia mu obiektywną ocenę jego jakości (zwłaszcza czytelności).

Kiedy można wykonywać Code Review?

Code Review można wykonywać nie tylko podczas Pull Request/Merge Request 😉

Code Review można wykonywać na każdym etapie realizacji i implementacji danego zadania. Można to robić w formie:

• pair review, mob review lub “przez ramię”
• formalnie lub nieformalnie
• dowolna inna forma

Dzięki uwzględnieniu review wcześniej przed Pull Requesntem, możemy znacznie przyspieszyć proces realizacji zadań 😉

Narzędzia wspomagające Code Review i poprawiające jakość kodu

Zalety:

• umożliwia automatyzację procesu CR,
• przeglądanie kodu staje się wygodne – mamy dostępne różne widoki, proste dodawanie komentarzy, oznaczanie defektów, dodawanie poprawek,
• statystyki mogą być automatycznie generowane,
• można dodać reguły, np. rozwiązanie wszystkich komentarzy lub akceptacja wyznaczonych do tego osób (tzw. opiekunów).

Przykładowe narzędzia:

• GitLab,
• GitHub,
• Crucible,
• statyczna analiza kodu za pomocą narzędzi,
• praktycznie każde narzędzie/repozytorium do przechowywania kodu oferuje obecnie narzędzia do Code Review.

Narzędzia wspomagające statyczna analizę kodu

Narzędzia wspomagające statyczna analizę kodu – Testerze, popraw swój kod! – czyli dwa słowa o pylint.

Również występują narzędzia, które automatycznie formatują (poprawiają) kod, np przy zapisie pliku. Reguły formatowania można dostosowywać do potrzeb projektu i coding standards. Pozwalają na formatowanie typu ustawienie odpowiednich wcięć, usuwanie nadmiarowych spacji, pustych linii, długości wierszy itp. Dla każdego języka programowania i IDE istnieją takie pluginy:

• prettier (JavaScript/TypeScript),
• CSharpier (C#),
• Black lub autopep8 (Python),
• IntelliJ, Pycharm, WebStorm i inne IDE od JetBrains posiadają wbudowane formatery kodu,
• wiele innych.

Korzyści z Code Review

Dla członka zespołu:

• rozwój – nauka od innych, świadomość, że mój kod będzie obejrzany,
• poprawa jakości kodu – ciągłe CRki mojego kodu wskazują, jakie błędy popełniam najczęściej – więc wiem, czego unikać,
• rozwijanie umiejętności czytania kodu,
• lepsze poznanie kodu, z którym pracuję.

Dla zespołu:

• rozwój relacji, dyskusje, poczucie wspólnej odpowiedzialności za tworzony kod,
• wymiana wiedzy,
• lepszy, łatwiejszy w utrzymaniu kod, stopniowe tworzenie coding standards,
• eliminacja silosów wiedzy, lepsza orientacja developerów w kodzie,
• szybsze i skuteczne wdrażanie nowych ludzi w projekt.

Dla klienta:

• lepszy produkt po wprowadzeniu lepszych standardów i wzorców,
• szybszy, sprawny development oraz testowanie,
• mniejsze koszty (wykrywanie potencjalnych problemów znacznie wcześniej).

Code review pozwala wykryć do 85% błędów w kodzie.
Źródło: Static Analysis Works Better with Peer Code Review
– *Capers Jones, Combining Inspections, Static Analysis and Testing to Achieve Defect Removal Efficiency Above 95%, January 2012.

Koszt Code Review

Koszty:

• czas i pieniądze klienta – doświadczenie pokazuje, że w dłuższej perspektywie CR obniża koszty (tym bardziej, im lepiej zorganizowane),
• wysiłek i znużenie zespołu – w fazie aktywnego developmentu liczba CRek może zniechęcić.

Negatywne aspekty Code Review

Efekt „Wielkiego Brata”

Developer może czuć się cały czas obserwowany:

• jak robi CR (czas, efektywność),
• ile błędów jest znajdowanych w jego kodzie.

Wielka liczba znalezionych błędów w kodzie niekoniecznie oznacza, że developer źle pracuje. Powodów można się doszukiwać w:

• skomplikowanym zadaniu, jakiego dotyczy kod,
• skutecznej pracy recenzentów kodu.

Tego typu dane nie powinny być zbierane w odniesieniu do konkretnego developera i wykorzystywane przeciwko niemu. Jeśli to ma miejsce:

• drastycznie spada morale zespołu,
• ludzie nastawiają się negatywnie do CR,
• dążą raczej do poprawy statystyk niż do pisania lepszego kodu.

Efekt (dominacji) silniejszego

Jak się to przejawia:

• dominacja osób bardziej doświadczonych, o silnym charakterze,
• brak wsparcia zespołu dla technicznych liderów (pobieżne review, bo “on to na pewno zrobił dobrze”),
• przesunięcie całej odpowiedzialności za dobre code review na osoby najbardziej doświadczone (“skoro on nie znalazł tu błędów, to na pewno jest ok”).

Efekt agresji

Najczęściej w mniej doświadczonych zespołach mogą pojawić się ataki lub niezrozumienie i odczytanie komentarzy jako ataki. Tego typu sytuacje mogą być przyczyną konfliktów w zespole.

Efekt „pójścia w zaparte” i dyskusji dla dyskusji

Bezproduktywne, jałowe dyskusje dotyczą spraw błahych (używamy “this”, czy nie, spacja przed nawiasem, czy nie itp…)

Rozwiązania:

• systematycznie tworzenie coding standards,
• zachowanie balansu pomiędzy bronieniem swojego zdania, a przyznaniem racji koledze, umiejętność dostrzeżenia, że “to jest sprawa trzeciorzędna”,
• ucinanie dyskusji przez innych uczestników lub posiadanie opiekuna repozytorium lub Lead QA / Lead Developera.

Jak efektywnie robić Code Review

Małe porcje kodu w Merge Requestach

W dużym skrócie – mały Merge/Pull Request oznacza krótkie Code Review, a to przekłada się na łatwiejsze znajdowanie potencjalnych błędów i problemów.

Dlatego warto wprowadzić w projekcie kulturę częstych commitów i małych Pull Requestów.

Nie spędzaj na CR więcej niż godziny (bez przerwy)

• po godzinie efektywność przeglądającego kod drastycznie spada,
• każdy jest w stanie znaleźć tylko ograniczoną ilość bugów i zaproponować skończoną liczbę usprawnień (wynika to z jego doświadczenia, wiedzy, aktualnej dyspozycji…),
• na CRce nie spędzaj też mniej niż 5 minut (nawet jeśli dotyczy jednej linii kodu).

Aby znaleźć więcej błędów, przeglądaj wolniej

Prędkość wykonywania CR:

• maksymalnie przeglądaj 300-500 linii kodu na godzinę,
• optymalnie przeglądaj 150-200 linii kodu na godzinę,
• przy 1000 linii na godzinę skuteczność wykrywania błędów spada niemal do zera.

Zawsze komentuj kod – nie (działanie) autora

• zły komentarz – po co to tak zrobiłeś? przecież jest biblioteka X, która to oferuje,
• poprawny komentarz – Można tutaj skorzystać z metody Y z biblioteki X.

Gdy czegoś nie rozumiesz – zadawaj pytania

• taka forma pozwala dowiedzieć sie więcej o kodzie, zanim sam wysuwasz wnioski.

Fakt znalezienia buga traktuj pozytywnie

Każdy wykryty i poprawiony błąd to:

• mniej problemów znalezionych przy testowaniu,
• mniej błędów zgłoszonych przez klienta.

Bądź konkretny w komentowaniu kodu

Podczas pisania komentarzy w CR:

• pisz jasno i wprost,
• unikaj jałowych dyskusji i skupiania się na nieistotnych detalach,
• proponuj konkretne lepsze rozwiązanie.

Automatyzacja podczas przeglądu kodu

Podczas przeglądu kodu można użyć narzędzi, które automatycznie sprawdzą kod i wyłapują błędy w formatowaniu, niepoprawne składnie, nazewnictwa etc. Dzięki takiemu podejściu zaoszczędzimy czas i będziemy mogli się skupić na funkcjonalności, złożoności oraz poprawności koncepcyjnej kodu.

W GitHub Actions (narzędziu do Ciągłej Integracji), istnieje plugin https://github.com/marketplace/actions/lint-action, który pozwala sprawdzać wystawione Merge Requesty.

Dokumentuj kod

W przypadku wywiązania się dyskusji nad fragmentem kodu, po podjęciu decyzji i wprowadzeniu stosownych poprawek w kodzie, rozważ również następujące akcje:

• dopisanie decyzji do coding standards, jeśli decyzje mają wpływ na ogólny standard pisania kodu w projekcie,
• dodanie odpowiednich komentarzy w kodzie, które ułatwią zrozumienie danych decyzji lub kodu.

Ustal mierzalne cele CR (statystyki i metryki)

Zbierz statystyki, aby usprawnić i poprawić proces. Tylko precyzyjne statystyki pozwalają nam zobaczyć skuteczność CR i tę skuteczność poprawiać.

Zewnętrzne i wewnętrzne statystyki

Zewnętrzne statystyki np.:

• o ile (procentowo) zmalała liczba bugów, zgłaszanych przez klienta.

Wewnętrzne statystyki (procesu):

• ile błędów znajdujemy i w jakich obszarach
• ile czasu developerzy poświęcają na CR.

Co ważne:

• zewnętrzne statystyki to jedyny miarodajny wyznacznik skuteczności CR, ale wewnętrzne statystyki dają szybki feedback i pozwalają na bieżąco poprawiać skuteczność CR i osiągać zamierzone cele,
• statystyki muszą być przygotowywane automatycznie – ręczne zbieranie statystyk prawdopodobnie zakończy się niepowodzeniem (zadanie to jest monotonne, łatwo o błąd, zapomnienie i może być czasochłonne).

Typy statystyk i metryk

Przykładowe statystyki i metryki, jakich można używać w CR:

• LOC (Lines of Code) – liczba linii kodu podlegających CR,
• czas przeglądu (inspection time) – czas poświęcony przez recenzentów na CR,
• współczynnik przeglądu (inspection rate) – czyli jak szybko jesteśmy w stanie przeglądać kod (kLOC/man-hour)
  • 100-200 LOC/hour – dokładny przegląd,
  • 200-500 LOC/hour – średnie wartości,
  • 800-1000 LOC/hour – zdecydowanie za szybko.
• współczynnik defektów (defect rate) – jak szybko znajdujemy defekty (defekt/man-hour) – średnio 5-20 na godzinę,
• gęstość defektów (defect density) – ile defektów znajdujemy w określonej ilości kodu (defects/kLOC)
  • 5 defects /kLOC – w kodzie doświadczonego developera w stabilnym projekcie,
  • 100-200 defects/kLOC – w kodzie juniora w nowym projekcie.
• liczba defektów (defect count) – ile ogólnie znajdujemy defektów w kodzie

Code Review Checklist

Jest to lista, która zawiera elementy, które powinny zostać sprawdzone w czasie CRki (minimalny zestaw rzeczy do sprawdzenia). Najtrudniej odkryć błąd polegający na braku danego elementu lub aspektu – w tym pomaga checklista.
Przykładowa checklista może zawierać:

• czy wszystkie wyjątki są obsłużone,
• czy napisane są odpowiednie testy,
• czy walidujemy odpowiednio parametry metod itp…

Lista powinna być:

• krótka – co najwyżej kilkanaście punktów, związanych z najczęściej występującymi problemami,
• systematycznie przeglądana i aktualizowana – problemy, które często występują w czasie Code Review, powinny zostać umieszczone na liście, natomiast te które występują rzadko powinny zostać z niej usunięte.

Niebezpieczeństwa związane z posiadaniem checklisty:

• tylko to, co się na niej znajduje, zostanie sprawdzone – wszystko inne z dużym prawdopodobieństwem zostanie pominięte.

Różne narzędzia wspierają umieszczenie takiej Checklisty w Merge Request:

• GitLab – Description templates
• GitHub – Creating a pull request template for your repository

Rodzaje

Możemy wyróżnić następujące rodzaje list:

• projektowa, czyli co sprawdzamy w ramach każdej CRki w danym projekcie lub repozytorium,
• indywidualna na potrzeby CR, czyli lista typowych błędów popełnianych przez danego autora, na które recenzenci powinni zwrócić szczególną uwagę
• indywidualna na własne potrzeby, czyli na co ja powinienem zwrocic i o czym zdarza mi się zapominać.

Przykład 1

Prosta lista dla początkujących:

• Do I understand the code?
• Are names of variables correct?
• Are methods parameterized correctly?
• Are coding standards applied?
• Is code doing what was described in the task?
• Are patterns applied? (SOLID, DRY etc.)

Przykład 2

• affects REST tests
• affects UI tests
• affects e2e tests
• affected tests have been executed
• changes require readme update
• boy scout rule has been applied
• types and interfaces are added properly

Przykład 3

• service / library version – is properly set?
• data migrations – are in place, are correct, are there any modifications of existing db migrations?
• tests – are present, sufficient, correct?
• technical debt – created? any possibilities to pay?
• completeness – all required changes done (also in other repositories)?
• impact on import/export – is import/export affected/broken?
• logging – done according to [DOC]
• release notes update – should it be updated? is there any manual step required? or any other additional step?
• documentation – provided changes require docs or README update? significant or questionable details or choices are described in the decision log?

Przykład 4

• Are (business) requirements fulfilled? (Take a look at jira issue / documentation, make sure that reviewed code properly implements these requirements)
• Do tests express requirements/functionality properly? Are they correct, exhaustive, readable?
• Will it work after a merge to develop? (backward compatibility if required, required changes in other services / libraries ready)
• Are data migrations prepared? (sql, elasticsearch etc.)

Is related documentation updated? (including confluence documentation, faq, release notes, jira…)

• Are logs added at proper level with enough information?

Przykład 5

# Code Review Checklist

## Implementation
- [ ] Does this code change accomplish what it is supposed to do?
- [ ] Can this solution be simplified?
- [ ] Does this change add unwanted compile-time or run-time dependencies?
- [ ] Was a framework, API, library, service used that should not be used?
- [ ] Was a framework, API, library, service not used that could improve the solution?
- [ ] Is the code at the right abstraction level?
- [ ] Is the code modular enough?
- [ ] Would you have solved the problem in a different way that is substantially better in terms of the code’s maintainability, readability, performance, security?
- [ ] Does similar functionality already exist in the codebase? If so, why isn’t this functionality reused?
- [ ] Are there any best practices, design patterns or language-specific patterns that could substantially improve this code? 
- [ ] Does this code follow Object-Oriented Analysis and Design Principles, like the Single Responsibility Principle, Open-Close Principle, Liskov Substitution Principle, Interface Segregation, Dependency Injection?

## Logic Errors and Bugs
- [ ] Can you think of any use case in which the
code does not behave as intended?
- [ ] Can you think of any inputs or external events
that could break the code?

## Error Handling and Logging
- [ ] Is error handling done the correct way?
- [ ] Should any logging or debugging information
be added or removed?
- [ ] Are error messages user-friendly?
- [ ] Are there enough log events and are they
written in a way that allows for easy
debugging?

## Dependencies
- [ ] If this change requires updates outside of the
code, like updating the documentation,
configuration, readme files, was this done?
- [ ] Might this change have any ramifications for
other parts of the system, or backward
compatibility?

## Security and Data Privacy
- [ ] What security vulnerabilities is this code susceptible to?
- [ ] Are authorization and authentication handled
in the right way?
- [ ] Is (user) input validated, sanitized, and escaped 
to prevent security attacks such as cross-site 
scripting, SQL injection?
- [ ] Is sensitive data like user data, credit card
information securely handled and stored?
- [ ] Is the right encryption used?
- [ ] Does this code change reveal some secret
information like keys, passwords, or usernames?
- [ ] Is data retrieved from external APIs or libraries
checked accordingly?

## Performance
- [ ] Do you think this code change will impact
system performance in a negative way?
- [ ] Do you see any potential to improve the
performance of the code?


## Usability and Accessibility
- [ ] Is the proposed solution well designed from a
usability perspective?
- [ ] Is the API well documented?
- [ ] Is the proposed solution (UI) accessible?
- [ ] Is the API/UI intuitive to use?

## Ethics and Morality
- [ ] Does this change make use of user data in a way that 
might raise privacy concerns?
- [ ] Does the change exploit behavioral patterns or human
weaknesses? 
- [ ] Might the code, or what it enables, lead to mental 
and physical harm for (some) users?
- [ ] If the code adds or alters ways in which people 
interact with each other, are appropriate measures
in place to prevent/limit/report harassment or abuse?
- [ ] Does this change lead to an exclusion of a certain
group of people or users?
- [ ] Does this code change introduce unjust impact on people, 
particularly those related to sensitive characteristics such as
race, ethnicity, gender, nationality, income, sexual orientation, ability, 
and political or religious belief?
- [ ] Does this code change introduce any algorithm, 
AI  or machine learning bias?


## Testing and Testability
- [ ] Is the code testable?
- [ ] Does it have enough automated tests
(unit/integration/system tests)?
- [ ] Do the existing tests reasonably cover the code
change?
- [ ] Are there some test cases, input or edge cases
that should be tested in addition?

## Readability
- [ ] Was the code easy to understand?
- [ ] Which parts were confusing to you and why?
- [ ] Can the readability of the code be improved by
smaller methods?
- [ ] Can the readability of the code be improved by
different function/method or variable names?
- [ ] Is the code located in the right
file/folder/package?
- [ ] Do you think certain methods should be
restructured to have a more intuitive control
flow?
- [ ] Is the data flow understandable?
- [ ] Are there redundant comments?
- [ ] Could some comments convey the message
better?
- [ ] Would more comments make the code more
understandable?
- [ ] Could some comments be removed by making the code itself more readable?
- [ ] Is there any commented out code?

## Experts' Opinion
- [ ] Do you think a specific expert, like a security
expert or a usability expert, should look over
the code before it can be committed?
- [ ] Will this code change impact different teams?
- [ ] Should they have a say on the change as
well?

Powyższa checklista podzodzi z https://github.com/mgreiler/code-review-checklist

Problem – rozwiązanie

Poniżej przedstawiam kilka przykładowych problemów jakie zauważyłem w projektach, w których pracowałem i konsultowałem. Do każdego problemu podrzucam też przykładowe rozwiązanie. Może znajdziesz wśród nich pomysły jak usprawnić swoją pracę oraz procesy 😉

Pamiętaj, że w każdym problemie należy dobrze zdiagnozować przyczynę, a następnie wybrać odpowiednie narzędzia i działania, które pozwolą zniwelować przyczynę problemu 😉

Również przy zastosowaniu rozwiązania pamiętaj o najważniejszym – metrykach i mierzeniu, czy sytuacja się poprawia czy pogarsza 😉

PS. W poniższych przypadkach dla uproszczenia użyłem słowa recenzent, w rozumieniu osoba, która przegląda kod podczas procesu Code Review.

Gigantyczny Merge Request i tysiące linii do przejrzenia podczas CR

Duże MR są trudne do przejrzenia i wymagają znacznego nakładu czasu i wysiłku ze strony recenzenta. Może to prowadzić do przeoczenia błędów, opóźnień w procesie recenzji oraz frustracji zarówno dla recenzenta, jak i autora MR.

Rozwiązanie
Rozwiązań może być kilka, np.:

• Podział samego zadania na mniejsze
• Wiele Merge Requestów w obrębie jednego zadania
• Ustalanie limitów rozmiaru MR – np. MR nie większy niż 1000 linii (z pominięciem np. plików typu JSON, które zawierają dane testowe, etc.)
• Automatyzacja i narzędzia wspierające proces Code Review

Praca nad dużymi MR i tysiącami linii kodu jest wyzwaniem, ale dzięki odpowiednim praktykom, narzędziom i wsparciu ze strony zespołu można skutecznie zarządzać tym problemem i poprawić jakość procesu Code Review.

Niechęć do wprowadzania zmian

Niektóre komentarze i znalezione problemy w kodzie mogą wymagać wielu dni na wprowadzenie stosownych poprawek.

Zignorowanie poprawek może prowadzić do wzrostu długu technicznego lub pozostawienia błędów w kodzie.

Wprowadzenie niektórych obszernych poprawek może prowadzić do znacznego wydłużenia pracy nad danym zadaniem. A w rezultacie – niedowiezienie na czas danej funkcjonalności.

Rozwiązanie
Należy określić priorytety i ważność danego zadania i poprawek. Osobiście stosuje mechanizm kilku pytań pomocniczych, np.:

• Kiedy jest termin na dowiezienie głównego zadania?
• Co się stanie, gdy nie dostarczymy tego zadania na czas?
• Jak bardzo ważna i krytyczna jest to poprawka?
• Jak kosztowne są zmiany i poprawki do wprowadzenia?
• Czy uwagi są zasadne?
• Co się stanie, gdy jej nie wprowadzimy?
• Czy możemy poprawkę wydzielić do osobnego zadania, które zrealizujemy w późniejszym terminie?

Na podstawie odpowiedzi można podjąć odpowiednie działania, np. wprowadzić zmiany w trakcie realizacji aktualnego zadania lub poprawki wydzielić do osobnego zadania.

Nieodpowiedni ton w komunikacji

Niektóre recenzje mogą być postrzegane jako krytyczne lub obraźliwe, co może prowadzić do frustracji i konfliktów.

Rozwiązanie
Ustanowienie wytycznych dotyczących profesjonalnej komunikacji podczas recenzji kodu oraz zachęcanie do konstruktywnego feedbacku. Warto tutaj rozważyć szkolenia miękkie dotyczące komunikacji, pracy w zespole itp.

Brak czasu na przeprowadzenie Code Review

Współczesne projekty często są w pośpiechu, a deweloperzy mają ograniczony czas na CR. Może to prowadzić do spadku jakości kodu, niespójności, długu technicznego czy błędów.

Rozwiązanie
Planowanie i priorytetyzacja Code Review jako kluczowego elementu procesu. Bazuj na danych – ile czasu zajmuje obecnie CR i do jakiego czasu chcesz zejść? Co zajmuje najwięcej czasu na CR?

Może to również oznaczać zwiększenie liczby osób w projekcie lub wprowadzenie narzędzi automatyzujących pewne aspekty CR (narzędzia do statycznej analizy kodu).

Nieścisłości lub brak dokładnych wytycznych

Brak jasnych kryteriów recenzji może prowadzić do subiektywnych opinii i niejednolitego podejścia.

Rozwiązanie
Opracowanie szczegółowych wytycznych i kryteriów recenzji kodu, które określają oczekiwania dotyczące jakości, wydajności i bezpieczeństwa kodu. Przykładowo można w projekcie wprowadzić Code Review Checklist, Coding Standards i narzędzia do statycznej analizy kodu, które będą aplikowały określone reguły przy analizie.

Niewystarczająca wiedza recenzenta

Jeśli recenzent nie ma wystarczającej wiedzy na temat danej technologii lub zagadnienia, może przeoczyć istotne błędy.

Rozwiązanie
Współpraca między zespołami oraz zapewnienie odpowiedniego szkolenia i rozwoju dla recenzentów. Można również rozważyć rotację recenzentów w zależności od obszaru kodu.

Można też zastosować pair review lub wygospodarować czas, aby osoba miała możliwość realizacji większej liczby zadań w obszarze, w którym wykonuje COde Review.

Zbyt duża liczba komentarzy lub uwag

Jeśli recenzent daje zbyt wiele uwag, może to znacznie wydłużyć realizację danego zadania.

Rozwiązanie
Warto zdiagnozować przyczynę wielu uwag i w zależności od niej zaplanować działania. Np:

• Może należy wprowadzić narzędzia do statycznej analizy kodu, które będą dostępne z poziomu IDE.
• Jeśli zespół nie ma wystarczającej wiedzy do realizacji zadań – może organizacja szkoleń lub pair coding będzie rozwiązaniem.
• Jeśli uwagi są trywialne, może warto wypoziomować recenzentów/uczestników CR, aby skupiali się na najważniejszych aspektach kodu.
• Jeśli MR jest bardzo dużych rozmiarów, może warto lepiej podejść do dzielenia zadań lub wprowadzić kilka MR w trakcie realizacji jednego zdania.
• Klasyfikacja komentarzy w MR jako blokujące lub opcjonalne.

Przeciążenie opiekunów repozytorium / kluczowych recenzentów

Niektórzy recenzenci mogą być przeciążeni pracą, co może prowadzić do opóźnień w przeprowadzaniu recenzji lub braku skupienia.
W niektórych procesach mamy opiekunów repozytorium. Są to osoby z największa wiedzą o danym repozytorium i kodzie. Ich approve (czyli akceptacja danego Merge Requesa) jest wymagany przy mergowaniu zmian.

Rozwiązanie
Monitorowanie obciążenia pracą i dostosowywanie procesu, aby zapewnić równowagę między codziennymi obowiązkami a czasem na Code Review.
Dodatkowo można rozważyć rozszerzenie grona opiekunów. Jednak tutaj kluczowe jest, aby upewnić się, że nowi opiekunowie posiadają odpowiedni skillset i mindset.
Również cały zespół może uczestniczyć w procesie CR, a na dopiero po takim review opiekunowie będą zaglądać do danego MR. W tym momencie większośc uwag powinna zostać zgłoszona i poprawiona, więc opiekunowie powinni mieć znacznie mniej pracy.

Brak zrozumienia celu biznesowego

Recenzenci mogą koncentrować się wyłącznie na technicznych aspektach kodu, nie zawsze rozumiejąc kontekst biznesowy lub wymagania użytkownika.
Rozwiązanie
Edukowanie recenzentów na temat celów biznesowych i wymagań użytkownika. Organizowanie regularnych spotkań między działem biznesowym a zespołem deweloperskim, aby zapewnić jasność i spójność w procesie CR.

Wolne Code Review? Może to problem Pull Requestów

Proces Pull Request Review może opóźniać wdrożenie zmian nawet o kilka dni, co w skrajnych przypadkach może się przeciągać na tygodnie.

Dodatkowo koszt wdrożenia zmian może być bardzo duży, Zmiany wprowadzone na etapie PR Review mogą wymagać znaczących modyfikacji w kodzie, w tym często całej architektury rozwiązania.

Rozwiązanie

• Pair Programming / Mob Programming

  Praca w parach lub w grupie może być efektywnym sposobem na szybkie sprawdzanie kodu. Jest to szczególnie skuteczne w identyfikowaniu błędów i dyskusji nad implementacją kodu. Narzędzia takie jak VS Code Live Share umożliwiają prace zdalne w jednym IDE.

• Ad-hoc code review

  Sugeruje się przeprowadzanie sprawdzeń kodu częściej niż tylko na etapie PR Review, np. po każdym commicie. Można to robić synchronicznie lub asynchronicznie, zgłaszając informacje o nowych commitach na wybranym kanale.

• Cykliczny code review

  Propozycja regularnych spotkań zespołu, na których omawiane są otwarte PR i proponowane rozwiązania. Ma to na celu uniknięcie rozpraszania uwagi i zwiększenie efektywności.

• Lekkie metody projektowania

  Zalecenie skupienia się na zaplanowaniu i projektowaniu rozwiązania przed jego implementacją. Wskazuje się na techniki takie jak Event Storming czy ADR, które pomagają w wczesnym etapie projektowania, co może znacząco zredukować liczbę potrzebnych zmian w procesie PR Review.

• Zrezygnowanie z pełnego przeglądania Pull Requestów

  Kontrowersyjne podejście, które sugeruje większe skupienie się na programowanie w parach, recenzowanie commitów po scaleniu, recenzja w czasie rzeczywistym (wideo rozmowa w grupie i dyskusja nad kodem).

Polecane zasoby zewnętrzne i linki

Poniżej podsyłam cenne materiały, które dokładniej opisują ten problem i sugerują potencjalne rozwiązania:

• Pull Request Review – szybciej, lepiej, inaczej
• Stop doing Pull Requests. Start doing Code Reviews.
• Disadvantages of Pull Requests
• Are Pull Requests Holding Back Your Team?
• Why your team doesn’t need to use pull requests
• Why Pull Requests Are A BAD IDEA
• Thinking About Code Review

Linki i zasoby

• Narzędzia wspomagające statyczna analizę kodu: Testerze, popraw swój kod! – czyli dwa słowa o pylint
• Code Review z użyciem checklisty
• Best Practices For Reviewing Code
• Przykładowe checklisty:
  • https://github.com/mgreiler/code-review-checklist
  • https://duecode.io/blog/the-ultimate-code-quality-checklist/
  • https://smartbear.com/blog/creating-your-code-review-checklist/
  • https://www.evoketechnologies.com/blog/code-review-checklist-perform-effective-code-reviews/

Krzysiek Kijas Senior Software Quality Engineer, Tech Lead, Mentor profil na LinkedIn

Z testowaniem i dbaniem o jakość oprogramowania jestem związany od 2011. Dobre podwaliny techniczne i ugruntowanie wiedzy certyfikatami pozwalają mi swobodnie pływać w tym bezkresnym oceanie technologii 😉 Na co dzień zajmuje się różnego rodzaju testami, poczynając od manualnych, eksploracyjnych, aż po tworzenie frameworków i projektowanie ich architektury. Obecnie jako Tech Lead odpowiadam za architekturę automatów w Playwright, procesy, dobre praktyki, testy na różnych poziomach (DB, UT, API, UI), CI/CD, oraz rozwój członków zespołu. Dbam o rozwój oraz potrzeby zespołu i klienta. Od wielu lat aktywnie uczestniczę w konsultacjach przy projektach różnych firm, świadcząc wsparcie m.in. poprzez mentoring, prowadzenie szkoleń oraz optymalizację procesów. Jako architekt testów pomagam zespołom zadbać o wysoką jakość oprogramowania. Moja ekspertyza obejmuje projektowanie i wdrażanie skutecznych strategii testowania, automatyzację, definiowanie wskaźników jakościowych, wdrażanie metryk. Moje zaangażowanie skupia się na doskonaleniu spełniania potrzeb klientów poprzez efektywne mentorowanie zespołów oraz zastosowanie lepiej dopasowanych narzędzi i podejść. Ciągły głód wiedzy i silna potrzeba rozwoju spowodowały, iż razem z Przemkiem stworzyliśmy inicjatywę jaktestowac.pl, która była naturalnym krokiem w mojej ewolucji. Dzięki niej, mogę realizować i rozwijać swoje pasje związane z mentoringiem oraz pomocy innym w zdobywaniu wiedzy. W wolnym czasie z pełnym poświęceniem oddaje się swoim uzależnieniom - treningom siłowym, crossfitowi, literaturze i szeroko pojętej sztuce.