Jak robić code review lepiej i krócej?

Code review jest to jedna z tych aktywności, do której niektórych bardzo ciężko przekonać, bo przecież to "kolejna rzecz do zrobienia". pamiętajcie jednak, że więcej nie znaczy wolniej i naprawdę warto spróbować, bo potencjalne korzyści są naprawdę duże.
Dzisiaj jednak nie mam na celu namawiania Was do wypróbowania tej aktywności w Waszym projekcie. Dzisiaj kilka rad dla tych, którzy już code review praktykują oraz dla tych, którzy mają w planach dodanie tej aktywności do swojego harmonogramu :)

co dwie głowy...

Niejednokrotnie spotkałem się z podejściem, że code review był robimy przez jedną, zawsze tą samą osobę, team leader'a, software architecta czy innego senior developera. Oczywiście pobódki ku temu są jak zawsze dobre - spójność oraz lepsza jakość kodu, bo przecież weryfikuje go osoba z teoretycznie największą wiedzą. I chociaż to oczywiście wszystko świetnie brzmi w teorii, to warto zwrócić uwagę na kilka rzeczy zanim na coś takiego się zdecydujemy.

Po pierwsze, review kodu nie należy do najprzyjemniejszych i twórczych czynności i tak naprawdę to po godzinie uważnego przedzierania się przez cudzą twórczość przeciętny programista ma dość, a jego poziom odmóżdżenia przekracza wszelkie skale.

Pierwsze implikuje następny powód - taka lektura, jeżeli jest dawkowana w dużych ilościach, może skutecznie zdemotywować osobę za nią odpowiedzialną i może się skończyć stratą jednego z naszych najcenniejszych ludzi.

Kolejną istotną kwestią jest fakt, że uczymy się całe życie. I choć może brzmieć to jak frazes, to nawet stary wyjadacz może się nauczyć czegoś nowego od "świeżaka". W końcu nie ma osoby, która wie wszystko, a nawet gdyby, to zawsze może umknąć nam coś, co ktoś inny zauważy.
Zmęczenie, przeoczenie, czy zwyczajny niedobór kofeiny nawet najlepszym się zdarza :)

Ponadto, zamykamy najprostszą drogę do nauki innym deweloperom, ponieważ "rzucić okiem na kod" będą mieli możliwość dopiero przy pracy nad nim.
O ile szybciej przyszła praca posuwałaby się do przodu, gdyby wcześniej zostali zaopatrzeni w niezbędną wiedzę?

co za dużo, to nie zdrowo

Z drugiej jednak strony, jeżeli Wasz zespół liczy cztery i więcej osób, to nie ma również sensu, aby na każdy kawałek kodu patrzyli wszyscy. Zdaję sobie sprawę, że mogę usłyszeć teraz "a co z przekazywaniem wiedzy?", niemniej jednak gdy każdy zacznie się wypowiadać i dokładać drobne uwagi, komentarze, to może się okazać, że code review będzie aktywnością, która zajmie więcej czasu całemu zespołowi niż implementacja jednemu programiście :)
Poza tym, nie każdy kawałek kodu jest aż tak istotny, nie wnosi tyle nowości, albo nie jest na tyle skomplikowany aby poświęcać czas wszystkich osób. Ot, zwyczajnie można założyć, że dla większości nic odkrywczego się tutaj nie dzieje, więc szybki rzut oka jednej osoby powinien wystarczyć.

są wyjątki od każdej reguły

Jakiego byście podejścia nie mieli, to osobiście uważam, że są fragmenty kodu, na które każdy powinien spojrzeć. Jeżeli nawet nie w celu skomentowania, czy szukania dziury w całym, to po aby zapoznać się z działaniem. Mam na myśli miejsca gdzie wprowadzamy coś nowego bądź skomplikowanego.
Taki rzut oka w przyszłości zaprocentuje szybszym rozpoczęciem prac nad zadaniami, które dotykają tej części kodu oraz większym zrozumieniem jego zawiłości, co bezpośrednio wpływa na zmniejszenie prawdopodobieństwa dodania bug'a bądź zepsucia czegoś nieumyślnie.

a tak w ogóle to, co trzeba było zrobić?

Dobrze dodać do opisu zadania listę rzeczy do zrobienia (definition od done, todo list, czy jakkolwiek inaczej tego nie nazwiecie). Dzięki takiemu prostemu zabiegowi osoba, która będzie robiła review nie będzie musiała się zastanawiać, co miało zostać zaimplementowane, jaki problem był rozwiązywany oraz jakie były ewentualne dodatkowe ustalenia.

I naprawdę nie polegajcie na swojej pamięci. Przy dużej ilości zadań i review jest fizyczną niemożliwością zapamiętanie wszystkiego. Z drugiej strony, po to przecież mamy aplikacje do zarządzania zadaniami, aby tam umieszczać tego typu informacje i, w razie konieczności, z nich korzystać.

to nie Twój kod

Nie zapominaj również o tym, że każda implementacja się różni i każdy z nas w inny sposób pisze, a każdy problem może zostać rozwiązany na naprawdę wiele sposobów. I wcale nie oznacza to, że będzie to złe rozwiązanie, nie, będzie po prostu inne.
Nie ma sensu w takim wypadku pisać komentarzy informujących autora kodu o tym, że Ty byś to zrobił inaczej, że to byś wyniósł do zmiennej, a tutaj wydzielił trzy zamiast dwóch metod prywatnych (oczywiście zakładając, że kod jest zrozumiały i czytelny). Pamiętajcie o tym, że to nie jest Wasz kod i nie wszystko musi być napisane "po Waszemu". Właściwie to jest zaleta, że tak się nie dzieje, bo dzięki temu mamy szanse się więcej nauczyć, zarówno jeżeli chodzi o samą implementację (może autor wybrał rozwiązanie, którego nie znaliśmy) jak i o czytanie kodu (zaczynasz większą uwagę przywiązywać do ekstrakcji metod i atrybutów oraz ich nazw, itp., ponieważ dostrzegasz jak dużą wartość to ma).

komentarz to... komentarz...

... i zawsze o tym pamiętaj. Co to oznacza?

Jeżeli ktoś dodaje komentarz, to nie musisz rzucać się do zmiany kodu, czy innej podobnego typu reakcji. Pamiętaj, że komentarz (gdy odnosi się do sposobu implementacji) to sugestia, rada na przyszłość. Nie wskazuje on na fragment kodu, który musi zostać podany natychmiastowej obróbce, nawet w przypadku, gdy w pełni się z nim (komentarzem) zgadzamy, a po jego lekturze usłyszeliśmy w głowie głos własnej świadomości, który pyta retorycznie: "dlaczego sam na to nie wpadłem?"
W takich przypadkach często zdarza się, że wysiłek, który zostałby włożony w poprawę kodu jest nie potrzeby. W końcu kod jest akceptowalny. Bo to tylko komentarz był :)

Dobrym pomysłem jest jeszcze umówić się z członkami swojego zespołu, że żaden komentarz nie może zostać zamieniony na defekt, jeżeli nie pojawią się nowe informacje. Tzn. jeżeli ktoś pisze komentarz, w którym sugeruje inne rozwiązanie, nie może nagle, po zauważeniu, że autor nic nie poprawił, zmienić komentarza na błąd.
Na definicję błędu nie może wpływać ruch planet czy też inna bliżej nie określona niewiadoma. Jeżeli coś wczoraj błędem nie było, to (bez pojawienia się niczego nowego) nie może się nim nagle stać.

czasami po prostu nie piszcie

Zdarzało mi się nie raz otrzymać (a że nieomylny nie jestem, to pewnie i napisać :) komentarz, który sprowadzał się do stwierdzenia, że osobie oglądającej kod "COŚ się nie podoba", uważa, że "da się TO zrobić lepiej", a obecny kod "nie może TAKI zostać", "trzeba zmienić".>
Więc, cóż... fajnie... tylko, że tego typu stwierdzenia nic nie wnoszą. Możecie śmiało założyć (a przynajmniej życzę Wam pracy w takich zespołach :), że osoba, która prosi o review kodu starała się zrealizować swoje zadanie najlepiej jak potrafiła i na stwierdzenie "zrób TO lepiej" nie usłyszycie odpowiedzi: "nie ma sprawy, myślałem, że to przejdzie, ale jak nie, to zaraz poprawię" :)

Jeżeli uważacie, że nazwa jest nieodpowiednia, że kod nie jest optymaly, że struktura jest zła, itp., itd., to równocześnie napiszcie propozycję rozwiązania, które Waszym zdaniem jest lepsze. Bez tego lepiej zrobicie jak podarujecie sobie komentarz i nie będziecie tracili swojego, jak i Waszych kolegów, czasu.

i niech ich będzie mało

Przede wszystkim, nim coś napiszecie to przemyślcie to dwa, trzy, pięć i więcej razy. Pamiętajcie, że Wasz komentarz zostanie przeczytany, autor (a może jeszcze ktoś, kto również będzie oglądał kod) się do niego odniesie, co może być początkiem dyskusji, a każda dyskusja pochłania czas.
Dlatego też naprawdę warto przed każdym zdaniem zastanowić się czy rzeczywiście dana uwaga jest niezbędna, czy więcej wniesie (informacji, jakości) niż zabierze (czasu, nerwów).

i jeszcze czytelność!

Powyżej wymieniłem kilka podpowiedzi, przez które przewija się podejście, żeby nie zwracać przesadnej uwagi na każdy element kodu i nie czepiać się każdej pierdoły.
Jest to prawda, niemniej jednak, uczulam Was dodatkowo, żebyście w trakcie tego nie przywiązywania uwagi do detali nie zapomnieli przypadkiem o czytelności kodu. Jeżeli musicie pytać autora o to, co kod robi, jeżeli on sam dodaje stosowny komentarz z opisem, to wiedzcie, że coś się dzieje i może warto zastanowić się nad innymi nazwami, nad ekstrakcją zmiennych, metod, a może nawet klas. Jeżeli kod jest niezrozumiały w chwili, gdy jest jeszcze "ciepły i świeży" to z biegiem czasu nie zyska na czytelności. Dlatego warto poświęcić na jej poprawę trochę czasu teraz, gdy dopiero powstaje i jest przynajmniej jedna osoba, która wie o co chodzi, niż irytować się i poszukiwać odpowiedzi w przyszłości, kiedy znów przyjdzie nam z nim pracować.



A jakie są Wasze doświadczenia i rady dotyczące code review? Jak Wy sobie z nimi radzicie?