Dobre i złe Code Review — przykłady
Zobacz na konkretnych przykładach w kodzie, jak wygląda dobrze zrobione Code Review, a jak źle.
W ostatnim artykule z naszego cyklu Tech Stories sporo uwagi poświęciliśmy dobrym praktykom Code Review [LINK] — temu, na co warto zwrócić uwagę podczas inspekcji kodu, jakich narzędzi użyć i jakie procedury w zespole wdrożyć, aby rzeczywiście i autor kodu, i programista go sprawdzający, doskonale zrozumieli swoje role i potrafili się porozumieć. W dzisiejszym wpisie zajmiemy się zatem już konkretnymi case studies przeglądu kodu, zaczynając od tych złych, bo przecież najlepiej jest uczyć się na błędach.
Złe Code Review — przykłady
Istnieje mit, że złe Code Review to takie, w którym ktoś nakłania nas do zmiany kodu pomimo tego, że to, co napisaliśmy jest poprawne. Co gorsze, proponowane przez tę osobę zmiany spowodują błędy. Takie sytuacje przydarzają się jednak niezwykle rzadko — są efektem pomyłki lub nieznajomości zagadnienia i najczęściej przydarzają się nowicjuszom, którym stosunkowo łatwo jest wytłumaczyć, dlaczego ich komentarz był mocno nietrafiony.
W praktyce, największym ryzykiem obarczone jest Code Review zrobione niedokładnie z powodu pośpiechu lub w momencie gdy jesteśmy już zmęczeni. To wtedy nie zauważamy błędów, które w innych przypadkach od razu byśmy wyłapali.
Niestety, najczęściej przegapianymi błędami w kodzie nie są wcale te powodujące małą szkodę w całym projekcie, które łatwo by można naprawić, a:
- błędy związane z łamaniem ogólnie przyjętej architektury,
- błędy polegające na niepoprawnym obsłużeniu wszystkich przypadków brzegowych
- błędy w logice naszej aplikacji (niezgodne zachowanie się aplikacji).
Błąd braku konsekwencji/systematyczności
Jednym z problemów jakie się często spotyka jest brak konsekwencji u osoby sprawdzającej kod. Jako przykład weźmy kod poniżej:
Programista odpowiedzialny za Code Review trafnie zauważył, że w metodzie doSomething autor kodu nie zadeklarował poprawnie typów dla parametru wejściowego oraz zwracanej wartości. Nie zauważył jednak tego samego w przypadku metody internalMethod.
Takie błędy wynikają z braku systematyczności podczas sprawdzania kodu i można sobie z nimi łatwo poradzić, dodając do swojej checklisty Code Review pozycję:
Czy wszystkie metody i funkcje mają zadeklarowane parametry wejściowe i zwracaną wartość?
Brak konsekwencji u osób przeprowadzających Code Review przejawia się w wielu postaciach — może dotyczyć sprawdzania standardów kodowania, poprawnej deklaracji typów czy weryfikacji nazw zmiennych i metod.
Na pierwszy rzut oka, nie są to bardzo duże i uporczywe błędy, jednak ich systematyczne gromadzenie się może mieć poważne skutki w przyszłości.
Błąd nierozwiązanych konfliktów
Podczas Code Review nierzadko dochodzi do sporów między osobą sprawdzającą kod, a autorem danego fragmentu. Następuje wymiana zdań, ale nic z niej nie wynika. Spójrzmy na poniższy fragment kodu:
Doszło tutaj do następującej wymiany zdań:
- Sprawdzający: zgłasza problem z nazwą metody attempt, sugerując użycie gettera czyli getAttempt(),
- Autor: nie zgadza się z tą sugestia stwierdzając, że getter jest używany, kiedy proces faktycznie zaciąga dane z innego źródła, a nie wprost zwraca dane z obiektu,
- Sprawdzający: wstawia linki do stron, na których prezentowane są przykłady używania w takim przypadku metod getAttempt(),
- Autor: odpowiada linkami do źródeł potwierdzających jego podejście.
Po wymienieniu kilkunastu komentarzy sytuacja nie jest w żaden sposób wyjaśniona. Nie wiadomo, które podejście jest bardziej słuszne. Napięcie między osobami zaczyna robić się niebezpiecznie wyczuwalne.
Do takich sytuacji dochodzi wtedy, gdy w naszym procesie Code Review nie uwzględniliśmy strategii rozwiązywania sytuacji patowych. Aby zespół mógł działać sprawnie w przypadku rozbieżnych poglądów, powinniśmy taki proces, umożliwiający podjęcie ostatecznej decyzji mieć wcześniej opracowany. Może to być zasięgnięcie opinii u innej osoby czy też, w skrajnych przypadkach, zebranie większej części zespołu w celu wspólnego ustalenia, jak powinien wyglądać nasz kod.
Dobry przykład rozwiązania tego samego konfliktu: wspólne ustalanie standardów
Powyższa sytuacja mogłaby zostać rozwiązana w sposób ugodowy, np.:
- Sprawdzający: zgłasza problem z nazwą metody attempt, sugerując użycie gettera czyli getAttempt(),
- Autor: nie zgadza się z tą sugestia stwierdzając, że getter jest używany kiedy proces faktycznie zaciąga dane z innego źródła, a nie wprost zwraca dane z obiektu,
- Sprawdzający: przychyla się do sposobu autora kodu, jednak wskazuje, by w takim razie lepiej nazywać takie metody, tak aby wskazywały faktyczną funkcjonalność. W ten sposób, ostatecznie nazwa metody zostaje zmieniona na numberOfAttepmts().
Dobre Code Review — przykłady
Szerzenie wiedzy
Code Review stwarza doskonałe warunki do wymiany wiedzy o języku programowania i pewnych technicznych zależnościach, których nie można zapisać w kodzie wprost, a wynikają one z doświadczenia. Spójrzmy na poniższy przykład przeglądu kodu:
Dla tak zdefiniowanej klasy, osoba przeprowadzająca Code Review zaczęła się zastanawiać nad sensownością użycia w tym miejscu funkcji sleep(). To, co wzbudziło jej wątpliwości, to wpływ tej funkcji na wydajność rozwiązania.
Sprawdzający zgłosił swoją wątpliwość, a autor kodu wyjaśnił, iż był to przemyślany zabieg, który wynika z konieczności utrzymania małej liczby wywołań API, które mają miejsce w dalszej części kodu i które mogłyby doprowadzić do niestabilności rozwiązania.
Wykluczanie potencjalnych błędów
Jedną z największych zalet Code Review jest wyłapywanie potencjalnych błędów już na bardzo wczesnym etapie rozwoju produktu. Jest to dużo mniej kosztowne, niż gdy błąd znajdujemy dopiero na serwerze produkcyjnym. Weźmy jako przykład następujący kod:
Pierwszą rzeczą, jaką możemy tu zaobserwować i już zgłosić w Code Review, jest brak deklaracji typów parametrów wejściowych i zwracanej wartości metody doSomeFormatting.
Po zgłoszeniu tego do autora, ten może nas poinformować, że deklaracja metody powinna wyglądać tak:
Jeśli jesteśmy wnikliwym programistą, to z miejsca zauważymy, że taka deklaracja typów powoduje, iż nasz kod zwróci błędny rezultat w linii 13.
Analizując kod dalej, możemy stwierdzić, że większość ciała tej metody jest zbędna i może spowodować niepotrzebne błędy.
Całość, po poprawnie przeprowadzonym Code Review, może wyglądać tak:
Tak napisana metoda jest dużo prostsza i nie niesie za sobą ryzyka wystąpienia błędu związanego z wywołaniem jej z niepoprawnym typem parametru wyjściowego czy błędami z obsługą zwracanej wartości.
Podsumowanie
Do tego, że warto jest robić Code Review, nie trzeba chyba przekonywać żadnego programisty. Warto jednak pamiętać, że tak samo jak programowanie jest umiejętnością, którą stale rozwijamy, tak i dobre prowadzenie Code Review również wymaga od nas ciągłego doskonalenia.
Gdy przegląd kodu prowadzimy w sposób poprawny, to podnosimy nie tylko jakość naszego kodu, ale także wiedzę i umiejętności programistów.
O autorze artykułu:
Mateusz Antkowiak | Programista PHP w Transparent Data
Zawodowy programista od prawie dekady. Swoje podróże z kodem rozpoczynał od takich egzotycznych kierunków jak Lazarus Pascal czy Delphi, a obecnie, ciągnie go w stabilne wody programowania obiektowego. Posiada ogromne doświadczenie w automatyzacji procesów, co przydaje się w jego obecnych RegTech taskach w Transparent Data |Prześwietl.pl.
Mogą Cię zainteresować również następujące artykuły:
