Co sprawia, że proces recenzji kodu jest dobry? Co powinieneś robić, a czego unikać? Pozwól, że podzielę się swoim doświadczeniem w tym obszarze 🙂

Cel przeglądu kodu

Recenzja kodu, niezależnie od tego, gdzie jest wykonywana, ma bardzo ważny cel: dostarczyć wydajny, dobrze napisany (czytelny, optymalny, zgodny z firmowymi wytycznymi), działający kod, który spełnia potrzeby biznesowe. Z punktu widzenia klienta czy użytkownika nie jest ważne, jaka jest konwencja nazewnictwa, standardy kodowania itd. – po prostu system musi działać zgodnie z oczekiwaniami.

Nie oznacza to, że przegląd kodu nie ma innych celów, one są po prostu mniej ważne 😉. Code Review pozwala nam:

• dzielić się wiedzą w ramach zespołów lub projektów międzyzespołowych
• rozwijać programistów, którzy mają mniejsze doświadczenie z językiem i/lub aplikacją
• znaleźć scenariusze, o których wcześniej nie pomyślał autor kodu
• upewnić się, że ogólna idea zmian jest poprawna

Główne zasady

Pomiędzy autorem merge requestu, a recenzentami/osobami zatwierdzającymi istnieje umowa, która może być różna w zespołach i projektach, ale istnieją pewne ogólne zasady, które znajdą zastosowanie wszędzie. Nie mówię o zasadach technicznych, ale o rzeczach, które mogą uczynić proces bardziej wydajnym i przyjaznym. Spójrzmy na to!

Bądź proaktywny

Śledź merge requesty, w których uczestniczysz. Bez względu na to, czy jesteś autorem, czy recenzentem, naprawdę powinieneś mieć oko na otwarte MRy, w których bierzesz udział. Nie krępuj się prosić autora/recenzentów o reakcję (wręcz powinieneś to robić), jeśli merge request zbyt długo jest nieaktywny, ponieważ im szybciej zostanie dokończony, tym lepiej. Na przykład, menu w prawym górnym rogu #Gitlaba zawiera przydatne pozycje, można w prosty sposób przejść do listy MRów lub listy zadań — powinieneś obserwować te kontrolki i reagować, jeśli masz na nich niezerowe liczniki. W Githubie z kolei jest widok notyfikacji, gdzie znajdują się wszystkie informacje o aktywnościach w zgłoszeniach i merge requestach, w których bierzesz udział — utrzymuj tę listę tak krótką, jak to tylko możliwe.

Bądź responsywny

Bardzo ważne jest reagowanie na aktywność w merge requestach — przeprowadzanie przeglądu kodu, odpowiadanie na komentarze, rozwiązywanie dyskusji, wypychanie poprawek itp. tak szybko, jak to możliwe. Nie ignoruj powiadomień e-mail — jeśli uważasz, że jest ich za dużo, rozważ zmianę ustawień powiadomień lub utworzenie reguł filtrowania. Jeśli pracujesz w rozproszonym zespole, nie wyciszaj komunikatora (jeśli naprawdę nie ma takiej potrzeby), by członkowie Twojego zespołu mogli się z Tobą skontaktować.

Stosuj klarowne komunikaty

Bardzo ważne jest, aby dobrze się komunikować, by nie marnować czasu innych.

Jako autor merge requestu opisz, co robi Twój kod, jak działa i jak powinien być weryfikowany. Twórz dobrze opisane commity, umieść komentarze w kodzie, jeśli coś jest złożone lub nieintuicyjne.

Jako recenzent pisz dobre komentarze, które jasno informują autora o tym, czego oczekujesz. Jeśli znalazłeś krytyczny błąd lub istotną pomyłkę, po prostu to podkreśl. Jeśli są jakieś opcjonalne ulepszenia lub możliwa refaktoryzacja — też to napisz. Każdy Twój komentarz powinien wskazywać, co jest nie tak i co jest „definicją wykonania” (ang. definition of done) z Twojej perspektywy. Unikaj krótkich i nieopisowych komentarzy, które mogą wywołać niepotrzebną dyskusję.

Jako dowolny uczestnik MR, jeśli uważasz, że pisemna dyskusja nie działa, po prostu zorganizuj spotkanie, aby ją rozwiązać. Czasami 5 minut rozmowy może być znacznie lepsze niż kilka komentarzy, których napisanie/przeczytanie zajmuje więcej czasu. Istnieje również różnica w percepcji, słowa pisane mogą być źle zrozumiane i prowadzić do niechcianych spięć między Tobą a innymi uczestnikami.

Bądź miły

Pamiętaj — nie musisz zgadzać się z innymi, ale szanuj ich i ich pracę. Masz prawo komentować wszystko, co jest to związane z kodem, konwencjami projektu, wymaganiami biznesowymi itp., ale przegląd kodu nie jest miejscem na osobiste animozje. Unikaj pasywno-agresywnych sformułowań, takich jak „To jest źle”, mogą one zaszkodzić i sprawić, że Twoje argumenty będą mniej przekonujące.

Bądź zdystansowany

Dotyczy to głównie autorów merge requestów, ale recenzenci powinni również pamiętać, że wszystkie komentarze w MR nie mają na celu atakowania ich, albo udowadniania, że się mylą lub że są kiepscy w tym, co robią. Dyskusje w merge requestach są narzędziem do ulepszania dostarczanego kodu. Nie bierz komentarzy do siebie — masz prawo mieć własne zdanie i konwencje, więc to naturalne, że czasami zrobisz coś inaczej, niż inni. Jeśli ktoś komentuje, że należy to zrobić w inny sposób, po prostu przedyskutuj to bez emocji (przynajmniej spróbuj 😉).

Rola autora

Jako autor powinieneś opiekować się merge requestem od momentu jego utworzenia, aż do jego scalenia/zamknięcia, więc powinieneś przydzielić siebie i pozostać jako assignee tak długo, jak to Ty nad nim pracujesz. Jeśli ktoś przejmuje od Ciebie zadanie i kontynuuje Twoją pracę, MR powinien zostać przypisany do tej osoby (assignee zawsze powinien wskazywać osobę odpowiedzialną za dostarczenie produktu końcowego).

Jeśli nie masz uprawnień do wykonywania merge w projekcie, powinieneś być przypisany do merge requestu, dopóki nie będzie gotowy do scalenia. Jesteś odpowiedzialny za dostarczenie zatwierdzonych zmian w kodzie, merge jest ostatnim krokiem technicznym, zatem możesz przypisać innego użytkownika do MR, gdy może on zostać scalony (ale można go scalić również bez zmiany osoby przypisanej, możesz po prostu wywołać użytkownika z prawami do wykonania merge lub dodać odpowiednią etykietę — powinna to być część procesu wytwarzania w zespole).

Wersja robocza / WIP

W większości przypadków należy otworzyć merge request, gdy kod jest gotowy do recenzji, ale można utworzyć MR i oznaczyć go jako wersję roboczą, nawet jeśli nie jest gotowy. Gdy merge request jest w trybie Draft, recenzenci NIE powinni aktywnie przeprowadzać przeglądu kodu, a jedynie odpowiadać na wątpliwości lub pytania autora. Oczywiście mogą zrobić przegląd kodu, ale powinni zapytać autora, czy ma to sens — być może autor chce całkowicie zmienić implementację, a wtedy przeglądanie bieżącego kodu jest stratą czasu. Gdy jako autor uważasz, że kod jest poprawny i gotowy do recenzji, zdejmij flagę Draft, zaczynając właściwy Code Review.

Rola recenzenta

Jako recenzent jesteś odpowiedzialny za sprawdzenie, czy dostarczone zmiany w kodzie są akceptowalne — pod względem ogólnych dobrych praktyk w programowaniu, standardów i konwencji zespołu oraz firmy, bezpieczeństwa i wydajności. Robisz to, sprawdzając zmiany w kodzie i dostarczając informacje zwrotne tam, gdzie jest to wymagane. Powinieneś użyć dyskusji lub sugestii zmian kodu, aby pomóc autorowi MR w dostarczeniu najlepszego możliwego kodu. Jeśli rozpoczniesz dyskusję, powinieneś mieć ją na oku i oznaczyć jako rozwiązaną, gdy tylko skomentowany kod zostanie zaktualizowany przez autora merge requestu i jeśli wprowadzone zmiany są satysfakcjonujące (ℹ️ dyskusje mogą być automatycznie rozwiązywane, jeśli są skonfigurowane w ten sposób — należy to przedyskutować w zespole/firmie). Pamiętaj, Twoje uwagi będą miały większą wartość, jeśli zawrzesz w nich:

• odniesienia do oficjalnych dokumentacji
• linki do wyspecjalizowanych artykułów napisanych przez ekspertów
• benchmarki pokazujące przewagę zaproponowanych zmian
• sugestie zmian (jeśli wspiera je platforma, na której odbywa się przegląd kodu)
• odniesienia do poprzednich merge requestów lub komentarzy powiązanych z aktualnie recenzowanymi zmianami

W kwestii sugestii: używaj ich w komentarzach, by zwizualizować proponowane zmiany. To pozwoli autorowi kodu lepiej zrozumieć Ciebie, a także umożliwi szybsze naniesienie poprawek. Jeśli nie wiesz czym są sugestie, przeczytaj dokumentację Gitlaba lub Githuba.

Autor(zy) i recenzenci zmian w kodzie są jak mały zespół, powinni ze sobą współpracować. Nie traktuj przeglądu kodu jako samodzielnej pracy — miej oko na komentarze innych recenzentów, przekaż opinię, jeśli się zgadzasz lub nie zgadzasz (może nawet bardziej w tym drugim przypadku, ponieważ może to zapobiec błędnym zmianom). Czytanie komentarzy innych recenzentów pomoże również zminimalizować redundancję — bez czytania komentarzy innych można dodać identyczny komentarz i wprowadzić niepotrzebny szum.

Gdy zmiany w kodzie są dla Ciebie akceptowalne, powinieneś zatwierdzić MR, jasno sygnalizując, że się z nimi zgadzasz.

Szczegóły techniczne

Wszystko zależy od platformy, na której odbywa się przegląd kodu, ale mimo to pewne zasady techniczne obowiązują zawsze:

• Tytuł powinien odzwierciedlać wprowadzone zmiany. Upewnij się, że tytuł jest przejrzysty i opisowy. Jeśli istnieje konwencja nazewnictwa (np. z identyfikatorem zadania JIRA), użyj jej. Użyj statusu Draft, aby wskazać, że kod nie został jeszcze ukończony (jeśli istnieje taka potrzeba).
• Użyj opisu, aby podać dodatkowe informacje recenzentom. Możesz opisać tło zmiany, wyjaśnić, dlaczego jest ona zaimplementowana w ten sposób, jak ją ręcznie zweryfikować i tak dalej. Umieść tam wszystko, co mogłoby pomóc w lepszym i szybszym przeglądzie kodu.
• Przypisz się do merge requestu, aby łatwiej dało się śledzić jego przebieg.
• Przypisz recenzentów (jeśli nie masz automatyzacji tego procesu). Jeśli projekt na to pozwala, możesz dostosować reguły zatwierdzania — zaleca się, aby do wykonania merge były wymagane 2 zatwierdzenia, ale w bardziej złożonych merge requestach możesz wymagać większej liczby zatwierdzeń, również od konkretnych osób.
• Jeśli Twój merge request jest powiązany z innymi MRami, możesz zdefiniować zależności między merge requestami, co zapobiegnie łączeniu Twoich zmian przed scaleniem innych MRów.

Podsumowanie zalecanego procesu przeglądu kodu

• Skonfiguruj ustawienia swoich projektów, aby zoptymalizować proces code review (zatwierdzenia; właściciele kodu; wymagania, które muszą być spełnione przed scaleniem).
• Zautomatyzuj wszystko, co jest możliwe z perspektywy QA (#standardy kodowania, #statyczna analiza, testy, lintery itp.), dzięki czemu możesz skupić się na rzeczywistej logice biznesowej i/lub zmianach technicznych w MR.
• Otwórz merge request, gdy tylko potrzebujesz informacji zwrotnej (od #CI lub od recenzentów), ale oznacz ją jako wersję roboczą, jeśli nie jest gotowa do ostatecznej recenzji. Usuń oznaczenie jako wersja robocza, gdy uznasz, że zmiany są gotowe (co wskazuje, że zakończyłeś pracę i potrzebujesz opinii).
• Przydziel recenzentów, gdy MR jest w stanie wymagającym informacji zwrotnej (wersja robocza z rzeczami do omówienia lub gdy jest gotowy do recenzji).
• Utrzymuj merge request otwarty tak krótko, jak to możliwe. Otwarte MRy w większości przypadków powinny mieć wyższy priorytet niż pozostałe prace, aby można je było sfinalizować i dostarczyć produkt.
• Utrzymuj kontakt z innymi uczestnikami — autor i recenzenci powinni pracować jako zespół odpowiedzialny za finalizację zadania. Śledź merge requesty, w których uczestniczysz, uczyń to swoją codzienną rutyną.
• Scal MR, gdy spełni wszystkie wymagania (zielony pipeline, zakończone dyskusje, zebrane wszystkie wymagane zgody). Czasami dobrym pomysłem jest rebase gałęzi przed połączeniem, ale zależy to od konwencji projektowych.

Miłego recenzowania!