Code review, czyli merytoryczna krytyka siebie i innych

Rafał Pieńkowski
Ikona kalendarza
10 kwietnia 2020

Siedzisz nad rozwiązaniem problemu kilka dni. Wpadasz na jego rozwiązanie. Projektujesz klasy i metody. Jesteś z siebie ogromnie dumny... i nagle dowiadujesz się, że źle nazwałeś zmienne, metody są nieintuicyjne, cała klasa do przepisania, a w ogóle to i tak nie do końca zrozumiałeś początkowy problem. Jak uniknąć zderzenia z brutalną rzeczywistością, oraz jak obiektywnie recenzować kod innych? Podpowie Ci to niniejszy artykuł.

Co to właściwie jest code review i po co? Review kodu odbywa się najczęściej po zaimplementowaniu danej funkcjonalności, wówczas pozostali członkowie zespołu decydują, czy dane zmiany wprowadzić do projektu, czy też wymagają jeszcze pewnych usprawnień. Uwagi i sugestie na code review można podzielić na 3 poziomy:

Poziom 1 – TYPOs, literówki i drobne uwagi

Jest to najbardziej podstawowy rodzaj uwag, najprostszy do zauważenia przez reviewerów.

obraz1.webp

Jak widać na powyższym przykładzie, metoda addd powinna zostać zmieniona na add. Szczegół nad szczegółami, jednak takie uwagi również są potrzebne, żeby utrzymać czysty kod. Sugestie znamy nazw są również potrzebne:

obraz2.webp

Metoda przekształca kolekcję imion – powinny one mieć co najmniej 3 i (akurat w tym konkretnym przypadku) co najwyżej 30 liter, następnie zamienia wszystkie litery na małe. Co tutaj można zmienić? Otóż nazwa metody, mimo że wyjaśniająca jej implementację, jest dość długa, można pomyśleć nad lepszą:

obraz3.webp

Nazwa metody została skrócona, co więcej, nie wyjawia ona na świat zewnętrzny szczegółów implementacyjnych – metoda „wie”, o jakiej długości mają być imiona – ktoś, kto tę metoda wywoła, nie musi znać tych detali. Zostały wyekstraktowane również zmienne lokalne opisujące liczby 3 oraz 30 – już nie są tzw. „magic numbers”. Uwagi tego stylu są jak najbardziej również potrzebne, jednak nie powinniśmy ograniczać się jedynie do nich.

Poziom 2 – Struktura klas i metod, testy, wzorce projektowe

Dodanie tego rodzaju uwagi wymaga już dłuższej i bardziej wnikliwej analizy. Weźmy pod uwagę jeszcze raz przykład z poprzedniego rodzaju uwag:

obraz4.webp

Uwagi z poziomu 2 mogą być następujące:

  • Czy kolekcja names powinna być polem, czy może jednak powinna zostać podana jako argument metody?
  • Czy kolekcja names powinna zostać listą, czy może jednak bardziej odpowiednią formą dla niej byłby set? Czy akceptujemy duplikaty?
  • Czy metoda applicableNamesLowercased powinna pozostać publiczna? Być może wywołujemy ją jedynie wewnątrz danej klasy, bądź w podklasach?
  • Czy do powyższej metody zostały napisane testy?

Wracając do początkowego przykładu z kalkulatorem:

obraz5.webp
  • Czy klasa nie powinna być oznaczona jako final? W końcu raczej nie będziemy rozszerzać klasy kalkulatora.
  • Czy metoda add nie powinna być statyczna? Jeśli cała klasa to klasa w stylu util?
  • A może jednak metoda add powinna pozostać niestatyczna, gdyż statyczne metody są trudniejsze w testowaniu?
  • Czy do powyższej metody zostały napisane testy?

Jak widzimy, Uwag kwalifikujących się do poziomu 2 jest o wiele więcej – można wręcz powiedzieć, że w pewnych niuansach, ile ludzi tyle opinii. Wszystkie jednak powinny mieć wspólny mianownik – dążenie do jak najbardziej czytelnego kodu.

Poziom 3 – Logika biznesowa

Żeby dodać uwagę tego rodzaju, trzeba dobrze znać projekt, w którym się pracuje, najlepiej całościowo, nie tylko kawałek funkcjonalności. Chociaż czasami wystarczy zdrowy rozsądek i rzetelne czytanie kodu. Spójrzmy na metodę, która się już nam pojawiła:

obraz6.webp

Minimalna długość została zmieniona! Programista, który nie do końca zrozumie początkowe założenia (minimum 3 litery), napisze funkcjonalność „działającą”, nawet przetestowaną i testy przechodzą... Jednak ów programista testy napisał, opierając się na fałszywym założeniu! Uwagi w stylu poziomu 3 są bardzo cenione przez innych programistów.

Poznaliśmy już różne rodzaje uwag w code review. Co jednak w przypadku odwiecznych wojen (taby vs spacje) lub różnych opinii dwóch programistów? Pamiętajmy o bardzo ważnym słowie – KONWENCJA. Uważasz, że Twój kolega źle nazwał klasę? Zwróć najpierw uwagę na to, czy nie postąpił według konwencji, czyli według przyjętych (przez zespół) zasad. Nawet „nie do końca czysty kod” w dużym projekcie, jeśli jest trzymany w konwencji, jest dużo bardziej czytelny niż „clean code”, gdzie każdy z modułów ma inne podejście.

Bądźmy również otwarci na merytoryczną krytykę od innych – zamiast „znowu się przyczepił”, pomyślmy o uwagach na code review jako o możliwości samorozwoju i zauważenia innych form widzenia. Każdy programista czuje satysfakcję ze swojego kodu, to naturalne i zdrowe (byłoby źle, gdyby nie było tej satysfakcji). Jednak nie przywiązujmy się zbyt emocjonalne do swojego rozwiązania. W drugą stronę, samemu również nie „wyżywajmy się” na kolegach z zespołu, wywyższając się gdy znajdziemy pewien błąd w ich kodzie. Pamiętajmy, że code review ma wspólny cel – dostarczanie kodu wysokiej jakości.

Przeczytaj także

Ikona kalendarza

29 marzec

To be or not to be a multi cloud company? Przewodnik dla kadry kierowniczej, doradców ds. chmury i architektów IT. (część 2)

Po przeczytaniu pierwszej części poradnika zauważymy, że strategie organizacji są różne. Część firm oparło swój biznes wyłącznie na j...

Ikona kalendarza

27 wrzesień

Sages wdraża system Omega-PSIR oraz System Oceny Pracowniczej w SGH

Wdrożenie Omega-PSIR i Systemu Oceny Pracowniczej w SGH. Sprawdź, jak nasze rozwiązania wspierają zarządzanie uczelnią i potencjałem ...

Ikona kalendarza

12 wrzesień

Playwright vs Cypress vs Selenium - czy warto postawić na nowe?

Playwright, Selenium czy Cypress? Odkryj kluczowe różnice i zalety każdego z tych narzędzi do automatyzacji testów aplikacji internet...