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).

Narzędzia do 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 – 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

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, 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. Dodatkowo od wielu lat jestem zaangażowany w prowadzenie szkoleń dla QA oraz przygotowywanie warsztatów na Quality Excites. 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.