PR Review - Usprawnienia - Inne podejścia Code Review
Wspólnie z Krzysztofem Witczakiem stworzyliśmy serię artykułów, w których zastanawiamy się skąd tak duża popularność Pull Request Review, oraz jakie problemy Pull Request Review tworzy w naszych zespołach. Następnie podjęliśmy temat rozwiązywania kwestii PR Review i mierzenia, czy jest lepiej.
W serii PR Review:
Usprawnienia - Inne podejścia Code Review [TEN ARTYKUŁ]
W poprzednim artykule omówiliśmy, jak mierzyć usprawnienia dookoła PR Review, dziś zajmiemy się kolejnym krokiem - jak wprowadzać je w życie.
Rozwiązania na Code Review
Żadna opisanych opcji nie jest oczywiście silver bulletem na wszystkie omawiane przez nas problemy – zachęcamy, aby zastanowić się, które z nich występują w Twoim zespole i zbudować zgraną kompozycję z kilku- poniższych propozycji.
Pair programming
Rozwiązaniem najczęściej rekomendowanym przez popularnych mentorów takich jak Dave Farley czy Martin Fowler jest pair programming. Dla osób słyszących o programowaniu w parach po raz pierwszy polecam krótkie wprowadzenie z artykułu On Pair Programming ze strony Martina Fowlera:
Pair programming essentially means that two people write code together on one machine. It is a very collaborative way of working and involves a lot of communication. While a pair of developers work on a task together, they do not only write code, they also plan and discuss their work. They clarify ideas on the way, discuss approaches and come to better solutions.
The Driver is the person at the wheel, i.e. the keyboard. She is focused on completing the tiny goal at hand, ignoring larger issues for the moment. The Navigator is in the observer position, while the driver is typing. She reviews the code on-the-go, gives directions and shares thoughts.
To co jest istotne z perspektywy PR Review to kwestia code review:
Pair programming liczy się tak jako approval, a programiści stają się ko-autorami rozwiązania również w kontroli wersji.
Tak przeprowadzone review jest również akceptowane przez reguły compliance i instytucje regulujące – informacja od Woutera Lagerweij.
Istnieje kilka zasad, które pomagają wprowadzić programowanie w parach do zespołu:
Warto rozpocząć od krótkiej sesji tłumaczącej złote zasady pair programmingu. Łatwo się krytykuje czyiś kod asynchronicznie. Kiedy robimy to na face-to-face to trzeba umieć wymieniać się spostrzeżeniami.
Warto eksperymentować z rotacją par i dedykowanym czasem na pair programming.
Znamy również dobrze działające przykłady z lekkim podejścia polegającym na ad-hoc synchronizacjach na slack huddle. Osoba chętna na dołączenie do sesji pair programmingu wchodzi na określony kanał dając sygnał reszcie zespołu, że można do niej dołączyć.
Pair programming dużo łatwiej wdraża się oczywiście w środowisku biurowym niż zdalnym. Do pracy zdalnej konieczne jest zastosowanie nowszych narzędzi. Na szczęście w ostatniej dekadzie powstało ich sporo (np. wtyczki do popularnych IDE)
Pair programming nie jest metodą, którą będziecie w stanie zastosować na 100% i w każdym momencie pracy. Błędem jest natomiast odrzucanie tej metody bez podjęcia jakiejkolwiek próby!
Non-blocking continuous code reviews
Klasycznie PR review dzieje się tuż przed mergem zmian. Z tego właśnie powodu pojawia się potrzeba, aby review był szybki:
Inny developer może być zablokowany bez tych zmian,
Biznes naciska, aby przeprowadzić szybko pierwsze testy i walidacje czy pomysł jest dobry.
Skuteczne zespoły świadome tego problemu dzielą scope rozwiązania na dwie sekcje:
Core - część zadania, która musi być dostarczona, aby odblokować biznes, robimy po niej release v1.0 - funkcja ma ograniczenia, może być schowana za feature flagą, może mieć błędy, kod może nie być najlepszy – ale biznes jest już w stanie weryfikować jej przydatność w praktyce.
Usprawnienia – rzeczy ważne, ale nieblokujące biznesu, dostarczamy je po release 1.0 w kolejnych wydaniach, tutaj często umieścimy skomplikowane reagowanie na rzadkie sytuacje, edge-case’y, usprawnienia jakościowe, security, wydajnościowe, dodatkowe testy.
Jeżeli pomyślimy o jakości kodu w oprogramowaniu zawsze można coś usprawnić, ulepszyć, poprawić, zmienić. Kod nigdy nie jest skończony, nigdy nie mamy pewności, czy nie ma w nim błędów, edge case’ów.
Idąc za tą teorią, PR review skupiony przede wszystkim na poprawianiu jakości i dzieleniu się wiedzą również nie musi być etapem blokującym dla biznesu. Jeżeli wyobrazimy sobie “code review” jako kolumnę w kanbanie tuż przed releasem rozwiązania, to teraz zostaje ona przeniesiona na moment po release.
Jak to działa w praktyce?
Zamiast przeprowadzania review przed każdym PR, zespół spotyka się na kilka godzin raz w tygodniu i przegląda kod razem.
Można eksperymentować ze spacerowaniem po codebase poprzez IDE i dyskusje na temat ostatnich epików, zamiast ograniczać się do zmian commit po commicie.
Wykonujemy review patrząc na kontrolę wersji lub korzystając z narzędzi wyspecjalizowanych w tym celu takich jak np. JetBrains Space, Atlassian Crucible, ex-remit, reviewboard.
Warto poświęcić trochę czasu na spojrzenie na epiki, które wdrożyliśmy kilka tygodni temu i zweryfikowanie, czy nasze hipotezy się sprawdziły - może ostatnie założenia dotyczące architektury i abstrakcji są niepoprawne?
Zauważ, że taka regularna sesja pozwala nam wejść w zagadnienia jakości znacznie głębiej. Z tego typu spotkania powinniśmy wrócić z szeregiem pomysłów na usprawnienia. Niewykluczone, że zajmą one wiele dni, ale biznes nie jest tym aż tak rozgoryczony, bo otrzymał już rozwiązanie, z którym może w międzyczasie eksperymentować.
Czy to jest realne do wprowadzenia? Thierry de Pauw opisuje takie podejście w swoim artykule Non Blocking Continuous Code Reviews, a case study i prezentacji na tej podstawie:
Podejrzewamy, że zastanawiasz się - ale co z funkcją PR review polegającą na wyłapywaniu błędów i znaczących nieporozumień, rzeczy poza samą jakością kodu? Jak ograniczyć ryzyko? Proponujemy zmixować tą metodę z innymi zaproponowanymi w tym poście 😊
Dev Carousel
Przykładem na zastosowanie cyklicznego sprawdzania kodu przed wdrożeniem dzieli się w swoim artykule Dev Carousels Maciej Jędrzejewski:
Besides swarming, pair and mob programming and other things that I described in this article, there is still a place for something called a Dev Carousel.
This is a regular, as flexible as possible, daily meeting attended only by technical people where we talk about technical issues related to architecture, structure, problems, patterns, strategies, implementation, and many, many more. Like coffee and cars :)
Spotkania te odbywają się z zasady rano, trwają od 30 minut do godziny i uczestniczą w nich wyłącznie osoby techniczne. Omawiane są kwestie dotyczące architektury, struktury kodu, wzorców projektowych oraz bieżących problemów i strategii.
Spotkania te mają na celu skrócenie czasu przetwarzania pull requestów. Sprawdzamy kod na bieżąco, co prowadzi do poprawy jego jakości oraz zwiększenia efektywności pracy zespołu. Na końcu mamy kod sprawdzony, czyli gotowy do wdrożenia.
Maciej podkreśla również, że Dev Carousel sprzyja integracji zespołu, tworząc przestrzeń do wymiany doświadczeń i wiedzy technicznej. Regularność i elastyczność tych spotkań pozwalają na bieżąco monitorować stan projektu.
Ship / Show / Ask
Możemy również spojrzeć na ten problem z zupełnie innej perspektywy. Jak często zdarza nam się angażować PR code review do zmiany jedno-wyrazowego typo, zmiennej czy innego buga? Czy w takiej sytuacji blokada, review i approval zawsze są niezbędne? Na pewno są sytuacje, gdy taki fix jest wart podzielenia się, ale z naszego doświadczenia wynika, że często robimy to tylko dla formalności.
Rouan Wilsenach proponuje metodę Ship / Show / Ask, która pomaga nam ograniczyć niepotrzebne PR review i wdrożyć ten proces stopniowo:
Ask: Pytaj o review, potem merguj
To jest nasza klasyczna metoda PR review, którą staramy się ograniczyć, ponieważ jest kosztowna.
Jak i kiedy ją stosować?
Pytaj o review celowo, a nie losowo i przez automat – to kosztowna metoda, więc stosujmy ją odpowiedzialnie, wymaga Twojego self-review i przygotowania.
Pytaj o konkretne obszary, szczególnie te związane z większym ryzykiem czy niepewnością.
Kiedy dotykasz kodu, którego nie znasz, jest bardzo skomplikowany, wpływa na wiele obszarów albo doprowadzi do trudno odwracalnych zmian.
Nie proś o approval, tylko o sprawdzenie jakości.
Show: najpierw merguj, potem pokaż i poproś o feedback
Szukaj okazji kiedy możesz zbudować zaufanie poprzez pokazanie, że nic złego się nie stało - jesteś w stanie dowieźć szybko rozwiązanie, ale jednocześnie tworzysz pole do dyskusji, natomiast nie jest ona dla nikogo blokująca. Świetnie się sprawdza do dzielenia się wiedzą.
Jak tę metodę stosować?
Pokaż interesujący, nietypowy błąd już po jego naprawieniu.
Przedstaw ciekawe rozwiązanie dla reszty, które jest FYI a nie blokerem.
Poproś o feedback na ewentualny refactor czy optymalizację, którą można wdrożyć chwilę później.
Ship: po prostu merguj
Wprowadzasz zmianę natychmiastowo bez zakłócania pracy innych osób, najlepiej w sytuacjach, gdzie review jest wyłącznie formalnością.
Kiedy ją stosować?
Naprawienie niewielkiego błędu, typo, przeoczenia - czegoś oczywistego, co nie wymaga specjalnej konsultacji
Dodanie prostej akcji CRUD’owej zgodnie z zasadami opracowanymi przez zespół, nic nowego, mamy dużą pewność tej zmiany
Aktualizacja dokumentacji albo pliku konfiguracyjnego dla dokumentacji
Usprawnienia w oparciu o feedback reviewera
Dodanie brakującego testu do już istniejącego suite’a
Zastosowanie prostej zasady scoute’a gdy zauważyliśmy ku temu potencjał
Kosmetyczne zmiany na UI
Wiele firm eksperymentuje też w ten sposób z małymi aktualizacjami zależności
Mogłoby się wydawać, że zmian typu “Ship” i “Show” powinno być mało, ale w praktyce jest ich całkiem sporo. W swoim artykule Rouan przedstawia kilka dodatkowych zasad i sugestii dla zespołów, z którymi warto się zapoznać.
Luca Rossi opublikował też niedawno artykuł o bliźniaczej metodzie “Automate / Defer / Pair”, w którym twierdzi, że w zdrowej organizacji najwięcej zmian powinno być typu “non-blocking review” w postaci “Defer”. Co ciekawe, alternatywą dla “Ask” w jego formule jest “Pair”… co dobrze łączy się z innymi pomysłami w naszym poście.
Jak sprawić by klasyczne PR Review było mniej uciążliwe?
Nie zawsze mamy niestety możliwość wprowadzenia w życie opisanych alternatywnych rozwiązań. Dlatego w kolejnej części serii przyglądamy się innej kategorii metod - tym służącym złagodzeniu bólu PR Review:
PR Review - Usprawnienia - Łagodzenie bólu Code Review