Pull Request i Code Review, czyli o empatii w programowaniu

Dla pra­cy w zes­pole pro­gramisty­cznym korzys­tanie z sys­te­mu kon­trolu wer­sji jest swego rodza­ju nor­mą. W dzisiejszym wpisie postaramy się opisać, co zro­bić, by dodawany kod, był jak najlep­szej jakoś­ci i cały zespół uczył się dzię­ki jego pow­stawa­niu.

Czym jest właściwie Code Review?

Jeśli wiesz doskonale czym jest code review i pull request, prze­jdź od razu do dal­szej częś­ci wpisu. Jeśli jed­nak dopiero zaczy­nasz swo­ją przy­godę z IT po pros­tu rozwiń pytanie poniżej.

Czym jest code review i pull request?

Zan­im prze­jdziemy do tech­niczego wyjaśnienia zaczni­jmy od porów­na­nia bardziej z życia. Pamię­tasz, jak będąc w pod­stawów­ce prosiłeś mamę albo starszą siostrę o sprawdze­nie wypra­cow­a­nia z języ­ka polskiego/zadania z matem­aty­ki? Zan­im je odd­ałeś, chci­ałeś mieć pewność, że nie ma tam błędów i wszys­tko jest zrozu­mi­ałe.  U mnie w szczegól­noś­ci na początku nau­ki było musi­ałam popraw­iać swo­je wypowiedzi pisemne, bo zjadałam liter­ki, nie wspom­i­na­jąc już, że cza­sem moje pisanie przy­pom­i­nało niezłe hirogli­fy. Dobrze pami­etam jak w pier­wszej czy drugiej klasie po zakońc­zonym pisa­niu, kładłam zeszyt na ław­ie w dużym poko­ju i czekałam kiedy to mama, albo tato, albo sios­tra będą mieli chwilkę by na to spo­jrzeć. Częs­to lep­iej było pisać na brud­no, żeby nie musieć aż tyle kreślić i korek­torować. Po otrzy­ma­niu uwag pracę poprawiałam/przepisywałam i prosiłam o dodatkowe spo­jrze­nie. Cza­sem wystar­czyło tylko użyć w jed­nym miejs­cu korek­to­ra, cza­sem przepisać wszys­tko od nowa. W końcu do pani w szkole szło popraw­ione zadanie, a ja starałam się zapamię­ty­wać te wszys­tkie ortograficzne reguły. Moją pracę z rodz­iną nad taką pracą domową moż­na porów­nać do pull requestów i  code review.

Gdy pro­gramista zaczy­na pra­cow­ać nad nowym zadaniem tworzy na nie osob­ną gałąź (branch) od głowego kodu w repozy­to­ri­um. Dzię­ki temu, każdy może swodob­nie pra­cow­ać nad swoim zadaniem, a gdy jest ono gotowe może utworzyć pull request (tak jak ja odkładałam zeszyt na ław­ie).

Pull request to nic innego jak powiedze­nie członkom zespołu: Hej, patrz­cie, zakończyłem swo­je zadanie! Oto moje zmi­any. Chce je połączyć z głównym kodem w repozy­to­ri­um. Zna­jąc his­to­rie sprawdza­nia mojego pisa­nia przez członków rodziny, może­cie się domyślać, że niedołączną częś­cią pull reques­tu jest inspekc­ja kodu, czyli Code Review.

Code Review (pozwól­cie, że będę uży­wać przys­tęp­niejszej i pop­u­larniejszej ang­iel­skiej nazwy), to nic innego sprawdzanie kodu przez zespół pro­gramistów. Stan­dar­d­ową prak­tyką jest, że towarzyszy ono każde­mu pull-requestowi. Tak, by jakość dodawanego kodu była coraz lep­sza, a cały zespół miał kontrolę/wiedzę o kodzie źródłowym aplikacji. Pod­czas takiego sprawdzenia, ważne jest nie tylko skupi­e­nie się na samym kodzie, ale też zrozu­mie­nie funkcji jaką on ze sobą niesie, bo prze­cież w rezulta­cie, to użytkown­ik ma być z tego kodu zad­owol­ny.

Naczynia połączone

Pull request i Code Review są niemal nierozłączne (cza­sem moż­na pomyśleć o review już ist­niejącego kodu, np. gdy zespół prze­j­mu­je go od innego zespołu) i  jakość jed­nego moc­no zależy od jakoś­ci drugiego. Źle przy­go­towany PR (Pull Request) nie zachę­ca do oceny,a brak czy, źle przeprowad­zone Code Review nie pod­niosą jakoś­ci pull reqes­ta. W obu najważniejsza chy­ba jest empa­tia i nastaw­ie­nie na drugą stronę, a nie na siebie i dostrze­ganie oczy­wistych rzeczy, które dla drugiej strony takie oczy­wiste być nie muszą. Co więc zro­bić, by przy­go­tować jak najlep­szy PR, oraz jak dostar­czyć wartoś­ciowe code review? Mam nadzieję, że w tym wpisie zna­jdziemy odpowiedź na to pytanie.

Pull Request

Tutaj pracę należy zacząć od samego początku. Masz przyp­isane do siebie jakieś User Sto­ry, warto już na samym początku dobrze je opisać (np. za pomocą sesji three ami­gos) i podzielić na jak najm­niejsze zada­nia. Dla każdego zada­nia o ile to możli­we i log­iczne tworzyć osob­ny PR.

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

— I Am Devlop­er (@iamdevloper) Novem­ber 5, 2013

Nat­u­ral­ną rzeczą jest, że przy dużej iloś­ci kodu jest go trud­niej zrozu­mieć, a także trud­niej wyłapy­wać błędy. Duże zada­nia mogą być też zagroże­niem jeśli zachorujesz/niespodziewanie weźmiesz kil­ka dni wol­nego, a ktoś kto będzie chci­ał dokończyć two­ją pracę będzie musi­ał się przez ten ogrom­ny frag­ment kodu przekopać ;)

Po drugie, warto już od początku pamię­tać, że ktoś będzie nasz kod oce­ni­ał. Dlat­ego, trze­ba com­mi­tować częs­to i  z sen­sowny­mi opisa­mi. A tworząc PR opisać, co się zro­biło. Najważniejsze, to podli­nować swo­je zadanie (częstą prak­tyką jest po pros­tu tworze­nie branchy z nazwą rozpoczy­na­jącą się od numeru zada­nia). U mnie w zes­pole przyjęło się, że przy więk­szej iloś­ci zmi­an dzieli się je na MAJOR: czyli te, które przynoszą funkcjon­alość i MINOR: czyli dodatkowe, związane z usprawnie­niem i refak­torem kodu (drob­nym refak­torem, więk­szy idzie w osob­nym PRze!). W ten sposób dzieli się je w opisie PR, co fajnie pozwala na wyła­panie, co ktoś zro­bił i dlaczego coś tykał, sko­ro wcale nie powinien :P.

Po trze­cie, oce­ni­a­ją­cy kod patrzą na pro­dukt final­ny i zazwyczaj nie wiedzą, co się dzi­ało pod­czas imple­men­tacji. Dlat­ego, jeśli odrzu­ciłeś jakieś oczy­wiste rozwiązanie, albo zas­tosowałeś nowe pode­jś­cie — lep­iej od razu opisać dlaczego tak się stało, bo możesz być pewien, że będzie to jed­no z pier­wszych pytań jakie pad­nie. Jeśli widzisz potrze­bę, opisz swo­ją drogę, tok myśle­nia związany z doborem rozwiąza­nia.

Po czwarte, bądź jak dobry skaut — zawsze zostaw­iaj kod lep­szym niż go zas­tałeś ;) Patrz na klasy w których pracu­jesz, metody, które mody­fiku­jesz, testy które dopisu­jesz uważnie i dostrze­gaj niezbędne zmi­any. Jeśli pracu­jesz z lega­cy codem, dodawaj testy, doku­men­tac­je, nie tylko na poziomie javaDoców, ale też z punk­tu widzenia How to devel­op new … — uratuj kole­jne­mu devel­oper­owi godziny, które sam spędz­iłeś na roz­gryza­niu aplikacji.

Po piąte, tes­tuj odpowiedzial­nie. Nie raz widzi­ałam testy, które nie sprawdza­ją nic, ale pod­noszą code cov­er­age, testy które nie obe­j­mu­ją wszys­t­kich ścieżek, czy w końcu brak testów inte­gra­cyjnych, przez co ciężko stwierdz­ić jak dana funkcjon­al­ność dzi­ała.

Po szóste, zan­im klikniesz ‘cre­ate’ — zrób sam sobie code review. Sprawdź, czy wszys­tko jest jak należy, czy ostat­nie zmi­any są wypushowane, a w kodzie nie ma oczy­wistych błędów. Jeszcze raz odpal wszys­tkie testy, sprawdź czy pro­jekt się budu­je. Tak na wszel­ki wypadek ;)

Po siódme, bądź pier­wszym komen­tu­ją­cym — jeśli nie jesteś czegoś pewny, zapy­taj o to oso­by, które dodałeś do PR.

Posumowu­jąc:

  1. Atom­iczne zada­nia = krót­sze PR — dokład­niejsze Code Review. 
  2. Podlinkowanie zada­nia.  Znaczące nazwy com­mitów. Podzi­ał zmi­an na te kluc­zowe dla funkcjon­al­noś­ci i te zwiazane z uspraw­ie­niem kodu.
  3. Sko­men­tuj nieoczy­wiste rozwiąza­nia, opisz drogę jaką przeszedłeś. 
  4. Zawsze zostaw­iaj kod lep­szym niż go zas­tałeś
  5. Tes­tuj odpowiedzial­nie
  6. Zan­im utworzysz PR sam zrób sobie code review
  7. Jeśli wahasz się nad poprawnoś­cią danego frag­men­tu kodu — sko­men­tuj i zapy­taj inne oso­by włąc­zone w PR o zdanie

Code Review

Dostałeś właśnie powiadomie­nie, że ktoś dodał Cię do PR i chcesz zacząć Code Review. Ucz­ci­wie jest zarez­er­wować na to trochę cza­su. Znacznie łatwiej wgryźć się w kod raz a porząd­nie, niż zmieni­ać wątek swo­jej pra­cy i robić to obok drugiego zada­nia.

Rozpocznij od sprawdzenia jakie było zadanie. Jeśli coś nie jest do koń­ca jasne spróbuj dopy­tać czy to devel­opera czy to biznes anal­i­ty­ka. Warto wiedzieć, co ten kod ma robić nie tylko z niego, bo cza­sem okazu­je się, że devel­op­er zapom­ni o jed­nym przy­pad­ku, albo coś źle zin­ter­pre­tu­je — może będziesz mieć okaz­je to wyła­pać.

Po drugie, dos­to­suj komen­tarze do ich odbior­ców, czyli całego zespołu. Pamię­taj, że code review to nie tylko dban­ie o jakość kodu, ale też o przepływ wiedzy. Częs­to od niego zaczy­na się np. wdrażanie nowych osób do zespołu. Jest to również sposób na prze­myce­nie sporej wiedzy dla mniej doświad­c­zonych członków zespołu. Zadawaj szczegółowe pyta­nia (angażu­ją i zmusza­ją do samodziel­nego myśle­nia), a w razie potrze­by wyjaś­ni­aj, nie nakazuj — autor kodu więcej się wtedy nauczy, a i przy­jem­niej będzie mu z takim feed­back­iem pra­cow­ać. Nie bój się linkować do doku­men­tacji, dobrych prak­tyk, poprzed­niego kodu. Ucz­cie się razem dzię­ki dobrze umo­ty­wowanym komen­tar­zom.

Po trze­cie, nie bój się pytać. Nie zawsze ma się od razu gotowe lep­sze rozwiązanie — niem­niej warto wypuk­tować, że coś się nie podoba/ wzbudza wąt­pli­woś­ci. Zapy­tać innych członków zespołu o opinie czy dopy­tać auto­ra. Code review powin­no być roz­mową o kodzie i obie strony powin­ny być otwarte zarówno na zadawanie, jak i odpowiadanie na pyta­nia. Nie ma nic złego w stwierdzeni­ach: “nie jestem pewna/wydaje mi się”. Ten punkt jak najbardziej odnosi się też do auto­ra PR — jeśli czegoś nie rozu­miesz w sug­es­ti­ach od oce­ni­a­ją­cych: pytaj śmi­ało!

Po czwarte, najlep­sze tech­niczne rozwiązanie nie oznacza najlep­szego rozwiąza­nia dla Two­jego pro­jek­tu. Trze­ba pomyśleć o całej orga­ni­za­cji, użytkown­iku, ograniczeni­ach czy szansach jakie przed Wami sto­ją. Kod to tylko nasze narzędzie do dochodzenia do pewnych rozwiązań, nie cel pro­gramowa­nia sam w sobie.

Po piąte, zawsze moż­na coś napisać lep­iej. Inaczej. Ale częs­to te zmi­any nie mają wpły­wu na ostate­czne dzi­ałanie Two­jej aplikacji. Refak­tor do koń­ca świa­ta nie ma sen­su (choć wiemy, że kusi), a w pro­jek­tach spójność jest ważniejsza od poprawnoś­ci. Jeśli w całym pro­jek­cie uży­wana jest tech­nolo­gia X, a Ty chcesz zapro­ponować dodanie Y bo jest lep­sze, może stanow­ić to prob­lem, przede wszys­tkim w dal­szym roz­wo­ju aplikacji. Mamy doświad­czenia w pra­cy w pro­jek­cie, który był ogrom­nym mono­litem z bard­zo skąpą doku­men­tacją — jedyne, co nas ratowało, to właśnie spójność i kon­sek­wenc­ja. Dzię­ki niej po kilku dni­ach pra­cy z jego inter­pre­tacją dostrze­gal­iśmy wzo­ry i ścież­ki jakie doprowadzą i nasze funkcjon­alnosci do dzi­ała­nia.

Po szóste, takie pomysły na refak­tor w całej aplikacji trze­ba zbier­ać — w ide­al­nym świecie zakłada­jąc od razu zada­nia, lub prze­my­ca­jąc je w ramach kole­jnego PR w ramach bieżącego tas­ka, w bardziej real­isty­cznym przy­pad­ku porusza­jąc je pod­czas kole­jnego Sprint Plan­ningu czy planowa­nia na kwartał. Podob­nie, gdy zobaczy­cie, że w bieżą­cym kodzie coś jest bard­zo nie tak — zarówno w trak­cie tworzenia kodu jak i pod­czas code review.

Po siódme, zawsze odpowiadaj na komen­tarze. Jeśli jesteś autorem Pull Reques­tu, twórz na ich pod­staw­ie zada­nia (więk­szość narzędzi na to pozwala), a także dawaj znać, że dana poprawka jest już nanie­siona.

Na koniec — Code Review jest o kodzie. Każdy pro­gramista popeł­nia błędy, a także nieustan­nie popraw­ia jakość swo­jego kodu. Nie bierz komen­tarzy per­son­al­nie, ucz się na ich pod­staw­ie. Komen­tu­jąc czyjś kod również miej to na uwadze.  Jeśli któraś ze stron poczu­je się urażona sposobem pisa­nia komen­tarzy warto o tym wspom­nieć i  wspól­nie wypra­cow­ać przy­jazny język.

Pod­sumowu­jąc:

  1. Zrozum zadanie
  2. Wyjaś­ni­aj, by każdy z tego mógł sko­rzys­tać
  3. Pytaj, proś o objaśnienia
  4. Najlep­sze tech­niczne nie jest zawsze najlep­szym dla pro­jek­tu
  5. Nie refak­toru­j­cie do koń­ca świa­ta, niek­tóre “ulep­szenia” to bardziej kos­me­ty­ka, która nie wpłynie na funkcjon­al­ność aplikacji
  6. Zbier­a­j­cie dług tech­no­log­iczny i adresu­j­cie go
  7. Zawsze odpowiadaj na komen­tarze
  8. Code Review jest o kodzie

Zasady, triki, narzędzia

Spe­cial­nie powyżej skupiłam się na postawach, a nie na konk­tet­nych narzędzi­ach czy zasadach, które również moż­na zas­tosować, bo wyda­je mi się, że pra­ca nad nimi wyma­ga znacznie więcej pra­cy. Niem­niej, nie sposób nie wspom­nieć o zasadach, których wcie­le­nie  może ułatwić Wam Pull Request — Code Review.

  • Opracu­j­cie sobie check­listę przed Pull Requestem. Pod­sta­wowe punk­ty to: kod dzi­ała, testy prze­chodzą, kod jest sfor­ma­towany, sto­su­je­my się do zasad Clean Code, robiąc UI do PR doda­je­my rzut ekranu, dodana niezbęd­na doku­men­tac­ja. Przykład­ową listę zna­jdziecie np. tutaj. Jeśli korzys­tanie z GitHu­ba czy Stasha w cloudzie, może­cie automaty­cznie dołączać listę do Waszych PR, a jej odhacze­nie może być warunk­iem rozpoczę­cie Code Review. Warto pamię­tać, że zbyt dłu­ga lista zasad to nic dobrego, a cza­sem zami­ast sto­su­jmy sie do Clean Code, warto zbier­ać rzeczy, które najsła­biej idą zespołowi i je wyszczegółow­ić.
  • Design review sposobem na szy­bką reakc­je na nien­ajlep­sze pomysły. Jeśli imple­men­tu­je­cie wiele nowych funkcjon­al­noś­ci, warto dodać inspekc­je pomysłu — zwery­fikować flow i pro­ponowaną architetkurę jeszcze zan­im zostanie ona zaim­ple­men­towana.
  • Korzys­ta­j­cie z gotowych narzędzi:
    • Staty­cz­na anal­iza kodu: Sonar­Cube, Check­style, ESLint, czy nawet coś do wyłapy­wa­nia litetówek w kodzie — wszys­tko, to powin­no być odpalone przed PR tak, by mieć pewność, że nie obniżyło się  jego jakoś­ci. Aut­o­for­ma­towanie kodu, czy orga­ni­za­c­ja importów na zapisie pliku to powin­na być nor­ma.
    • Narzędzia do code-cov­er­age np. Jaco­co i inne, by mieć pewność, że wszys­tko jest przetestowane.
    • Narzędzia do CI np. Jenk­ins — warto ustaw­ić odpale­nie buil­da na każdy PR com­mit, by mieć pewność, że pro­jekt się budu­je.
    • Narzędzia do Code Review, wiele narzędzi do kon­troli wer­sji ma je wbu­dowane, jeśli nie korzysta­cie z takiego, albo szuka­cie lep­szego,  warto spo­jrzeć na: Ger­rit, Upsource, Col­lab­o­ra­tor czy Review­able
  • Miej­cie czas dla Code Review. U nas w pro­jek­cie na początku był prob­lem z wyezek­wowaniem Code Review. Cza­sem trze­ba było czekać i 2 dni. Wszyscy zgodzil­iśmy, się, że tak być nie może i naszym obow­iązkiem jest zaczy­nać od niego dzień. Na początku i mi nie szło to najlepiej, bo wolałam robić Code Review na odd­ech od swo­jej pra­cy, ale szy­bko się do tego przys­tosowałam. Dzię­ki takiej postaw­ie przed lunchem wszys­tko jest oce­nione, a częs­to i popraw­ione i gotowe do merge’u.
  • LGTM -looks good to me. To skrót, który przyjął się w moim zes­pole a oznacza nic innego, niż to, że zaak­cep­towanie PR nie odbyło się przez przy­padek. W poprzed­nim zes­pole skolei, po zakońc­zonym Code Review pisal­iśmy komen­tarz, że już skon­czyliśmy i moż­na zacząć nanosić popraw­ki. Mała rzecz, bard­zo zależ­na od tea­mu, ale może ułatwić pracę.
  • Dodaj do Pull Reqes­ta niezbędne oso­by. Im mniej osób, tym więk­szą odpowiedzial­ność one czu­ją, a więc i review idzie szy­b­ciej. Częstą prak­tyką jest, że  dwie oso­by wystar­czą, jeśli funkcjon­al­ność jest “znana” , jest to tylko jej rozsz­erze­nie ‚lub jest to bug­fix. Z drugiej strony, gdy pracu­je­cie w kil­ka zespołów nad jed­nym pro­duk­tem pamię­taj, by zawsze dawać znać, o swoich zmi­anach jeśli wpły­wa­ją one na ich pracę (wtedy częstą prak­tyką jest dodawanie jed­nej oso­by z każdego zespołu, nierzad­ko tylko w charak­terze akcep­tacji zmi­an (sprawdzenia czy nie popsują/nie dup­liku­ją się z pracą jej zespołu) niż samej inspekcji jakoś­ci rozwiąza­nia. Jeśli czu­jesz potrze­bę doda­nia kogoś nie­s­tandar­d­owego -np. super­wymi­at­acza w danej tech­nologii, daj mu też znać na komu­nika­torze czy oso­biś­cie, żeby dany PR mu nie uciekł.
  • Ustal­cie kto ‘kil­ka merge’. Zazwyczaj albo ten, kto utworzył PR, albo ten kto uznał, że potrze­bu­je tych zmi­an, zaak­cep­tował i odblokował przycisk(zazwyczaj merge jest możli­wy gdy X osób zaak­cep­tu­je dany PR, gdy PR budu­je się na środowisku itp, moż­na dodać swo­je zasady).

Patrząc na ten wpis moż­na dość do wniosku, że dobry Pull Request i Code Review to sporo pra­cy. I ja się z tym abso­lut­nie zgadzam. Jed­nak myślę, że znacznie więcej w tym pra­cy na postaw­ie, nastaw­ie­niu i zrozu­mie­niu, niż tech­nicznej akrobatyki.W rezulta­cie chce­my prze­cież uzyskać kod, zrozu­mi­ały jak proza, który zad­owala potrze­by użytkown­i­ka. I jak­by nie patrzeć znacznie łatwiej zro­bić to z pomocą całego zespołu niż samodziel­nie. A PR/Code Review jest jed­ną z najlep­szych opcji by uzyskać takie zaan­gażowanie. Aby to osiągnąć warto skupić się na potrze­bie tej drugiej strony, by swo­ją postawą ułatwić jej akty­wny udzi­ał w dostar­cza­niu dobrych rozwiązań.

  •  
  •  
  •  
  •  
  •