Code review, które wykonuję innym osobom z projektu, bywa dla mnie okazją do treningu umiejętności analitycznych i niesamowitą możliwością zdobycia nowej wiedzy (jak również zweryfikowania tej już posiadanej). Bywa jednak i tak, że CR staje się mordęgą oraz najgorszym elementem dnia pracy. Na moje nastawienie wpływa wiele czynników, takich jak choćby złożoność implementowanego rozwiązania, zastosowanie przez autora konwencji panujących w projekcie czy moja znajomość wykorzystanych wzorców lub zewnętrznych narzędzi. Tak czy inaczej za wykonywaniem code review idą w mojej ocenie same korzyści, zarówno dla czytającego, jak i dla autora kodu.
Mój obecny projekt jest pierwszym, w którym miałam okazję tam naprawdę włączyć się w czytanie kodu innych programistów. Na początku nie widziałam większego sensu w tym, żebym to ja robiła code review kolegom z zespołu. Koniec końców byłam w projekcie bardzo krótko, nie znałam kodu ani panujących konwencji, poza tym byłam juniorem i jak ja miałabym robić review midom czy seniorom. Z czasem okazało się, że moje obawy są nieuzasadnione. Oczywiście nic nie zastąpi doświadczenia i szerszej perspektywy starszych stażem programistów, jednak od czasu do czasu świeże spojrzenie na pewne problemy również się przydaje.
Korzyści z CR dla reviewera
Okazało się również, że to ja sama czerpię korzyści z czytania cudzego kodu. Najważniejsze w mojej ocenie wymieniam poniżej:
- Poznaję konwencje panujące w projekcie
Każdy projekt ma pewne założenia, które musi spełniać kod. Są to zasady dotyczące nazewnictwa, umiejscowienia pewnych elementów, użycia konkretnych rozwiązań zewnętrznych. Podczas czytania kodu moich kolegów zbieram informacje dotyczące tego, co gdzie powinno się znajdować, jak nazywać odpowiednio metody, jak nazywać zmienne, jak opisywać poprawnie typy danych czy jak konstruować większe części aplikacji, np. całe moduły. - Poznaję częste przyczyny występowania określonych błędów
Code review odbywa się nie tylko przy implementowaniu nowych funkcjonalności, ale również przy naprawianiu błędów, czyli bugfixingu. Zdarza się, że podczas CR można poznać przyczynę wystąpienia konkretnego błędu oraz rozwiązanie zaproponowane przez innego programistę. W przyszłości mogę więc z tego rozwiązania skorzystać. - Na bieżąco zaznajamiam się z przyrostem aplikacji
Nawet jeżeli kod jest napisany w taki sposób, że nie ma do niego żadnych komentarzy, to samo czytanie go daje świadomość istnienia pewnych elementów, z których mogę chcieć skorzystać w przyszłości, np, z jakichś metod stworzonych do ogólnego wykorzystania. - Uczę się na błędach innych
Code review każdego pull requesta wykonuję nie tylko ja, ale również inni programiści, którzy wskazują autorowi kodu miejsca, które ten powinien poprawić. Nierzadko w komentarzach pojawiają się linki do dokumentacji czy innych cennych źródeł. - Poznaję logikę aplikacji w praktyce
Dzięki rozwiązaniom implementowanym przez moich kolegów poznaję dokładnie flow aplikacji, nad którą pracuję. Jest to na tyle rozbudowany projekt, że nie szans dokładnie zgłębić każdego fragmentu każdej funkcjonalności, więc dzięki review mam ogólne pojęcia na temat tego, co to oprogramowanie potrafi zrobić.
No dobrze, ale jak właściwie wygląda moje code review?
Code review w praktyce
Do review z reguły siadam rano, około 9:30, tuż po spotkaniu Daily Scrum. Przeglądam listę pull requestów, do których jestem przypisana na reviewer. Sprawdzam również z grubsza pozostałe, bo zdarza się, że ktoś napisał kod front-endowy nie oznaczając wszystkich front-end developerów (co oczywiście nie jest konieczne, ale jest traktowane jako dobra praktyka).
Zdarza się również, że jakieś zadanie ma szczególny priorytet i należy je przetestować jak najszybciej, wtedy prośby o bezzwłoczny CR pojawiają się na dedykowanym kanale na Slacku.
Z reguły nazwa brancha, którego kod ma być scalony z branchem docelowym zawiera informację o tytule zadania. Jeżeli więc nazwa brancha i tytuł pull requesta dotyczą np. zmiany treści komunikatu błędu, lub zmiany koloru elementu, to od razu przechodzę do czytania kodu. Jeżeli jednak tytuł jest niewiele mówiący, a kodu wprowadzonych zmian jest sporo, wtedy przed code review zaglądam do Jiry, czytam opis zadania i jego kryteria akceptacji.
Na co zwracam uwagę?
Przyjmijmy zatem, że mam już otwarty pull request i otwieram pierwszy plik. Na co zwracam uwagę? Oczywiście mój opis będzie mocno związany z językiem, w którym piszę i frameworkiem, w którym odbywa się development czyli TypeScript i Angluar.
- Brak stałych elementów
Na wiele z błędów zwróci autorowi uwagę kompilator oraz narzędzie do pre-commitowania ustawione w odpowiedni sposób. Samo IDE również punktuje pewne niedociągnięcia, w zależności od ustawień. Pewne rzeczy jednak nie zostają z góry określone, a jednak należy o nich pamiętać. Do tych elementów należą np. typy danych zwracanych przez metody. - Elementy zbędne
Bezużyteczne konstruktory, nieużywane metody cyklu życia czy zbędne importy to zdecydowanie niepotrzebne linie w kodzie. - Wykorzystanie generycznych metod z serwisów
Z czasem dorabiamy się w projekcie coraz większych zasobów metod lub całych klas abstrakcyjnych, z których korzystamy w wielu miejscach. Pisanie ponownie metody, która robi praktycznie to samo co generyczna metoda z serwisu jest zatem niepotrzebne. - Wykorzystanie dedykowanych komponentów
Podobnie jak wyżej: w projekcie powstają generyczne komponenty, które rozszerzają docelowy element o potrzebne funkcje oraz niezbędne stylowanie. Dla przykładu: elementy formularza zostały tak przystosowane w postaci wewnętrznych komponentów, że wystarczy użyć jednego komponentu aby obsłużyć input, label oraz komunikat o błędzie z walidatora. Ktoś to już za nas napisał i dopóki nie trzeba w ekstremalny sposób modyfikować tego komponentu, tak długo nie ma potrzeby pisać tego kodu ponownie. - Fromatowanie kodu
To również do pewnego stopnia można załatwić ustawieniami globalnymi w projekcie. Zdarza się jednak, że gdzieniegdzie występuje zbędny enter czy podwójna spacja. - Stosowanie konwencji
Konwencji w projekcie jest sporo, ot choćby rozpoczynanie nazw metod od „on…”, a metod zwracających boolean od „onIs…”. Zachowanie spójnych konwencji nazewniczych znacznie ułatwia czytanie kodu, zwłaszcza kiedy upłynie sporo czasu od implementacji. - Prostota zaproponowanego rozwiązania
Ten punkt staram się wprowadzać w życie, choć nie jestem jeszcze mistrzem w upraszczaniu rozwiązań. Dorosłam jako programistka do tego, że po wymyśleniu szkieletu skomplikowanego rozwiązania potrafię zaorać swój pomysł, żeby znaleźć rozwiązanie obejmujące bardzo proste i nieskomplikowane zmiany w kodzie. Mam jednak trudności z wczuciem się w cudzy kod na tyle, żeby zaproponować rozwiązanie prostsze od proponowanego, chyba że jest to miejsce w kodzie, które dobrze znam. - Logika biznesowa
Tutaj sytuacja jest bardziej skomplikowana. Jeżeli nie znam bardzo dokładnie założeń analitycznych konkretnej funkcjonalności, to bardzo łatwo o pomyłkę. Np. pewne funkcjonalności są różnie dostępne dla użytkowników w różnych rolach. Jednak pewne ustawienia konkretnego użytkownika mogą wpływać na dostępność tych funkcjonalności. W takich skomplikowanych sytuacjach warto zajrzeć do dokumentacji konkretnej funkcjonalności lub rozwiać wątpliwości z pomocą analityków. - Wpływ zmian na inne miejsca w kodzie
Może się zdarzyć, że autor wprowadził zmianę, która spowoduje modyfikację działania aplikacji w innych miejscach. Jeżeli nie takie było założenie zadania, warto dopytać autora czy ma świadomość tych zmian.
Podsumowanie
W telegraficznym skrócie review do czytanie, analizowanie, zrozumienie i pisanie komentarzy. Komentarze to oczywiście nie tylko sugestie zmian, ale pytania do autora kodu. Przy okazji review można oznaczyć innego programistę i poprosić go o konsultację. Rzecz jasna staram się, aby moje komentarze nie brzmiały ofensywnie (pewnie nie zawsze mi to wychodzi) i były w miarę możliwości jak najbardziej precyzyjne.