O code review napisano już całkiem sporo. W internecie można znaleźć dokładne opisy jak powinny wyglądać, jakie dają efekty, czy ile kodu sprawdzać na raz. Dlatego nie będę dokładnie analizować tych aspektów. Zamiast tego krótko opiszę najważniejsze korzyści i kilka przydatnych technik na podstawie własnych doświadczeń. Z code review korzystałem już w wielu projektach i zawsze miało to pozytywny wpływ na jakość kodu. Moim zdaniem code review powinno być elementem każdego poważnego projektu.
Co zyskujemy dzięki code review?
Jakie są najważniejsze cele realizowane przez code review? Moim zdaniem można wyróżnić trzy:
- Wykrycie pewnych kategorii błędów trudnych do znalezienia w inny sposób.
- Transfer wiedzy między członkami projektu.
- Wypracowanie jednolitego stylu.
Znajdowanie błędów
Istnieją pewne typy błędów, które ciężko wyłapać za pomocą testów, czy narzędzi do statycznej analizy. Jednak osoba patrząca na nasz kod jest w stanie je wychwycić. Szczególnie kiedy wie na co zwrócić szczególną uwagę. W ten sposób możemy znaleźć szczególnie złośliwe błędy związane ze współbieżnością, wskaźnikami czy alokacją pamięci. Recenzent jest też w stanie sprawdzić zgodność kodu z dokumentacją i wymaganiami. Może ocenić poprawność podjętych decyzji architektonicznych i odnieść się do wiedzy dziedzinowej.
Transfer wiedzy
Dzięki code review nowi członkowie zespołu mogą się szybko wdrożyć do projektu. Recenzując kod innych mogą zadawać pytania dotyczące konkretnych decyzji. A kiedy ich kod jest oceniany, mogą szybko poznać różne kruczki i wiedzieć na co zwracać uwagę przy developmencie. To samo tyczy się juniorów, którzy dzięki code review mają dużo materiału do nauki. Ale z transferu wiedzy korzysta każdy członek zespołu. Dzięki temu dobre praktyki są szybciej propagowane.
Wypracowanie wspólnego stylu
Ważnym aspektem jest również wypracowanie stylu pisania, którego wszyscy się trzymają. Zwykle jest on opisany w dokumencie Coding Standard, ale nie jest w żaden sposób wymuszony. Dzięki code review zespół się synchronizuje co do wspieranych i unikanych praktyk, reguł nazewnictwa zmiennych, funkcji i plików. Dzięki temu łatwiej odnaleźć się w kodzie napisanym przez kogoś innego.
Wiemy już dlaczego powinniśmy robić code review, przejdźmy teraz do wskazówek jak je przeprowadzać.
To co się da niech robią za nas toole
Recenzent powinien całą swoją uwagę skupić na ważnych uwagach, a nie rozpraszać się np. złym umiejscowieniem nawiasów klamrowych. W tym celu przed rozpoczęciem review to co możliwe, powinno być wykryte i najlepiej poprawione automatycznie. Dlatego zanim review się w ogóle rozpocznie dobrze najpierw sprawdzić, czy:
- kod się kompiluje,
- nie ma żadnych warningów,
- przechodzi unit testy,
- ma wystarczający code coverage (nie brakuje testów do nowego kodu),
- statyczna analiza nie znajduje błędów,
- nie ma błędów formatowania,
- pliki mają odpowiednie nagłówki.
W dzisiejszych czasach code review jest wspierane przez toole współpracujące z serwerem continuous integration, który potrafi zrobić dla nas te wszystkie rzeczy.
Autor sam robi wstępne review
Autor przed oddaniem kodu do sprawdzenia najpierw sam czyta jeszcze raz wszystkie wprowadzone zmiany. W ten sposób można wychwycić różne głupie błędy typu nieusunięty kod testowy, proste błędy logiczne i tymczasowe komentarze. Poza tym jest to okazja do naniesienia adnotacji dla sprawdzającego. Możemy w ten sposób wytłumaczyć podjętą decyzję albo wskazać fragment do którego mamy jakieś wątpliwości. Dzięki tym zabiegom możemy oszczędzić trochę czasu.
Wymagana akceptacja dwóch recenzentów
Techniką dającą zadziwiająco dobre efekty jest sprawdzenie kodu przez dwie osoby. Jedna może przejrzeć kod bardzo dokładnie, natomiast druga wystarczy jeśli skupi się na rzeczach bardziej ogólnych i może intereniować, kiedy pierwszy recenzent coś przeoczy. Może również przejrzeć jego uwagi i przedstawić swoje stanowisko. Przydaje się to w sytuacjach spornych.
Aby taki system dobrze działał, recenzentem nie może być autor kodu, ani manager dbający głównie o to, aby feature był jak najszybciej zrobiony. Jeżeli reviewerami są dwaj inni programiści, w ich interesie leży, aby kod spełniał odpowiednie standardy. W końcu sami też będą go wykorzystywać.
Sytuacje sporne
Czasem się zdarza, że autor i recenzent nie mogą dojść do porozumienia. Szczególnie w pierwszych review może się to często zdarzać, kiedy nie ma jeszcze dobrze wykrystalizowanego stylu i dobrych praktyk. Takie problemy powinny być wyjaśnione na forum całego zespołu, a podjęta decyzja będzie wiążąca w kolejnych podobnych przypadkach. Po takich dyskusjach warto na bieżąco uzupełniać coding standard. Tego typu sporne sytuacje to normalna kolej rzeczy i nie są niczym złym. Jeżeli uważasz, że jakaś praktyka powinna być stosowana w projekcie, warto o nią walczyć. W końcu od tego zależy komfort przyszłej pracy.
Review checklist
Skoro istnieją pewne typy błędów, które najlepiej znajdować w code review, warto sobie dodatkowo w tym pomóc. Review checklist jest listą rzeczy na które powinniśmy zwrócić szczególną uwagę analizując czyjś kod. Powinna być dosyć krótka, aby można ją było przeczytać przed każdym review. W ten sposób od razu nastawiamy nasz mózg na szukanie konkretnych problemów. W miarę trwania projektu warto rozszerzać listę wiedząc jakie bugi najczęściej nam umykają.
Uwagi końcowe
Code review jest prostą techniką, która ma niesamowity efekt na jakość kodu. W jednym z projektów nad którym pracowałem została wprowadzona dopiero po dłuższym czasie. Dlatego miałem porównanie jak było przed i po. Byłem zaskoczony jak pozytywnie code review wpłynęło na komfort pracy z kodem i ilu potencjalnych błędów udało się uniknąć.
Ciekawym aspektem jest tutaj świadomość, że nasz kod będzie oceniony przez innych. Przez to bardziej się przykładamy i chcąc oszczędzić czas od razu myślimy do czego można by się przyczepić i to poprawiamy. Nie pomijamy również testów i dokumentacji wiedząc, że bez tego i tak nie przejdzie review.
18 czerwca 2018 at 07:35
Ja dodam od siebie że git commit message również powinien podlegać code review. Niestety nie wszystkie toole to wspierają (np. VSTS) choć powinny.
Inna sprawa to wypracowanie sobie CR-flow, czyli jakie kroki podejmuje każda z osób uczestniczących w CR. Na przykład kto oznacza komentarze jako „zrobione”, co robimy gdy zostaną pchnięte poprawki itp. Ja stosuje taki flow że zawsze robię review całości a komentarze są dla autora (a więc to on je zamyka – w ten sposób można je traktować jako listę TODO), po pchnięciu zmian robię review całości od nowa (tak trochę pracochłonne ale potrafi wyłapać więcej błędów). Dopóki build nie przejdzie reivew nie zaczynam.
Inna ważna sprawa to wielkość commitów, zmiany >400 linii są już praktycznie nie review’owalne. Dlatego ważne jest żeby w zespole panowała kultura robienia małych logicznie spójnych commit’ów. Niestety jest to chyba najczęstszy problem jakiego jako reviewer doświadczam.
Inny problem to „YakShaving”, niestety nawet mi się to zdarza – ktoś wytyka na CR że warto by tu zrobić taki refactor, albo skoro już jesteśmy w tym kawałku kodu to zmieńmy X bo to już czeka od roku na zmianę. O ile takie uwagi są jak najbardziej na miejscu to nie powinny być robione w obrębie review’owanego commita – ale raczej powinno się zrobić na nie oddzielne zadania i dodać do Sprintu/Kanban boarda/czy co tam ludzie mają. Inaczej nasz logicznie spójny commit staje się workiem na faktyczne zadanie i szereg pobocznych nic z nim nie mających refactorów.
Na koniec jeszcze taka rada – wszystkie refactory powinny iść jako osobne commity. Wiem że może być to męczące niestety nie raz już doświadczyłem sytuacji że niewinna poprawka (przy dużym pokryciu >80%) potrafiła wywalić aplikację. W takim wypadku możliwość szybkiego rollback’a i deploy na prod na prawdę pomaga.
18 czerwca 2018 at 17:44
Dzięki za dużo cennych uwag 🙂
Też uważam, że commity powinny podlegać code review. Zarówno same message, jak i ilość zmian przypadająca na commit i to czy każdy niezależny commit się builduje. To jest dodatkowa praca, bo jak się nad czymś pracuje to mamy często dużo niepołączonych logicznie zmian. Ale trochę dodatkowej pracy teraz może się zwrócić wielokrotnie podczas przeglądania git loga.
Po tym jak autor doda nowe zmiany dostosowujące się do uwag, dobrze jeśli reviewer jeszcze potwierdzi, czy o to mu chodziło. Review całości od nowa to też dobra praktyka, niestety nie zawsze starczy czasu i chęci. Za którymś razem już nie jesteśmy tacy uważni.
Z wielkością review główny problem jest taki, że często robimy pull request do konkretnego taska, którego robimy dłuższy czas. Wtedy faktycznie można naklepać tyle kodu, że nie da się tego tak łatwo sprawdzić. Z drugiej strony jak podzielimy na mniejsze części to czasem brakuje fragmentów kodu niezbędnych do zrozumienia intencji autora. Rozwiązaniem może tu być oddawanie do review code in progress, tylko wtedy pushujemy coś co się może zmienić i odbieramy sobie na przykład możliwość rebasów.
Z refactorami też to zależy. Jeżeli jest coś małego to faktycznie można by jako oddzielny commit dodać. Na większe rzeczy warto wystawiać nowe taski. A z małym refactorem w większym review jest taki problem, że może być on potrzebny innym developerom a nie jest zmergeowany dopóki całość nie przejdzie review. Dlatego takie refactory mogą być robione w oddzielnym review, które przechodzi szybciej i jest potem mergeowane z tym wcześniejszym.