Czysty kod – dobre praktyki programowania i code review. Zasady Skauta

Nie każdy w dzieciństwie był harcerzem/skautem. Nie każdy też był w lesie na obozie harcerskim. Większość z was jednak powinna kojarzyć ”boy scout rule” (zasada skautów). Ale czy potrafilibyście wymienić chociaż kilka czynności, które składają się na tę zasadę?

Michał Romańczuk. Programista PHP w Accesto. Od 6 lat pracuje głównie z Symfony, ale projektem legacy nie pogardzi. Pasjonat czystego kodu, uczulony na copy-paste i powtarzanie błędów. Od niedawna próbuje swoich sił w dzieleniu się wiedzą na łamach bloga firmowego.

Kiedy zacząłem szukać informacji na ten temat, czyli co konkretnie muszę zrobić, żeby móc powiedzieć “właśnie zastosowałem boy scout rule”, jedyne co znalazłem to wszechobecne odwołania do cytatu Uncle Boba – „leave your code better than you found it” (zostaw kod lepszy niż zastałeś). Ale Wujku, co ja dokładnie mam zrobić, aby poprawić jakość kodu?

W harcerstwie jest prościej, każdy widzi czy dookoła jest czysto, wie co można posprzątać żeby było czyściej, ale w kodzie? Spróbuję pokazać kilka przykładów z mojej codziennej pracy, w której staram się stosować do tej zasady.

Większość z nas używa jakiegoś IDE (Zintegrowane Środowisko Programistyczne) do pracy, stają się one coraz sprytniejsze i myślą za nas o wielu rzeczach, pilnują jakości i czystości kodu, sugerują lepsze rozwiązania, nawet same sprzątają kod na jedno kliknięcie. Warto jednak mieć oko na to, co dzieje się w kodzie i nie polegać całkowicie na IDE.

Code review – obóz, a nie cały las!

Mam alergię na wielokrotne puste linie w kodzie. Kojarzycie sytuację, w której komuś się za dużo razy kliknął enter lub coś źle wyciął/wkleił. Kiedy robię code review, zwracam uwagę na to jak kod wygląda. Kod musi być czysty i czytelny, a wielokrotne puste linie, według mnie, czytelność psują.

Dlatego czasami podczas code review, gdy taką sytuację zauważę, zostawiam komentarz „jedna pusta linia wystarczy (wszędzie)„. Tutaj „wszędzie” w takim sensie, że nie będę się powtarzał i komentarz dotyczy wszystkich zmian. Pewnego dnia kolega zrozumiał to jednak inaczej niż miałem na myśli.

Merge request, o którym mówię, przed moimi uwagami zawierał 7 zmienionych plików. Możecie sobie wyobrazić moje zdziwienie, kiedy po poprawkach, ta liczba urosła do 350… Kolega „wszędzie” zrozumiał jako cały projekt. Taka zmiana wprowadza ogromny szum w merge requeście, staje się on totalnie nieczytelny (uprzedzając komentarze, wiem, że niektóre narzędzia mają opcję “hide whitespace changes”, ale nie wszystkie i to tylko przykład).

„Morał” z tego taki, że nie można przesadzić ze sprzątaniem, „zostaw obóz czystszy niż go zastałeś„. Widzisz? Obóz, a nie cały las!

Sprzątanie kodu powinno się ograniczać do plików, z którymi pracujemy. Jeżeli dodajecie nową metodę w jakiejś klasie i widzicie, że coś możecie poprawić lub posprzątać w metodzie obok, to to jest to miejsce, gdzie możecie spokojnie zastosować boy scout rule.

Czysty kod – przykłady

deprecated

Jeśli używacie mądrego IDE, wykrywa on przestarzałe metody. IDE, którego ja używam, przekreśla nazwy elementów, które są oznaczone jako deprecated (przestarzałe). Poniżej pokazałem przekreślone adnotacje @Route i @Method. Taki sposób prezentacji jest bardzo zauważalny, trudno go przeoczyć.

Kiedy otworzyłem klasy tych adnotacji znalazłem taki oto kod:

To są zmiany bardzo łatwe do wdrożenia. Autor opisał dokładnie co należy zrobić, wystarczy podmienić importowaną klasę i to właśnie zrobiłem. Kilkusekundowa zmiana i jedna rzecz mniej kłuje w oczy.

To była przestarzała adnotacja, teraz czas na przestarzałą metodę. Poniżej więcej przekreślonych literek. Metoda setData(), tak samo jak przed chwilą, nie da się tego przeoczyć.

Dobrzy harcerze tego nie zignorują. To jest jak plastikowa butelka na ziemi przy obozowym ognisku, to musi być posprzątane. Sprawdźmy więc czy autor nam zostawił jakąś podpowiedź oznaczając tę metodę jako przestarzałą.

Tak jest, wiemy dokładnie co należy zrobić.

I przed nami ostatni przykład. Używamy zewnętrznej biblioteki, w której jedna z klas została oznaczona jako przestarzała.

Po prostu sprawdźmy, tak jak ostatnio.

Kolejna szybka zmiana.

Oczywiście należy zawsze zachowywać czujność i sprawdzać czy autor nie wprowadza nas nieświadomie na minę i czy przypadkiem metoda/klasa, którą podmieniamy nie zmieniła się na tyle istotnie, że coś nam zepsuje.

Jeżeli wasze IDE nie jest tak mądre i nie pokazuje palcem “hej, tutaj jest deprecated”, to poszukiwanie przestarzałych metod nie jest takie proste. Nikt o zdrowych zmysłach nie otwiera każdej używanej któryś już raz klasy czy metody i nie sprawdza czy czasem coś w niej się nie zestarzało od ostatniego razu. Po prostu, używamy tego co znamy, zazwyczaj bez większej refleksji. Dlatego też warto jest korzystać z narzędzi które wspomagają nas w pracy.

nieużywany kod (Remove unused code)

Kod zmienia się bardzo często, szczególnie we wczesnej fazie rozwoju projektu. Razem ze zmianami, czasami zapomnimy usunąć jakiś nieużywany import, zmienną lub metodę.

Mądre IDE wykrywa nieużywany kod, na przykład wyszarzając nazwę nieużywanej metody, zmiennej lub cały nieużywany import co widać poniżej

Ale znów bądźcie ostrożni, ja nigdy nie ufam całkowicie mojemu IDE, zawsze lepiej jest sprawdzić dwukrotnie czy usunięcie fragmentu kodu, nie zepsuje nam gdzieś czegoś. Oczywiście wasze projekty są w pełni pokryte testami i możecie być pewni, że taka zmiana nic nie psuje.

Z publicznymi metodami nie jest tak łatwo, nie wyszarzają się automatycznie. Trzeba do nich podchodzić trochę ostrożniej. Kiedy przestajemy korzystać z publicznej metody, sprawdźmy, czy czasem nie jest gdzieś indziej używana. IDE też może być tutaj pomocne, czasami wystarczy kliknąć na taką metodę i wybrać “znajdź użycia” lub po staremu, skorzystać z opcji “Szukaj”. Jeżeli już się upewnicie, że taka metoda nie jest używana, to po prostu ją usuńcie.

Powinniśmy usuwać nieużywany kod. Po to właśnie korzystamy z systemów kontroli wersji, żeby nie wpaść na pomysł w stylu “a może się jednak kiedyś przyda”. (YAGNI).

nazewnictwo w programowaniu

W naszej pracy co chwilę musimy coś nazwać. Zmienna, metoda, klasa, wszystko musi się jakoś nazywać. Jak powinna wyglądać dobra nazwa? Nie za długa, nie za krótka, zrozumiała dla każdego, kto ją zobaczy. To nie jest takie proste jak się wydaje. Według mnie to jest kluczowy element naszej pracy. Źle nazwana zmienna, może sporo namieszać i dodać pracy tym, którzy będą później próbowali zrozumieć nasz kod. Nazwa musi być zrozumiała bez konieczności zastanowienia. Po przeczytaniu nazwy metody, nazw jej argumentów i tego, co zwraca, powinniśmy dokładnie wiedzieć, co ona robi, bez zaglądania do jej wnętrza.

Kiedy w kodzie widzę zmienne takie jak $a, $var, $arr lub $array, od razu dostaję gorączki.

Poniżej jest przykład dwuliterowej nazwy zmiennej “ac”. Żeby dowiedzieć się czym ona może być, trzeba znaleźć miejsce, w którym coś do niej jest przypisywane, czytelne? O ile dzieje się to w konstruktorze, jak w pokazanym przykładzie, to można to zrobić w miarę szybko, ale nie zawsze tak jest. W momencie tworzenia tej zmiennej dla autora oczywiste było to, że przecież ac to authorizationChecker. Ale nie wszyscy od razu na to wpadną, nawet autor po czasie może nie mieć pewności co to “ac” znaczyło.

Dobrzy harcerze, widząc taką zmienną jak wyżej, szybko zmienią jej nazwę. Skoro już zmarnowaliśmy czas, żeby ustalić co się za nią kryje, sprawmy żeby nikt nie musiał marnować go ponownie:

Taka zmiana wystarczy. Nie oszczędzajcie na literach, odpłaci się wam to w przyszłości, kiedy nie będziecie musieli się zastanawiać, co autor miał na myśli.

czysty kod a formatowanie kodu

Jedną z gorszych rzeczy jakie widziałem w temacie poprawiania czystości kodu, było użycie autoformatowania IDE na całym katalogu projektu, przy okazji innych wprowadzanych zmian w kodzie (“obóz, a nie cały las!”). Wiem, że autor tego MR miał dobre intencje, jednak zabrakło w tym przypadku doświadczenia. Sprzątajmy miejsca, w których pracujemy, nie idźmy jednak z tym zbyt daleko, wystarczy jeden plik lub jedna metoda. Ktoś, kto robi code review, powinien być w stanie skupić się na najważniejszych rzeczach, a nie przewijać setki nieistotnych zmian.

“Moje formatowanie jest najlepsze i najczystsze” – powiedział każdy programista/tka. Każdy ma swoje upodobania i swoje nawyki, ale na co dzień pracujemy w zespołach, dlatego też, musimy się dogadać, co do zasad jakich będziemy przestrzegać przy formatowaniu kodu. Prawdopodobnie w waszej firmie lub w zespole macie jakieś standardy formatowania, jest jednak standard, którego według mnie powinien przestrzegać każdy – PSR-12 (jeżeli przegapiliście to PSR-2 jest deprecated)

Pora na przykłady:

• jednolinijkowe ify

Bardzo nieczytelne

po prostu dodajcie kilka spacji i enterów

lub nawet skrócony zapis

czy nie łatwiej się czyta taki kod?

• wielokrotne puste linie

Nie mam pojęcia dlaczego, ale bardzo popularne

po prostu bądźcie dobrymi harcerzami i usuńcie niepotrzebne linie

• dziwne wcięcia,

też czujecie dyskomfort czytając taki kod? Kiedy widzicie takie wcięcia możecie zrobić coś dobrego i zwyczajnie poprawić wcięcia

• wszystkie inne rzeczy niezgodne z PSR-12

Dobrzy harcerze nie grzebią w “git blame”, tylko sami sprzątają jak coś znajdą, zostawiając kod czystszym, niż ten zastany.

WAŻNE, zasada skauta nie może być stosowana podczas robienia komuś code review, złe rzeczy trzeba punktować, tak żeby czysty kod wszedł w krew. Poprawiając go za innych na tym etapie robimy im krzywdę.

złożone warunki w programowaniu

Ile razy marnowaliście czas na zrozumienie skomplikowanego warunku? Dla mnie, zbyt wiele. Jeszcze gorzej jeżeli macie do czynienia z wieloma warunkami, bardzo łatwo się w tym wszystkim pogubić (jeżeli macie napisane testy to jesteście harcerzami szczęściarzami).

Przygotowałem poniżej przykład, powiedzmy, że jest to metoda, która obsługuje odpowiedź z zewnętrznego serwisu. Najpierw sprawdzane jest czy odpowiedź jest prawidłowa, jeżeli tak, to coś z tą odpowiedzią dzieje się dalej. Skupmy się na tym “jeżeli”.

To nie jest skomplikowany warunek do przeanalizowania, ale chwilę zajmuje żeby sprawdzić co się tutaj dzieje.

Dobrzy harcerze znajdą tutaj wiele rzeczy, które mogliby poprawić po to żeby innym przyjemniej się czytało ten kod. Ale najważniejsze moim zdaniem tutaj będzie wydzielenie wszystkich warunków do osobnej metody, której nazwa jasno odzwierciedla co jest tam sprawdzane.

Oczywiście możecie iść dalej i usprawniać same warunki. Ale to co najważniejsze to to, że poświęciliśmy czas na zrozumienie co tutaj się dzieje, więc niech inni nie muszą marnować czasu na to samo.

powtórzony kod

Dlaczego w ogóle znajdujemy w kodzie powtórzone fragmenty? Bo jeżeli coś działało w jednym miejscu, będzie przecież działało w kolejnym. Poza oczywistym nonsensem kopiowania kodu, niesie to za sobą inny problem. Jeżeli później chcemy coś zmienić, musimy pamiętać o wszystkich miejscach, które są kopiuj-wklejką. Bezmyślne kopiowanie jest nieakceptowalne i nikt nie powinien tego robić. Ale bądźmy szczerzy, to się zdarza, nawet często. Kiedy dobrzy harcerze widzą skopiowane fragmenty kody, zwyczajnie je sprzątają. Mądre IDE zaznaczają duplikaty kodu informując nas o tym, że coś jest nie tak.

Poniżej przykład tego o czym mówię, fragment kopy-pasty

Widzimy dwie metody, nawet jeżeli wytężycie wzrok to nie znajdziecie między nimi różnicy innej niż nazwa metody.

Pierwsza metoda wysyła email z jakąś pozytywną decyzją, druga metoda natomiast wysyła negatywną decyzję. Z jakiegoś powodu, zamiast wydzielić wspólny kod lub zwyczajnie zmienić nazwę metody tak żeby pasowała, autor skopiował całą z inną nazwą.

Po tym przychodzi dzień, kiedy dostajesz zadanie żeby zmienić tytuł maila z decyzją. Teraz masz dwie możliwości. Pierwsza – “na leniwca”, zmieniasz tytuł maila w obu miejscach i po kłopocie. Przecież zadziała, to czemu nie? A właśnie, że NIE! Dobrzy harcerze zrobią coś więcej niż to, posprzątają ten fragment kodu.

Dobre praktyki programowania – co dalej?

Cokolwiek co poprawi czytelność kodu oraz ułatwi jego utrzymanie. Nie robicie tego dla innych, robicie to dla siebie z przyszłości. Wdrożenie każdego pokazanego przeze mnie przykładu, trwało mniej niż minutę. To dobrze zainwestowany czas, zwróci się w przyszłości. Jeśli interesują Cię dobre praktyki programowania, zobacz artykuł o 10 zasadach pair programmingu.

Artykuł został pierwotnie opublikowany na blog.accesto.com. Zdjęcie główne artykułu pochodzi z unsplash.com.

Oferty pracy z