Więcej nie znaczy wolniej cz.6 - code review

Ech... i jeszcze to review kodu

Pora na kolejną "problematyczną" praktykę, a mianowicie na code review. Sama idea jest prosta do zrozumienia i polega ona na tym, że kod (jego forma, wygląd i sposób rozwiązania problemu) musi zostać zaakceptowany przez kogoś poza jego twórcą.
Tak naprawdę nie jest istotne czy używasz do tego celu specjalnie dedykowanych narzędzi, czy rozsyłacie patche ze zmianami, czy stosujesz do tego celu jeszcze inną technikę, ważne jest, aby na każdą linijkę, która chce "dostać się" na produkcję, popatrzył ktoś poza jej autorem. Dzięki temu nieustannie dbamy i poprawiamy jakość kodu, eliminujemy bugi implementacyjne, dodajemy, nie wzięte pod uwagę przez autora, test case'y itp.

Dobra, to skoro podstawowe informacje mamy za sobą, to pora przejść do jakichś konkretów :)

jakość i czytelność

Jakość oraz czytelność kodu to dość szerokie pojęcia i nie chcę nawet podejmować się próby ich zdefiniowania i wytłumaczenie, bo z pewnością musiałbym na to przeznaczyć dużo więcej miejsca, niż na wszystkie swoje wpisy do tej pory. Wszystkim zainteresowanym polecam książkę Roberta C. Martina - Clean Code: A Handbook of Agile Software Craftsmanship, to tak na dobry początek :)

Pisząc kod, który ma być czytelny, warto pamiętać, że częściej będziesz go czytał niż pisał :) Oczywiste? Po przeczytaniu zapewne tak, ale często zapominamy o tym spisując kolejne linijki. I w tym miejscu z pomocą przychodzi nam code review :)

Jeżeli słyszycie pytanie: "Czy mógłbyś mi powiedzieć, o co tutaj chodzi?", to znaczy nie mniej nie więcej, że Wasz kod nie jest czytelny i warto zastanowić się w jaki sposób można to poprawić. W takim wypadku warto siąść z osobą, która zadała pytanie i pracować tak długo aż oboje będziecie zadowoleni z efektów.
Zdaję sobie sprawę, że wielu z Was może powiedzieć, że to przecież przede wszystkim chodzi o nazewnictwo i inne podobne pierdoły i, szczerz mówiąc, tak , właśnie o nie chodzi. Tylko, jeszcze raz powtarzam, że dany kod częściej będziecie czytać, niż pisać i dlatego tak istotne jest to, aby była to czynność prosta, nie wymagająca pomocy i większego wysiłku, powinna być przyjemna.
Wyobraźcie sobie, że w książce, którą czytacie nie zastosowano by znaku nowej linii. Czy przełożyłoby się to na przyjemność czytania tej książki, na tempo tej czynności i na zrozumienie treści? A przecież to tylko jeden głupi znak?

To teraz pora na jakość. Dlaczego poruszam obie kwestie w jednym paragrafie? Ponieważ na tym etapie są ze sobą mocno powiązane. Jakość interfejsów, klas, metod powinna być zweryfikowana po review projektu oraz poprzez testy, które (tak gwoli przypomnienia :) są jednym z narzędzi poprawiających jakość projektu.

Więc jaką jakość mam na myśli? Chodzi mi tutaj o jakość powiązaną z implementacją. Brak literówek, błędów w założeniach, czy metody nie są zbyt duże, czy nie stosujemy zbyt złożonych bloków warunkowych itp. Jak widać taka jakość jest mocno powiązana z czytelnością.
Na przykład zbyt duża metoda charakteryzuje się często również niejasnością i trudnością w "przeczytaniu" jej, trudnością w jej testowaniu. Dla osoby piszącej kod, problemy te mogą nie być oczywiste i widoczne, ale dla kogoś, kto ogląda review, kto nie brał udziału w procesie powstawania kodu, mogą być one istotne.
Co z taką metodą można zrobić? Z pewnością rozbić na mniejsze, powstanie nam wtedy masa metod prywatnych. Więc może iść dalej i przenieść te metody do innej klasy? A co za tym idzie, przenieść sporą ilość testów? Taka delegacja ułatwia zarządzanie kodem i w przyszłości zazwyczaj przynosi wymierne korzyści.

utrzymanie standardów

Nie mam na myśli żadnych konkretnych standardów. Nie chcę nikogo namawiać do jedynego prawidłowego sposobu pisania kodu: ile wcięć, kiedy, jak duże, jak komentować, gdzie i kiedy nawiasy itp. Szczerze mówiąc, forma jest nieistotna dopóki w obrębie zespołu jest spójna i zaakceptowana.

Code review pozwala wyłapać wszystkie "odstępstwa od normy". I o ile problemem nie byłoby, gdyby owo odstępstwo się zdarzyło, to robi się to kłopotliwe, gdy zaczynają się one zdarzać, a niestety jeżeli pozwolimy sobie na "ten jeden jedyny raz", to te "razy" mają dziwną tendencję do zwiększania swojej ilości w dość szybkim tempie.

A jakie są konsekwencje nie trzymania się standardów? Im większa grupa ludzi, tym większe i szybsze do zauważenia. Każda osoba, to indywidualny sposób pisania kodu, a jeżeli jesteśmy zmuszeni przeskakiwać pomiędzy metodami i każda z nich różni się odrobinę od innych, to więcej czasu poświęcamy na znajdowanie interesujących nas fragmentów niż rzeczywistą analizę problemu. Gdy są standardy, dokładnie wiemy gdzie i czego szukać, robimy to po pewnym czasie automatycznie i bez wysiłku.

bugi, ach te bugi

Chodzi mi o drobne bugi, czyli literówki, błędy w założeniach, zła konstrukcja wyrażeń logicznych, podwójny znak równości zamiast potrójnego, brak sprawdzania typu itp. Są to na tyle drobne rzeczy, że podczas pisania możemy zwyczajnie ich nie zauważyć, zbyt jesteśmy pogrążeni procesem twórczym, żeby zawracać sobie głowę tymi istotnymi "bzdetami".
Dlatego dobrze, że ktoś jeszcze rzuci okiem na nasz kod, bo to, co przeoczyła jedna osoba, może zostać zauważone, gdy jest ich więcej.

i jeszcze jeden i jeszcze raz

Z pisaniem testów to jest tak, że nie można (zazwyczaj) wyczerpać wszystkich przypadków i nawet nie jest (nie powinno) to stać się naszym celem. Nie oznacza to jednak, że jeżeli jest jakiś istotny to można go pominąć.
Jednak czasami nie uda nam się pokryć kodu wszystkimi interesującymi case'ami, czy to przez roztargnienie, czy jest to spowodowane faktem, że po prostu o nim nie pomyśleliśmy, to nie istotne. Jeżeli robimy review to jest szansa, że ktoś zwróci na to uwagę. W dodatku jedni mają lepszy "zmysł testera" niż inni i zwyczajnie widzą więcej :)

Dodatkowo code review pomaga w eliminacji zbędnych testów, false postive'ów czy też zwyczajnych błędów w adnotacji, które mogą spowodować, że część z nich w ogóle nie będzie się wykonywać.

wzrasta zrozumienie kodu i wiedza

Im większy zespół, tym więcej kodu powstaje bez Twojego zaangażowania, tym więcej rzeczy Ci "ucieka".
Review pozwala na zmniejszenie tych strat, dzięki niemu jesteście na bieżąco z tym, co się dzieje z kodem. W dodatku, jeżeli zastosujecie odpowiednie oznaczenia, to żadne istotne zmiany nie umkną Waszej uwadze.

Poza tym, mimochodem i bez większego wysiłku, powiększacie swoją wiedzę. Zdaję sobie sprawę, że nie każdy kawałek kodu nauczy Was czegoś nowego, ale stanie się to raz na jakiś czas, czasami będzie to drobnostka, a czasami coś większego. Dzięki review proces wzajemnego przekazywania wiedzy wychodzi "po drodze".

commit przed czy po?

To już zależy od ustaleń w zespole. My przyjęliśmy zasadę, że nie commitujemy kodu, dopóki nie przejdzie przez review, ale nawet od tej "żelaznej zasady" są pewne odstępstwa. Czasami, gdy musimy pokazać działające GUI i mamy już gotowy kod, który jeszcze czeka na zaakceptowanie albo dopiero pojawił się w review, to po prostu wysyłamy kod, a ewentualne błędy naprawiamy później.
Tak naprawdę sama kolejność nie jest istotna, ważne jest natomiast, aby zadanie nie zostało zamknięte zanim review nie zostanie zrobione, a kod zaakceptowany.

lepsze jest wrogiem dobrego

Nie starajcie się zmuszać wszystkich do oglądania Waszego kodu, nie wszyscy muszą go widzieć. Zazwyczaj wystarczy jedna, dwie osoby i można uznać, że review zostało zrobione. Jeżeli będziecie chcieli, aby każde było oglądane przez wszystkich, to odbije się to negatywnie na tempie pracy, a to będzie dowód dla przeciwników tej aktywności, że nie przynosi ona korzyści.
Oczywiście są czasami kody, które powinien zobaczyć każdy, ponieważ to jest coś nowego, innowacyjnego, jakaś krytyczna zmiana. W takim wypadku, warto je w jakiś sposób oznaczyć. I nie, nie każdy kawałek kodu jest na tyle istotny, żeby była potrzeba, aby każdy go oglądnął.

Poza tym programiści wolą tworzyć niż patrzeć na to, co ktoś inny stworzył i nawet jeżeli widzą sens w robieniu review, odczuwają korzyści, to nie należy narzucać im weryfikacji każdej linii kodu. Jeżeli jednak się na to zdecydujecie, to chciałbym Was przestrzec, że w pewnym momencie, przy którymś review z kolei, ich robienie przestaje przynosić korzyści. Każda aktywność wymaga skupienia, trzeba zapoznać się z problemem i rozwiązaniem, przejrzeć testy itp. Przy trzecim, czwartym to już przestaje być efektywne.

Pamiętajcie o tym, że review powinno być małe, 5-10 plików to taka optymalna wartość, a i to zależy od wielkości zmian. Jeżeli Wasze zadanie wymaga większej ich ilości, to podzielcie je na części, jeżeli jest to coś małego (np. zmiana deklaracji metody), ale pociąga szereg modyfikacji w innych plikach, to zróbcie to samo, możecie stworzyć dwa review: jedno z istotą problemu, a drugie z poprawkami wynikającymi z modyfikacji.
Jeżeli nie będziecie trzymać się tych zasad, to zauważycie ten sam problem, co w przypadku przeładowania dużą ilością review - przestaną być one efektywne, bo nim osoba, która je ogląda, dotrze do końca, to jej uwaga i efektywność mimowolnie się zmniejszy. Im więcej plików, tym bardziej odczuwalne będą tego konsekwencje.

Pamiętajcie, że code review to review kodu, nie projektu, ten już wcześniej powinien zostać przemyślany i zaakceptowany, więc nie skupiajcie się ponownie nad tym, czy dane zależności są odpowiednie czy też nie, o tym już myśleliście i dyskutowaliście wcześniej i zdecydowaliście, że to jest najlepsze rozwiązanie. Co innego jeżeli zależności wynikają z implementacji i nie były wzięte pod uwagę przy projekcie np. skomplikowana i złożona metoda zostaje "rozbita" na kilka klas. W takim wypadku, sprawdzenie poprawności powiązań pomiędzy obiektami i zależności, również jest częścią review.

I jeszcze na koniec o defectach. Jeżeli chcecie zgłosić błąd to zastanówcie się kilka razy: czy nie wynika on z moich przyzwyczajeń? Czy kod rzeczywiście nie działa lub działa źle, czy to może jedynie kosmetyka?
Acha, i błąd w stylu "Nie podoba mi się to", "Można zrobić to lepiej" to nie jest błąd, więc po prostu darujcie sobie takie komentarze :)

<< Poprzednie Następne >>