Code review – techniki

Aby przeprowadzić code review nie musimy wymyślać własnych rozwiązań. Dostępny jest cały pakiet technik, które wskazują właściwy kierunek i na nich właśnie skupię się w tym artykule. Postaram się przy tym odnieść do doświadczeń własnych, by przebieg procesu opisać w możliwie przystępny i przydatny sposób.

Inspekcja Fagana (Fagan inspection)

Jedynie tej z opisywanych tu metodyk nie przetestowałam osobiście. Opisał ją szczegółowo Michael Fagan w 1976 roku w artykule pod tytułem “Design and Code Inspections to Reduce Errors in Program Development” (opublikowanym w IBM Systems Journal, który, podobnie jak pozostałe dotychczas wydane numery, nadal dostępny jest na firmowej stronie IBMa).

Od tamtej pory powstało wiele publikacji na ten temat, proces ewoluował i poszczególne opisy różnią się w detalach od jego pierwotnego przebiegu. Nie są to jednak znaczące zmiany, a to co istotne, pozostało: inspekcja Fagana jest procesem formalnym, uporządkowanym i możliwym do zmierzenia.

Standardowe role, jakie przyjmują uczestniczy tego procesu to: Author, Moderator, Reader, Reviewer oraz Recorder. Czasem zaangażowany w inspekcję jest także Observer.*

Typowe etapy inspekcji Fagana:

Planning

• Autor przygotowuje materiały, które muszą spełnić określone wcześniej warunki, by możliwe było poddanie ich inspekcji
• Ustala się termin spotkania wprowadzającego (Introduction Meeting) oraz jego uczestników

Introduction Meeting

• Autor przedstawia biznesowe tło przygotowania funkcjonalności
• Moderator określa cel przeprowadzenia inspekcji oraz wyjaśnia uczestnikom zasady, wedle których będzie ona prowadzona
• Ustalany jest harmonogram spotkań w ramach inspekcji
• Jeżeli uczestnicy są wprowadzeni w temat, którego dotyczy inspekcja, znają jej cel i zasady, a harmonogram spotkań został ustalony wcześniej, ten etap można pominąć

Reading Phase

• Każdy z uczestników musi przeczytać materiały, aczkolwiek zależnie od roli, skupia się na innych aspektach
• Na tym etapie nie wyszukuje się błędów

Inspection Meeting

• Moderator odpowiada za to, by każdy z uczestników skupił się na zadaniu przynależnym dla swojej roli oraz by całość przebiegła sprawnie i bez osobistych przytyków
• Czytelnik ma za zadanie zaprezentować materiały, aby zapobiec ewentualnej ich nadinterpretacji i złemu zrozumieniu przez pozostałych uczestników
• Materiały podlegają inspekcji przez grupę
• Błędy są zapisywane (Defect Log)
• Mierzeniem całego procesu zajmuje się Recorder
• Jeżeli w trakcie fazy inspekcji nie zostaną znalezione żadne błędy, proces można zakończyć

Rework

• Autor dokonuje poprawek, kierując się dziennikiem błędów (Defect Log)
• Ustala jest termin spotkania weryfikującego

Verification Meeting

• Reviewer weryfikuje, czy wszystkie poprawki zostały wykonane
• Jeżeli wszystkie zgłoszone błędy zostały naprawione, kończy to proces inspekcji, w przeciwnym wypadku proces wraca do fazy przeróbek

Już poza procesem możliwe jest zorganizowanie spotkania, podczas którego uczestniczy inspekcji zastanowią się, co można zrobić, by proces następnym razem przebiegł sprawniej (Follow-up meetings).

* Pozwoliłam sobie pozostać przy nazwach angielskich, jako że łatwiej odnaleźć się w źródłach, bazując na oryginalnych nazwach.

Korzyści:

• metodyka ta faktycznie działa i znakomicie sprawdza się przy usuwaniu błędów z wszelkich dokumentów – od specyfikacji po niskopoziomowy kod
• pomaga trenować nowych zatrudnionych
• cały proces jest „mierzalny” – prowadzone metryki pozwalają śledzić progress

Minusy:

• proces jest czasochłonny i kosztowny – wymaga dużej ilości spotkań, w których bierze udział dużo osób, a ponadto 40 lat temu, gdy powstawała ta technika, wszystkie materiały były drukowane i każdy z uczestników otrzymywał własną kopię

Opisany minus jest tak poważny, ze praktycznie dyskwalifikuję metodykę, zwłaszcza przy zastosowaniu Agile. Przez lata wiele firm próbowało ją wdrożyć, jednak ze względu na koszty, w przeważającej większości przypadków niemożliwe okazywało się objęcie inspekcją całego „wchodzącego” kodu. Próby podejmowano w małych, eksperymentalnych grupach, a efektów ich prac zazwyczaj nie dało się przeskalować na pracę całej firmy.

A zatem co w tej sytuacji? Z pomocą przychodzą nam inne, lżejsze, szybsze, angażujące mniejszą ilość osób i często asynchroniczne metody code review. Dobrze wpasowujące się w pracę w SCRUMowych sprintach czy kolejkowanie zadań w Kanbanie. Poniżej ich opis.

Programowanie w parach (pair programming)

To jedna z praktyk wywodzących się z eXtreme Programming. Polega ona na wspólnym tworzeniu kodu przez parę programistów. Zadania są tu podzielone, jedna osoba tworzy partię kodu, podczas gdy druga obserwuje jej pracę i doradza, koncentrując się mniej na detalach, a bardziej na całokształcie wykonania zadania. Osoby w parze zamieniają się rolami, wskazane jest, by robić to nie rzadziej, niż co kilkadziesiąt minut. Przykładowo w jednym z podejść (Ping-pong), programiści rotują w bardzo krótkich odstępach czasu. Pierwszy z programistów pisze nieprzechodzący test. Drugi programista przejmuje klawiaturę i pisze tylko tyle kodu, by możliwe było poprawne przejście testu. Następnie obaj wspólnie refaktoryzują napisany kawałek kodu. Gdy cykl zaczyna się od początku, role się odwracają i drugi z programistów pisze nieprzechodzący test.

Z powodzeniem można programować w parach zarówno pracując przy jednym komputerze, jak i zdalnie współdzieląc ekran. Jestem członkiem zespołu, w którym część z nas (np. ja ;)) pracuje całkowicie zdalnie i mogę Wam zaręczyć, że jak najbardziej jest to możliwe.

Pozostaje tylko pytanie, co programowanie w parach ma wspólnego z code review. Przede wszystkim jest to znakomita okazja do bieżącego przeglądu tworzonego kodu i wykrycia błędów na najniższym możliwym poziomie. To również okazja do propagacji wiedzy domenowej oraz poznawania pryncypiów kodowania, praktycznego użycia wzorców czy nowych możliwości IDE. Umożliwia też twórczą dyskusję i wspólne opracowywanie rozwiązań.

Korzyści

• Reviewer bardzo dobrze rozumie problemy i może na bieżąco wrzucać uwagi
• Bardzo efektywnie wykrywane są błędy
• Mocno wzrasta przepływ wiedzy w zespole
• Część programistów lubi taki sposób pracy

Minusy

• Tak naprawdę trudno tu mówić o recenzencie – programiści zamieniają się rolami, ściśle ze sobą współpracują i może zabraknąć spojrzenia na całość – architekturę i czytelność rozwiązania (testowaliśmy w zespole tę technikę i po kilku sprintach koniecznością okazało się dodatkowy przegląd kodu przygotowanego przez parę)
• Problemem staje się brak poczucia odpowiedzialności za kod – często brakuje na koniec wspólnego przejrzenia całości stworzonego rozwiązania oraz ręcznego przetestowania poszczególnych wariantów, co z pewnością miałoby miejsce podczas pracy jednego programisty (świadomość, że ktoś będzie oceniał moją pracę, automatycznie sprawia, że przykładamy większą wagę kodu, który oddajemy)
• Zajmuje więcej czasu, niż praca jednego programisty
• Część programistów nie lubi takiego sposobu pracy
• Programiści w parze mogą się znacząco różnić temperamentem i stylem pracy, przez co może się ona stać męcząca dla obu stron oraz dużo mniej efektywna
• Zarówno praca przy jednym komputerze, jak i zdalna mają swoje ograniczenia, które nie występują podczas samodzielnej pracy na własnym stanowisku (różne IDE, skróty klawiszowe czy układ klawiatury, utrudniona praca przy przekazywaniu kontroli nad pulpitem lub konieczność wielokrotnego synchronizowania gałęzi repozytorium – mogą skutecznie utrudnić pracę, a tym samym zniechęcić)
• Brak metryk, które pozwoliłyby mierzyć proces oraz śledzić, czy wszystkie poprawki zostały zaimplementowane

„Review przez ramię”

Ta technika świetnie sprawdza się przy niewielkich zmianach w kodzie (np. parę linijek kodu), gdy z różnych przyczyn chcemy, by review odbyło się możliwie jak najszybciej.

Ogólnie polega ono na znalezieniu dewelopera, który akurat dysponuje wolną chwilą i chęciami, dzięki czemu będzie mógł przeprowadzić z nami szybką konsultację. Z powodzeniem można przeprowadzić to zarówno metodą tradycyjną, na własnym komputerze, jak i korzystając z narzędzi umożliwiających komunikację głosową z równoczesnym udostępnieniem ekranu (np. Skype, Google+ Hangouts).

Programista prezentuje na ekranie kod oraz wyjaśnia, co należało zrobić w ramach zadania. Recenzent jeśli ma taką potrzebę, zadaje pytania dodatkowe lub daje wskazówki do kodu, a po ewentualnych zmianach, rzuca ponownie okiem w kod i akceptuje go (lub nie ;))

Korzyści

• Łatwość przeprowadzenia
• Przyspieszenie przejścia przez część DoD (Definition of Done) zadania
• Możliwość werbalnego wyjaśnienia tła zadania, bez potrzeby samodzielnego przegryzania się przez kod i opis zadania
• Zdjęcie z recenzenta części prac, związanych z przenoszeniem zmian do głównej gałęzi, czy stawianiem środowiska do testów

Minusy

• Przeglądając kod kolegi na własnym komputerze nie ma presji, zatem łatwiej zweryfikować czy kod jest czytelny, zmiany nie mają wpływu na inną funkcjonalność, a testy jednostkowe zostały zaktualizowane
• Odbywa się szybko, przez co recenzent często nie poświęca mu dość uwagi i istotne błędy pozostają niewykryte
• Brak możliwości zmierzenia procesu, a uwagi przekazywane są ustnie, więc przez czysty przypadek część z nich może zostać niezrealizowana

Z użyciem dedykowanych narzędzi / z wykorzystaniem maila

Celowo wpięłam te dwa sposoby w jednej podpunkt, ponieważ w dzisiejszych czasach, gdy standardem jest korzystanie z systemów kontroli wersji takich jak SVN czy GIT, są one prawie identyczne – różnią się raptem sposobem śledzenia błędów.

Developer tworzy kod na osobnej gałęzi repozytorium. Po zakończeniu kodowania daje znać pozostałym programistom, że zadanie czeka na review. W przypadku naszego zespołu odbywa się to przez ustawienie stanu zadania na „oczekiwanie na review” w systemie zarządzania zadaniami. Czasem dodatkowo komunikujemy to na wspólnym czacie czy mailu, gdy chodzi o temat szczególnie istotny i warto, by dla przeprowadzenia review ktoś przerwał swoją pracę. W przypadku korzystania z dedykowanych narzędzi, takich jak Github, odbywa się to przez wykonanie pull requesta.

Kolejnym krokiem jest przegląd nowego kodu przez jednego z członków zespołu. Aby nie pominąć żadnego z kryteriów przyjętych przez zespół, mamy checklistę review, zawierającą punkty, na które szczególną uwagę musi zwrócić recenzent. Między innymi wypisane są tam pryncypia kodowania czy wskazówki, pozwalające ocenić jakość testów jednostkowych. Warto, by taka lista nie zawierała zbyt wielu punktów (mnogość ich utrudnia i wydłuża przeglądanie bazy kodu) oraz by była aktualizowana w miarę rozwoju zespołu. Przykładowo pierwotna wersja naszej checklisty w dużej mierze dotyczyła standardów kodowania i ewoluowała wraz ze wzrostem doświadczenia zespołu.

Zastrzeżenia do nowo powstałej funkcjonalności przesyłamy sobie na maila. Zależnie od potrzeb kolejne paczki uwag dostarczane są etapami lub w całości. Korzystanie z dedykowanych narzędzi znacznie ułatwia ten proces, ponieważ komentarze można zawrzeć bezpośrednio w podglądzie kodu udostępnianym przez narzędzie. W przypadku komunikacji mailowej zastępujemy to np. przez podanie nazwy klasy i nr linii w pliku, jednak stanowi to pewną niewygodę.

Po wykonaniu poprawek przez autora, recenzent weryfikuje je i zależnie od efektów oznacza to koniec przeglądu lub zgłaszane są kolejne uwagi.

Kod, który przeszedł review mergowany jest do głównej gałęzi rozwojowej, gdzie przeprowadzane będą jego testy (kolejny etap po recenzji kodu). Alternatywny scenariusz ma miejsce w przypadku hot fixów dla produkcyjnej wersji kodu, w którym kod stawiany jest na feature branchu (osobnej instancji aplikacji, której baza kodu opiera się na osobnej gałęzi zawierającej zrecenzowany kod) i dopiero po testach trafia do głównego brancha.

Korzyści

• asynchroniczność procesu – właściwie całość można przeprowadzić bez bezpośredniego kontaktu autora oraz recenzenta (choć oczywiście kontakt taki przyspiesza flow i ułatwia ewentualne dyskusje), dzięki czemu można to zrobić w dogodnym dla obydwu czasie
• łatwość włączenia tej metodyki jako stały element pracy zespołu
• spisywanie uwag, które ułatwia weryfikację przeróbek
• i oczywiście wszystkie plusy code review – wczesne wykrywanie błędów, podnoszenie jakości kodu, weryfikacja architektury aplikacji czy propagacja wiedzy w zespole (domenowej i programistycznej)

W tym przypadku minusy są kwestią dyskusyjną.

Pewien problem wynika z komunikacji mailowej – w przypadku recenzji dużej partii kodu, do której jest dużo uwag, trudno śledzić, czy faktycznie wprowadzone zostały wszystkie poprawki. Jednak rozwiązaniem jest tu użycie dedykowanych narzędzi. Podobnie jest z “mierzalnością” procesu – tu również rozwiązaniem są dedykowane narzędzia.

Z kolei w przypadku pracy w sprintach, asynchroniczność narzuca konieczność wprowadzenia dodatkowych zasad, które zagwarantują zakończenie procesu review na czas (niezbędne, gdy recenzja kodu stanowi stały element DoD i zadanie, dla którego nie zakończono recenzji, uznaje się za niewykonane). Jednak tu rozwiązaniem jest po prostu dobra organizacja. W przypadku naszego zespołu przyjęliśmy, ze wykonanie przeglądu kodu i poprawek po nim ma priorytet wyższy, niż dokończenie niezakodowanego zadania.

 

Łatwo zauważyć, która z metod jest moim faworytem 😉

A jakie są Wasze doświadczenia z code review? Czy znacie inne techniki jego przeprowadzania?

 

Poprzedni artykuł dotyczący code review:

Code review – part 1 : Korzyści i koszty