Czy Code Review jest elementem procesu, które daje Tobie lub Twojemu klientowi korzyści? Czy robisz go tylko dlatego, żeby przepchać notkę na testy? A może uważasz, że Code Review to tylko marnowanie czasu?
Code Review wykonywane na ślepo „bo szef mi kazał” nie ma większego sensu. Natomiast my mamy zamiar pokazać, że Code Review może być bardzo wartościowe, jeżeli podejdzie się do tego z głową i ze zrozumieniem celów, jakie chcemy osiągnąć.
1. Jak nie robić Code Review
Bardzo wiele złych opinii odnośnie Code Review wynika z tego, że cel tego Code Review jest nieznany. Sami widzieliśmy sporo dysfunkcji takich jak:
- Oznaczenie 60 razy „brak kropki w komentarzach” oraz jednej faktycznie ważnej rzeczy. Ta rzecz została przeoczona.
- Oznaczenie fragmentów kodu bez wyjaśnienia czemu te fragmenty są oznaczone, tylko komentarz „to musi być naprawione” – brak jasnego sprecyzowania problemu.
- Przyczepianie się do nazw metod i zmiennych. To może być istotne, ale jest różnica pomiędzy naprawą bardzo złej nazwy a zmianą jednej trudnej nazwy na drugą, nie mniej trudną – np. „ModelFailureInterceptor” zmieniony na „ErrorInterceptor”.
- Nadużywanie autorytetu i skupianie się na tym, by pokazać komuś, że nie potrafi programować.
- Kłótnie i wszelkiego typu konflikty emocjonalne zamiast argumentacja pod kątem korzyści i kosztów
- Długie i “rozmemłane” code-review, męczące dla wszystkich uczestników.
Nic dziwnego, że mając powyższe doświadczenia Code Review może nie kojarzyć się dobrze. Jednak nie warto wylewać dziecka z kąpielą. Code Review można przeprowadzić tak, by zadziałało.
2. Jak zrobić udane Code Review
2.1. Cel oraz potrzeba
Co najważniejsze – osoby robiące Code Review muszą mieć potrzebę dbania o stan kodu aplikacji. Jeżeli im na tym nie zależy, Code Review nie będzie udane - korzyści mogą się po prostu nie zmaterializować.
Załóżmy, że taka potrzeba istnieje w Twoim zespole. W tym momencie dobrze zrobione Code Review staje się pożądaną praktyką. Jak zatem zrobić Code Review, by się udało?
Przede wszystkim, należy pamiętać, że celem Code Review jest analiza kodu. W Code Review nie chodzi o ocenę czy krytykę osoby piszącej ów kod. Jeżeli ktoś poczuje się zaatakowany i zacznie się bronić przy Code Review, będzie bardziej skupiać się na zachowaniu twarzy a mniej na wspólnym budowaniu najlepszego możliwego rozwiązania.
Dodatkowo, osoby oceniające kod (audytorzy kodu) i autor kodu muszą skupiać się na najważniejszych tematach. Zamiast skupiać się na drobnostkach, wszyscy powinni dyskutować o tym, co jest ważne i co nie może być sprawdzone automatycznie.
Odnośnie sprawdzania automatycznego i sprawdzania tylko tego, co ważne - wszystko, co można oddelegować maszynie należy oddelegować maszynie. Tu właśnie wchodzą wszelkie lintery, formattery i narzędzia statycznej analizy kodu.
Co więcej, audytorzy kodu i sam autor kodu muszą wiedzieć, pod jakim kątem robią Code Review. Tu bardzo pomocne okazują się coding standards i checklisty - instrukcje na co konkretnie zwracać uwagę i jak kształt kodu powinien wyglądać.
Uczestnicy Code Review powinni się skupić na merytoryce kodu i na rzeczach niemożliwych do oceny przez maszynę. Na tym, gdzie znajduje się wartość tego kodu.
2.2. Procedura
Jeżeli zmiana jest nieduża i stosunkowo prosta, kilka osób może równolegle, niezależnie od siebie dodawać komentarze. Sprawdzi się to w zupełności.
Przy dużym rozmiarze zmiany warto rozważyć inne podejście. To, co u nas zadziałało w bardzo dużym projekcie to następująca procedura, luźno wzorowana na formalnych zasadach przeglądu kodu z ISTQB:
- W Code Review uczestniczą trzy osoby. Dwie oceniające kod i jeden autor.
- Przed Code Review osoby oceniające kod powinny się z nim zapoznać.
- Autor przeprowadza oceniających przez swój kod i przede wszystkim skupia się na tym co zrobił, czemu to zrobił i dlaczego podjął takie decyzje.
- Oceniany jest zarówno kod jak i testy mają wykazać jego poprawność i sprawdzające przypadki brzegowe
Nie jest to tak czasochłonne jak się wydaje – osoby oceniające kod i tak spędziłyby ten czas czytając ów kod. Co więcej, zajęłoby to jeszcze więcej czasu – bez narracji autora trudniej byłoby się domyślić jaka jest intencja analizowanego kodu.
2.3. Słowo o testach
Testy zawsze podlegają przeglądowi a ich brak jest podstawą do odrzucenia kodu. Jeżeli nie da się jednoznacznie udowodnić, że kod działa przynajmniej w ramach założonych parametrów to nie powinien nigdy trafić na produkcję.
To rozwiązuje jeden z głównych problemów miary „pokrycia kodu”. W naszym przypadku pokrycie kodu jest mierzone automatem – to sprawia, że bardzo kuszące jest pisanie testów, które mają maksymalizować to pokrycie.
Dzięki przeglądom kodu ludzie analizują te testy i sprawdzają, czy są sensowne. W ten sposób miara pokrycia kodu nie zawiera testów, których jedynym celem jest podniesienie tego pokrycia.
Tak więc połączyliśmy automatyczną miarę “pokrycie kodu” z ludzką weryfikacją sensowności testów i otrzymaliśmy miarę “pokrycie kodu sensownymi testami”.
3. Po co warto robić to wszystko?
Powyższe brzmi jak bardzo dużo pracy, jeśli porównać to z prostym „commit – merge – push” i włączeniem kodu na produkcję. Jednak korzyści z Code Review sprawiają, że zdecydowanie warto się tym zająć.
Jakich korzyści można się więc spodziewać?
4. Zarządzanie złożonością kodu
4.1. Zależności i nieczytelność
Za „A philosophy of software design” Ousterhouta, problem z utrzymaniem kodu i programowaniem jest pochodną złożoności tego kodu.
Samą złożoność kodu można rozbić na dwa główne czynniki:
- Zależności (dependencies) – dany fragment kodu jest niemożliwy do stworzenia lub zmodyfikowania w izolacji; przy modyfikowaniu kodu trzeba rozpatrzyć coś więcej niż tylko sam kod, który chcemy zmodyfikować.
- Nieczytelność (obscurity) – informacja nie jest oczywista, nie do końca wiadomo co należy zrobić (błędne nazwy, nieznajomość API, nieoczywista architektura)
Na przykład, skonfigurujmy Gitlab CI dla aplikacji w .NET Core. Żeby to zrobić, musimy wykorzystać następujące zależności:
- Docker
- Test runner .NET Core + NUnit
- Mechanizm sterowania serwerem CI w Gitlabie przy użyciu yamla.
Za pierwszym razem jak próbowaliśmy to zrobić to nie było to przyjemnym doświadczeniem. Zupełnie nie mieliśmy pojęcia jak się za to zabrać (szczęśliwie, Gitlab zapewnił dobrą dokumentację).
Ostatecznie, zrobiliśmy to w niecałych 30 linijkach tekstu – ale na początku nie mieliśmy pojęcia jak w ogóle się zabrać za te 30 linijek (nieczytelność).
Dzięki Code Review można stosunkowo łatwo wykryć złożoność, która wynika z zależności i nieczytelności - zwłaszcza, jeśli Code Review robi ktoś, kto zna dane narzędzia wystarczająco dobrze. Można też łatwo usunąć taką zbędną złożoność.
Jako przykład - ten nasz nieszczęsny skrypt yamlowy sterujący Gitlab CI. Jesteśmy pewni zarówno tego, że w zbudowanym przez nas skrypcie są niepotrzebne instrukcje jak i tego, że da się to zrobić prościej.
Gdyby teraz osoba trzecia znająca się na tym spojrzała na nasz skrypt, byłaby w stanie pokazać jak zrobić to lepiej. Takie Code Review skryptu nie tylko by go usprawniło - dodatkowo, my byśmy się nauczyli jak robić takie rzeczy lepiej w przyszłości.
Dokładnie ta sama korzyść dotyczy kodu produkcyjnego. Być może napisany algorytm da się uprościć. Być może ktoś napisał coś, co już było dostępne gdzieś w aplikacji. No i łatwo zlokalizować coś w stylu „ktoś dodał nową metodę do UtilityHelpera, którego próbujemy się pozbyć”.
4.2. Habitability
Swego czasu Richard Gabriel opisał własność kodu którą nazwał habitability („mieszkalność”).
W skrócie, habitability to miara tego, jak bardzo programista czuje się w danym kodzie jak w domu. Jest to więcej niż tylko czytelność – jest to poczucie pewności i bezpieczeństwa, że kod jest zrozumiały i programista może łatwo go zmienić.
Dzięki Code Review można zlokalizować takie fragmenty kodu, które dla odbiorców raczej przypominają dżunglę pełną tygrysów niż są przyjaznym i wygodnym domem. Jako konsekwencję, można podnieść habitability.
Jak?
- Powinniśmy pilnować spójności nowo dodanego kodu z architekturą systemu.
- Powinniśmy szukać błędów i problemów, których nie wykryje kompilator
- Powinniśmy sprawdzić, czy istnieje żywa dokumentacja w formie testów
Przykładowo, warto spojrzeć na problem fabryk i konstruktorów przy programowaniu.
W programowaniu obiektowym czasem pojawiają się złożone obiekty, które mają sporo zależności i trudno je przetestować.
Np. – nie może powstać obiekt bez odpowiednio silnej walidacji, bo to zagrażałoby spójności zarządzania pieniędzmi. Ale jeżeli umieścimy walidację w konstruktorze, zbudowanie takiego obiektu do testów staje się koszmarem.
Jednym z rozwiązań jest wykorzystanie metody fabrycznej i wprowadzenie zasady – „w kodzie produkcyjnym nie wolno używać konstruktorów. Wolno wykorzystywać tylko fabryki”.
Problem polega na tym, że jeżeli jedna osoba w zespole nie będzie przestrzegać tej zasady to sytuacja jest dużo gorsza niż gdybyśmy mieli walidację w konstruktorach. Czyli to jest problem ludzki, nie techniczny. Wiemy jak to naprawić, ale nie wiemy jak zapewnić by to się działo.
Nie wiemy?
Żeby rozwiązywać takie problemy wykorzystuje się właśnie Code Review. W podanym przykładzie gdy w kodzie zostanie znaleziony konstruktor, to w tym momencie można przedyskutować sytuację.
Jeśli ten błąd popełnił nowy programista, tym szybciej wdroży się w projekt – nie tylko dowie się co należy zrobić ale i dlaczego tak w tym projekcie robimy.
A dodatkowo Code Review zapewnia, że potencjalnie niebezpieczny błąd nie pojawi się w aplikacji.
5. Sprawdzaj najczęściej pojawiające się uwagi automatycznie
Wcześniej napisaliśmy, że nie powinno się w Code Review sprawdzać rzeczy typu „kropka w komentarzu” czy „spójność z ogólnie przyjętym stylem programowania” – np. camelCase, PascalCase, taby i spacje. Jednak te rzeczy są ważne i silnie wpływają na habitability. Jak zatem to pogodzić?
Większość rzeczy tego typu są łatwe do wyłapania przez automat i narzędzia statycznej analizy kodu. Wystarczy odpowiednio skonfigurować ustawienia i zapewnić, że cały zespół używa jednoznacznych ustawień.
Ważne jest to, że jeżeli nie zrobi się tego na początku projektu to po uruchomieniu monitorów statycznej analizy będziemy mieli do czynienia z 98742308 problemami do rozwiązania. Lepszym rozwiązaniem będzie wtedy wyłączenie mniej istotnych reguł i skupienie się na tych, na których naprawdę nam zależy – i konsekwentne zmniejszanie ich liczby. Alternatywnie, można sprawdzać zmiany tylko dla nowych zmian, pomijając stare, znane problemy.
Narzędzia statycznej analizy kodu zwykle pozwalają na dodawanie własnych reguł. Jeżeli podczas Code Review często pojawiają się pewne problemy oraz te problemy są możliwe do identyfikacji przez narzędzie statycznej analizy kodu, to warto dodać nową regułę.
W ten sposób ludzie nie muszą pilnować stylu programowania. Mogą robić to dla nas maszyny. Wtedy od razu widać, kiedy naruszamy zasady.
6. Trendy w Code Review
6.1. Trendy?
Jedną z największych zalet Code Review jest nie tylko to, co Code Review przynosi samo w sobie.
Jeżeli zbierasz informacje z Code Review na przestrzeni czasu to umożliwiasz śledzenie trendów w Twojej aplikacji. I na bazie tych trendów jesteś w stanie podjąć odpowiednie decyzje.
6.2. Przykład wykorzystania trendów
Swego czasu w pewnym projekcie Code Review zaczęło zajmować coraz więcej czasu. Przeciętny czas od momentu stworzenia pull requesta do momentu merge wzrósł średnio o 30%. Jednocześnie zwiększyła się ilość Code Review, które trwały poniżej 30 sekund.
Taka sytuacja pokazywała, że zaczęliśmy mieć problem z przeprowadzaniem Code Review w tym projekcie.
- Wzrost bardzo szybkich Code Review świadczył o tym, że najpewniej ludzie po prostu chcieli mieć to z głowy i nie przeprowadzali Code Review prawidłowo – po prostu akceptowali zmiany by wrócić do swoich zadań. My wiemy, że częścią problemu było duże ciśnienie na dostarczanie funkcjonalności, ale to nie jedyna możliwa przyczyna.
- Wzrost czasu od momentu stworzenia do momentu merge przy braku aktywności w pierwszych dniach Code Review oznaczały, że niewiele osób chciało się zabrać za Code Review. Alternatywnie, ludzie byli tak obciążeni, że nie mieli czasu.
Poszperaliśmy chwilę i okazało się, że średnia ilość plików wymaganych do przejrzenia w pojedynczym Code Review się podwoiła w okresie kilku miesięcy.
Albo typ zadań się zmienił i zaczęliśmy pracować nad poważną przebudową systemu albo dogonił nas dług techniczny i zaczynamy mieć problemy z architekturą.
Po sprawdzeniu przeszłych zadań i paru innych miar sprawa stała się dość jednoznaczna – kilka miesięcy temu trzeba było przygotować materiały na demonstrację dla klienta i to doprowadziło do uszkodzenia struktury projektu.
Mając tego typu miary dużo łatwiej było przekonać osoby decyzyjne, że ta sytuacja wymagała interwencji i w tym projekcie trzeba wpierw naprawić sytuację zanim można mówić o dalszych poważnych zmianach.
To bardzo ważne – jeżeli się coś zmieniło w jakichkolwiek miarach czy trendach, to ta zmiana odzwierciedla zmianę stanu projektu, procesu bądź zespołu. Zawsze są pewne odchylenia i anomalie, ale w perspektywie długoterminowej trend staje się widoczny.
6.3. Jak można interpretować poszczególne trendy?
Kilka przykładowych miar i sposobów ich interpretacji, na zachęcenie:
1. W ramach Code Review średnia ilość plików zmienianych zaczyna rosnąć.
Najprawdopodobniej nasza architektura przestaje działać poprawnie, albo kiepsko rozbijamy zadania i są za duże. To znaczy, że mamy problem z planowaniem.
2. Ciągle zmieniamy te same pliki.
Najpewniej te pliki robią za dużo – trzeba pomyśleć o ich zakresach odpowiedzialności.
3. Ilość odrzuconych Pull Requestów rośnie.
To jest bardzo ważna miara. Co to znaczy „odrzucony Pull Request”? To jest wykonana praca, która nie została dołączona do aplikacji. Innymi słowy, jest to zmarnowany czas. Jest to sygnał alarmowy i temu należy się natychmiast przyjrzeć. Być może to nic strasznego - nie każdy tak zwany Proof Of Concept będziemy akceptować, ale jeśli dotyczy to zadań “zwykłych” to możemy mieć poważny problem.
4. Code Review zaczyna zajmować coraz więcej czasu.
Albo mamy problem z przeprowadzaniem Code Review, albo mamy za dużą ilość zwrotek. Być może Code Review wisi i nikt na to nie spojrzał? Być może ludzie są przeciążeni własną pracą albo nie widzą korzyści z przeprowadzania Code Review?
Załóżmy, że Code Review trwa tydzień i faktycznie tam jest aktywność – pojawiają się komentarze, zmiany itp. Albo kod jest kiepskiej jakości i ktoś się po prostu uczy, albo np. kiepsko przedyskutowaliśmy sprawę na początku i nie było dość dobrego zrozumienia wymagań.
W ten sposób raz udało nam się przy użyciu analizy trendów CR znaleźć problemy związane z dokumentacją na wejściu.
Jeżeli ludzie nie chcą po prostu robić Code Review, to czemu? Może nie mają dość czasu? Może nie widzą wartości? Może to wyjątkowo upierdliwe lub nieprzyjemne?
Tak czy inaczej – trzeba to naprawić; najpewniej trzeba będzie zmienić proces.
5. Ilość plików i zmian w Code Review rośnie
Ile zmian jesteś w stanie ogarnąć w ramach jednego Code Review? Ile jesteś w stanie zrobić zanim zaczniesz się męczyć i popełniać błędy?
W chwili, w której ten trend rośnie w połączeniu z trendem „ilość błędów znalezionych po CR” – trzeba coś zrobić, by ilość zmian na zadanie była mniejsza.
Albo musimy zmniejszyć wielkość zadań albo musimy zmienić coś w strukturze kodu.
6. Pokrycie kodu testami (Code Coverage)
To jest interesujący przypadek. W naszym projekcie wprowadziliśmy zasadę, że pokrycie kodu testami nie może spadać – jeśli jest 75%, to nie może spaść do 74%. Jednocześnie podczas samego Code Review pilnujemy, aby testy były sensowne i nie były pisane tylko po to, by zwiększać pokrycie kodu.
Dzięki temu pokrycie kodu testami może iść tylko w jedną stronę – w górę. A same testy będą sensowne.
Ogólnie:
Jak zapewne widzisz, trendy i miary dają sporo możliwości interpretacji stanu zdrowia systemu.
6.4. Jak zbierać takie dane?
Takie dane najlepiej zbierać automatycznie.
Część danych znajduje się w Gicie (czy odpowiednim systemie kontroli wersji) i są dostępne przez odpowiednie połączenie polecenia git log oraz grep; tego typu tematy omawia świetna książka „Software Design X-Rays”.
Część danych można pobrać z narzędzia do Code Review – odpytać odpowiednie API. Przykładowo, dla GitHuba.
Dane o złożoności cyklomatycznej itp. możecie wygenerować odpowiednim narzędziem, np. w wypadku Pythona - radon.
Dane o przeszłych zadaniach znajdują się w notkach lub ich odpowiedniku.
Gdy macie już przygotowane wszystkie interesujące Was dane to warto zbudować prosty skrypt agregujący to wszystko i wyświetlający te dane w formie interesujących Was agregacji.
7. Podsumowanie
7.1. Code Review a narzędzia statycznej analizy kodu
Jeżeli wykorzystujesz Code Review do znalezienia braku kropek w komentarzach czy do uspójnienia camelCase w projekcie to marnujesz czas dobrze opłacanych specjalistów. Do tego służą narzędzia statycznej analizy kodu – są tańsze i zdecydowanie skuteczniejsze.
Code Review warto wykorzystać do znajdowania rzeczy, które są bardzo trudne do znalezienia przez narzędzia statycznej analizy kodu. Code Review to okazja do dialogu, uczenia się od siebie nawzajem i pilnowania spójności architektonicznej, habitability i innych rzeczy na których Ci zależy.
7.2. Udane Code Review ma znany cel i jest dialogiem
Udane Code Review nie polega na tym, że programiści się zwalczają czy przekrzykują. Nie dość, że wszyscy polegają na tych samych zasadach to jeszcze autor przeprowadza audytorów kodu przez swoje rozumowanie.
Audytorzy pomagają autorowi zwrócić uwagę na te rzeczy, które są ważne dla zdrowia projektu i które są niemożliwe do sprawdzenia przez statyczną analizę kodu (np. sensowne testy). Rolą audytorów jest zapewnić dodatkowe pary oczu i chronić projekt przed potencjalnymi błędami i przyszłymi problemami.
7.3. Wartość Code Review
Dzięki Code Review można pozbyć się problemów wynikających ze złożoności przypadkowej – jeżeli ktoś nie wie jak coś zrobić lub jeśli ktoś nie wie, jakie są zależności w aplikacji.
Dodatkowo, Code Review jest sam w sobie procesem uczenia się i wymiany wiedzy odnośnie samej aplikacji. To zmniejsza prawdopodobieństwo, że odejście jednej osoby sprawi, że nagle nikt nie zna ważnego fragmentu aplikacji i nie da się go rozbudować.
Code Review umożliwia też wprowadzenie technik i zasad niemożliwych do analizy narzędziami statycznej analizy kodu – takich technik, które zdecydowanie mogą podnieść jakość pracy w aplikacji ale gdzie błąd programisty może być dość problematyczny.
7.4. Wykorzystanie miar i trendów
Oprócz tego wartość Code Review polega też na tym, że trendy z danych zbieranych pozwalają nam na monitorowanie stanu procesu, ludzi oraz aplikacji.
Jeżeli programiści robią Code Review coraz wolniej – co się zmieniło? Jakiego typu interwencję należy wprowadzić?
Code Review nigdy nie jest traktowane jako najważniejsza rzecz na świecie. Dodatkowo, Code Review jest na styku ludzi, procesów oraz aplikacji.
To sprawia, że Code Review jest świetnym wskaźnikiem tego, że w całym stanie systemu wytwarzania oprogramowania coś się zmieniło - trzeba tylko z pozostałych miar wyszukać informację o tym, co się naprawdę stało.
8. Następne kroki
Jeżeli nie masz w projekcie Code Review, rozważ, czy coś z zaprezentowanych przez nas rzeczy by Ci się przydało. Jeżeli tak, skup się tylko na tej jednej rzeczy i spróbuj wprowadzić minimalną ilość rzeczy które przyniosą Ci jak największe korzyści.
Jeżeli masz w projekcie Code Review, sprawdź czy nie można przerzucić części badanych tam rzeczy do narzędzia statycznej analizy kodu. Oszczędzi Ci to czas i sporo niepotrzebnych dyskusji.
Niezależnie od okoliczności, sprawdź czy nie możesz zacząć zbierać jakichś danych i obserwować jakichś trendów. Nawet, jeżeli jeszcze nie chcesz tego robić sam fakt zebrania danych otwiera Ci takie możliwości w przyszłości.
Jeżeli chcesz pogłębić temat Code Review i lubisz słuchać podcastów, warto przesłuchać odcinek o Code Review na DevCast.
Powodzenia!
Monika Januszek, Łukasz Januszek
