Pull Request i Code Review, czyli o empatii w programowaniu

Dla pracy w zespole programistycznym korzystanie z systemu kontrolu wersji jest swego rodzaju normą. W dzisiejszym wpisie postaramy się opisać, co zrobić, by dodawany kod, był jak najlepszej jakości i cały zespół uczył się dzięki jego powstawaniu.

Czym jest właściwie Code Review?

Jeśli wiesz doskonale czym jest code review i pull request, przejdź od razu do dalszej części wpisu. Jeśli jednak dopiero zaczynasz swoją przygodę z IT po prostu rozwiń pytanie poniżej.

Czym jest code review i pull request?

Zanim przejdziemy do techniczego wyjaśnienia zacznijmy od porównania bardziej z życia. Pamiętasz, jak będąc w podstawówce prosiłeś mamę albo starszą siostrę o sprawdzenie wypracowania z języka polskiego/zadania z matematyki? Zanim je oddałeś, chciałeś mieć pewność, że nie ma tam błędów i wszystko jest zrozumiałe.  U mnie w szczególności na początku nauki było musiałam poprawiać swoje wypowiedzi pisemne, bo zjadałam literki, nie wspominając już, że czasem moje pisanie przypominało niezłe hiroglify. Dobrze pamietam jak w pierwszej czy drugiej klasie po zakończonym pisaniu, kładłam zeszyt na ławie w dużym pokoju i czekałam kiedy to mama, albo tato, albo siostra będą mieli chwilkę by na to spojrzeć. Często lepiej było pisać na brudno, żeby nie musieć aż tyle kreślić i korektorować. Po otrzymaniu uwag pracę poprawiałam/przepisywałam i prosiłam o dodatkowe spojrzenie. Czasem wystarczyło tylko użyć w jednym miejscu korektora, czasem przepisać wszystko od nowa. W końcu do pani w szkole szło poprawione zadanie, a ja starałam się zapamiętywać te wszystkie ortograficzne reguły. Moją pracę z rodziną nad taką pracą domową można porównać do pull requestów i  code review.

Gdy programista zaczyna pracować nad nowym zadaniem tworzy na nie osobną gałąź (branch) od głowego kodu w repozytorium. Dzięki temu, każdy może swodobnie pracować nad swoim zadaniem, a gdy jest ono gotowe może utworzyć pull request (tak jak ja odkładałam zeszyt na ławie).

Pull request to nic innego jak powiedzenie członkom zespołu: Hej, patrzcie, zakończyłem swoje zadanie! Oto moje zmiany. Chce je połączyć z głównym kodem w repozytorium. Znając historie sprawdzania mojego pisania przez członków rodziny, możecie się domyślać, że niedołączną częścią pull requestu jest inspekcja kodu, czyli Code Review.

Code Review (pozwólcie, że będę używać przystępniejszej i popularniejszej angielskiej nazwy), to nic innego sprawdzanie kodu przez zespół programistów. Standardową praktyką jest, że towarzyszy ono każdemu pull-requestowi. Tak, by jakość dodawanego kodu była coraz lepsza, a cały zespół miał kontrolę/wiedzę o kodzie źródłowym aplikacji. Podczas takiego sprawdzenia, ważne jest nie tylko skupienie się na samym kodzie, ale też zrozumienie funkcji jaką on ze sobą niesie, bo przecież w rezultacie, to użytkownik ma być z tego kodu zadowolny.

Naczynia połączone

Pull request i Code Review są niemal nierozłączne (czasem można pomyśleć o review już istniejącego kodu, np. gdy zespół przejmuje go od innego zespołu) i  jakość jednego mocno zależy od jakości drugiego. Źle przygotowany PR (Pull Request) nie zachęca do oceny,a brak czy, źle przeprowadzone Code Review nie podniosą jakości pull reqesta. W obu najważniejsza chyba jest empatia i nastawienie na drugą stronę, a nie na siebie i dostrzeganie oczywistych rzeczy, które dla drugiej strony takie oczywiste być nie muszą. Co więc zrobić, by przygotować jak najlepszy PR, oraz jak dostarczyć wartościowe code review? Mam nadzieję, że w tym wpisie znajdziemy odpowiedź na to pytanie.

Pull Request

Tutaj pracę należy zacząć od samego początku. Masz przypisane do siebie jakieś User Story, warto już na samym początku dobrze je opisać (np. za pomocą sesji three amigos) i podzielić na jak najmniejsze zadania. Dla każdego zadania o ile to możliwe i logiczne tworzyć osobny PR.

’10 lines of code = 10 issues. 500 lines of code = “looks fine.” Code reviews.’

— I Am Devloper (@iamdevloper) November 5, 2013

Naturalną rzeczą jest, że przy dużej ilości kodu jest go trudniej zrozumieć, a także trudniej wyłapywać błędy. Duże zadania mogą być też zagrożeniem jeśli zachorujesz/niespodziewanie weźmiesz kilka dni wolnego, a ktoś kto będzie chciał dokończyć twoją pracę będzie musiał się przez ten ogromny fragment kodu przekopać ;)

Po drugie, warto już od początku pamiętać, że ktoś będzie nasz kod oceniał. Dlatego, trzeba commitować często i  z sensownymi opisami. A tworząc PR opisać, co się zrobiło. Najważniejsze, to podlinować swoje zadanie (częstą praktyką jest po prostu tworzenie branchy z nazwą rozpoczynającą się od numeru zadania). U mnie w zespole przyjęło się, że przy większej ilości zmian dzieli się je na MAJOR: czyli te, które przynoszą funkcjonalość i MINOR: czyli dodatkowe, związane z usprawnieniem i refaktorem kodu (drobnym refaktorem, większy idzie w osobnym PRze!). W ten sposób dzieli się je w opisie PR, co fajnie pozwala na wyłapanie, co ktoś zrobił i dlaczego coś tykał, skoro wcale nie powinien :P.

Po trzecie, oceniający kod patrzą na produkt finalny i zazwyczaj nie wiedzą, co się działo podczas implementacji. Dlatego, jeśli odrzuciłeś jakieś oczywiste rozwiązanie, albo zastosowałeś nowe podejście – lepiej od razu opisać dlaczego tak się stało, bo możesz być pewien, że będzie to jedno z pierwszych pytań jakie padnie. Jeśli widzisz potrzebę, opisz swoją drogę, tok myślenia związany z doborem rozwiązania.

Po czwarte, bądź jak dobry skaut – zawsze zostawiaj kod lepszym niż go zastałeś ;) Patrz na klasy w których pracujesz, metody, które modyfikujesz, testy które dopisujesz uważnie i dostrzegaj niezbędne zmiany. Jeśli pracujesz z legacy codem, dodawaj testy, dokumentacje, nie tylko na poziomie javaDoców, ale też z punktu widzenia How to develop new … – uratuj kolejnemu developerowi godziny, które sam spędziłeś na rozgryzaniu aplikacji.

Po piąte, testuj odpowiedzialnie. Nie raz widziałam testy, które nie sprawdzają nic, ale podnoszą code coverage, testy które nie obejmują wszystkich ścieżek, czy w końcu brak testów integracyjnych, przez co ciężko stwierdzić jak dana funkcjonalność działa.

Po szóste, zanim klikniesz ‚create’ – zrób sam sobie code review. Sprawdź, czy wszystko jest jak należy, czy ostatnie zmiany są wypushowane, a w kodzie nie ma oczywistych błędów. Jeszcze raz odpal wszystkie testy, sprawdź czy projekt się buduje. Tak na wszelki wypadek ;)

Po siódme, bądź pierwszym komentującym – jeśli nie jesteś czegoś pewny, zapytaj o to osoby, które dodałeś do PR.

Posumowując:

  1. Atomiczne zadania = krótsze PR – dokładniejsze Code Review. 
  2. Podlinkowanie zadania.  Znaczące nazwy commitów. Podział zmian na te kluczowe dla funkcjonalności i te zwiazane z usprawieniem kodu.
  3. Skomentuj nieoczywiste rozwiązania, opisz drogę jaką przeszedłeś. 
  4. Zawsze zostawiaj kod lepszym niż go zastałeś
  5. Testuj odpowiedzialnie
  6. Zanim utworzysz PR sam zrób sobie code review
  7. Jeśli wahasz się nad poprawnością danego fragmentu kodu – skomentuj i zapytaj inne osoby włączone w PR o zdanie

Code Review

Dostałeś właśnie powiadomienie, że ktoś dodał Cię do PR i chcesz zacząć Code Review. Uczciwie jest zarezerwować na to trochę czasu. Znacznie łatwiej wgryźć się w kod raz a porządnie, niż zmieniać wątek swojej pracy i robić to obok drugiego zadania.

Rozpocznij od sprawdzenia jakie było zadanie. Jeśli coś nie jest do końca jasne spróbuj dopytać czy to developera czy to biznes analityka. Warto wiedzieć, co ten kod ma robić nie tylko z niego, bo czasem okazuje się, że developer zapomni o jednym przypadku, albo coś źle zinterpretuje – może będziesz mieć okazje to wyłapać.

Po drugie, dostosuj komentarze do ich odbiorców, czyli całego zespołu. Pamiętaj, że code review to nie tylko dbanie o jakość kodu, ale też o przepływ wiedzy. Często od niego zaczyna się np. wdrażanie nowych osób do zespołu. Jest to również sposób na przemycenie sporej wiedzy dla mniej doświadczonych członków zespołu. Zadawaj szczegółowe pytania (angażują i zmuszają do samodzielnego myślenia), a w razie potrzeby wyjaśniaj, nie nakazuj – autor kodu więcej się wtedy nauczy, a i przyjemniej będzie mu z takim feedbackiem pracować. Nie bój się linkować do dokumentacji, dobrych praktyk, poprzedniego kodu. Uczcie się razem dzięki dobrze umotywowanym komentarzom.

Po trzecie, nie bój się pytać. Nie zawsze ma się od razu gotowe lepsze rozwiązanie – niemniej warto wypuktować, że coś się nie podoba/ wzbudza wątpliwości. Zapytać innych członków zespołu o opinie czy dopytać autora. Code review powinno być rozmową o kodzie i obie strony powinny być otwarte zarówno na zadawanie, jak i odpowiadanie na pytania. Nie ma nic złego w stwierdzeniach: „nie jestem pewna/wydaje mi się”. Ten punkt jak najbardziej odnosi się też do autora PR – jeśli czegoś nie rozumiesz w sugestiach od oceniających: pytaj śmiało!

Po czwarte, najlepsze techniczne rozwiązanie nie oznacza najlepszego rozwiązania dla Twojego projektu. Trzeba pomyśleć o całej organizacji, użytkowniku, ograniczeniach czy szansach jakie przed Wami stoją. Kod to tylko nasze narzędzie do dochodzenia do pewnych rozwiązań, nie cel programowania sam w sobie.

Po piąte, zawsze można coś napisać lepiej. Inaczej. Ale często te zmiany nie mają wpływu na ostateczne działanie Twojej aplikacji. Refaktor do końca świata nie ma sensu (choć wiemy, że kusi), a w projektach spójność jest ważniejsza od poprawności. Jeśli w całym projekcie używana jest technologia X, a Ty chcesz zaproponować dodanie Y bo jest lepsze, może stanowić to problem, przede wszystkim w dalszym rozwoju aplikacji. Mamy doświadczenia w pracy w projekcie, który był ogromnym monolitem z bardzo skąpą dokumentacją – jedyne, co nas ratowało, to właśnie spójność i konsekwencja. Dzięki niej po kilku dniach pracy z jego interpretacją dostrzegaliśmy wzory i ścieżki jakie doprowadzą i nasze funkcjonalnosci do działania.

Po szóste, takie pomysły na refaktor w całej aplikacji trzeba zbierać – w idealnym świecie zakładając od razu zadania, lub przemycając je w ramach kolejnego PR w ramach bieżącego taska, w bardziej realistycznym przypadku poruszając je podczas kolejnego Sprint Planningu czy planowania na kwartał. Podobnie, gdy zobaczycie, że w bieżącym kodzie coś jest bardzo nie tak – zarówno w trakcie tworzenia kodu jak i podczas code review.

Po siódme, zawsze odpowiadaj na komentarze. Jeśli jesteś autorem Pull Requestu, twórz na ich podstawie zadania (większość narzędzi na to pozwala), a także dawaj znać, że dana poprawka jest już naniesiona.

Na koniec – Code Review jest o kodzie. Każdy programista popełnia błędy, a także nieustannie poprawia jakość swojego kodu. Nie bierz komentarzy personalnie, ucz się na ich podstawie. Komentując czyjś kod również miej to na uwadze.  Jeśli któraś ze stron poczuje się urażona sposobem pisania komentarzy warto o tym wspomnieć i  wspólnie wypracować przyjazny język.

Podsumowując:

  1. Zrozum zadanie
  2. Wyjaśniaj, by każdy z tego mógł skorzystać
  3. Pytaj, proś o objaśnienia
  4. Najlepsze techniczne nie jest zawsze najlepszym dla projektu
  5. Nie refaktorujcie do końca świata, niektóre „ulepszenia” to bardziej kosmetyka, która nie wpłynie na funkcjonalność aplikacji
  6. Zbierajcie dług technologiczny i adresujcie go
  7. Zawsze odpowiadaj na komentarze
  8. Code Review jest o kodzie

Zasady, triki, narzędzia

Specialnie powyżej skupiłam się na postawach, a nie na konktetnych narzędziach czy zasadach, które również można zastosować, bo wydaje mi się, że praca nad nimi wymaga znacznie więcej pracy. Niemniej, nie sposób nie wspomnieć o zasadach, których wcielenie  może ułatwić Wam Pull Request – Code Review.

  • Opracujcie sobie checklistę przed Pull Requestem. Podstawowe punkty to: kod działa, testy przechodzą, kod jest sformatowany, stosujemy się do zasad Clean Code, robiąc UI do PR dodajemy rzut ekranu, dodana niezbędna dokumentacja. Przykładową listę znajdziecie np. tutaj. Jeśli korzystanie z GitHuba czy Stasha w cloudzie, możecie automatycznie dołączać listę do Waszych PR, a jej odhaczenie może być warunkiem rozpoczęcie Code Review. Warto pamiętać, że zbyt długa lista zasad to nic dobrego, a czasem zamiast stosujmy sie do Clean Code, warto zbierać rzeczy, które najsłabiej idą zespołowi i je wyszczegółowić.
  • Design review sposobem na szybką reakcje na nienajlepsze pomysły. Jeśli implementujecie wiele nowych funkcjonalności, warto dodać inspekcje pomysłu – zweryfikować flow i proponowaną architetkurę jeszcze zanim zostanie ona zaimplementowana.
  • Korzystajcie z gotowych narzędzi:
    • Statyczna analiza kodu: SonarCube, Checkstyle, ESLint, czy nawet coś do wyłapywania litetówek w kodzie – wszystko, to powinno być odpalone przed PR tak, by mieć pewność, że nie obniżyło się  jego jakości. Autoformatowanie kodu, czy organizacja importów na zapisie pliku to powinna być norma.
    • Narzędzia do code-coverage np. Jacoco i inne, by mieć pewność, że wszystko jest przetestowane.
    • Narzędzia do CI np. Jenkins – warto ustawić odpalenie builda na każdy PR commit, by mieć pewność, że projekt się buduje.
    • Narzędzia do Code Review, wiele narzędzi do kontroli wersji ma je wbudowane, jeśli nie korzystacie z takiego, albo szukacie lepszego,  warto spojrzeć na: Gerrit, Upsource, Collaborator czy Reviewable
  • Miejcie czas dla Code Review. U nas w projekcie na początku był problem z wyezekwowaniem Code Review. Czasem trzeba było czekać i 2 dni. Wszyscy zgodziliśmy, się, że tak być nie może i naszym obowiązkiem jest zaczynać od niego dzień. Na początku i mi nie szło to najlepiej, bo wolałam robić Code Review na oddech od swojej pracy, ale szybko się do tego przystosowałam. Dzięki takiej postawie przed lunchem wszystko jest ocenione, a często i poprawione i gotowe do merge’u.
  • LGTM -looks good to me. To skrót, który przyjął się w moim zespole a oznacza nic innego, niż to, że zaakceptowanie PR nie odbyło się przez przypadek. W poprzednim zespole skolei, po zakończonym Code Review pisaliśmy komentarz, że już skonczyliśmy i można zacząć nanosić poprawki. Mała rzecz, bardzo zależna od teamu, ale może ułatwić pracę.
  • Dodaj do Pull Reqesta niezbędne osoby. Im mniej osób, tym większą odpowiedzialność one czują, a więc i review idzie szybciej. Częstą praktyką jest, że  dwie osoby wystarczą, jeśli funkcjonalność jest „znana” , jest to tylko jej rozszerzenie ,lub jest to bugfix. Z drugiej strony, gdy pracujecie w kilka zespołów nad jednym produktem pamiętaj, by zawsze dawać znać, o swoich zmianach jeśli wpływają one na ich pracę (wtedy częstą praktyką jest dodawanie jednej osoby z każdego zespołu, nierzadko tylko w charakterze akceptacji zmian (sprawdzenia czy nie popsują/nie duplikują się z pracą jej zespołu) niż samej inspekcji jakości rozwiązania. Jeśli czujesz potrzebę dodania kogoś niestandardowego -np. superwymiatacza w danej technologii, daj mu też znać na komunikatorze czy osobiście, żeby dany PR mu nie uciekł.
  • Ustalcie kto ‚kilka merge’. Zazwyczaj albo ten, kto utworzył PR, albo ten kto uznał, że potrzebuje tych zmian, zaakceptował i odblokował przycisk(zazwyczaj merge jest możliwy gdy X osób zaakceptuje dany PR, gdy PR buduje się na środowisku itp, można dodać swoje zasady).

Patrząc na ten wpis można dość do wniosku, że dobry Pull Request i Code Review to sporo pracy. I ja się z tym absolutnie zgadzam. Jednak myślę, że znacznie więcej w tym pracy na postawie, nastawieniu i zrozumieniu, niż technicznej akrobatyki.W rezultacie chcemy przecież uzyskać kod, zrozumiały jak proza, który zadowala potrzeby użytkownika. I jakby nie patrzeć znacznie łatwiej zrobić to z pomocą całego zespołu niż samodzielnie. A PR/Code Review jest jedną z najlepszych opcji by uzyskać takie zaangażowanie. Aby to osiągnąć warto skupić się na potrzebie tej drugiej strony, by swoją postawą ułatwić jej aktywny udział w dostarczaniu dobrych rozwiązań.

  • 2
  •  
  •  
  •  
  •  
  • justa

    Paniu Aniu czy odpowiada Pani na maile?? – Justyna K.

    • Tak, staramy się odpowiadać na maile, niestety z braku czasu często czekają one kilka tygodni w kolejce.

    • tak, odpowiadamy na maile, natomiast, zazwyczaj na odpowiedź trzeba trochę poczekać.

  • kawa

    Ciekawy tekst, większość rad odnośnie sposobu pracy i commitowania stosowałam intuicyjnie, chociaż niestety z „Pull Request” i „Code Review” jeszcze nie miałam do czynienia.

    Mała pomoc językowa:

    „gdy zobaczycie, że w bierzącym kodzie coś jest bardzo nie tak” -> „gdy zobaczycie, że w bieżącym kodzie coś jest bardzo nie tak”

    „Moją pracą z rodziną nad taką pracą domową można porównać do pull requestów” -> „Moją pracę z rodziną nad taką pracą domową można porównać do pull requestów”

    „A by to osiągnąć warto skupić się na potrzebie tej drugiej strony” -> „Aby to osiągnąć warto skupić się na potrzebie tej drugiej strony”

    To nie jest złośliwość, ja takie rzeczy po prostu widzę w trakcie czytania ;)