Code review: najczęstsze błędy wyłapywane u innych developerów

Joanna

Mindset programisty

Code review to nie tylko nauka dla osoby, której kod jest przeglądany, to również doskonała okazja do obserwacji powtarzających się wzorców błędów w zespole. Po przejrzeniu dziesiątek pull requestów zaczynają wyłaniać się pewne klasyki gatunku, błędy, które pojawiają się niezależnie od poziomu doświadczenia programisty. Niektóre z nich to drobne niedopatrzenia, inne mogą poważnie wpłynąć na wydajność strony/aplikacji lub jej stabilność. Choć wiele problemów wynika z pośpiechu, to część z nich jest efektem braku dobrych nawyków programistycznych. Warto je znać, bo świadomość typowych pułapek pomaga ich unikać we własnym kodzie.

Najczęstsze błędy wyłapywane podczas code review

Oto moja lista najczęściej spotykanych problemów, które regularnie wychodzą podczas review – od oczywistych po te bardziej subtelne, ale równie istotne.

1. Podwójne odpytywanie bazy danych

Klasyk gatunku, który pojawia się zaskakująco często:

if (get_user_data($userId)) {
    $userData = get_user_data($userId);
    // ...
}

Sama złapałam się na tego typu błędach. Problem jest prosty – wykonujemy dokładnie to samo zapytanie do bazy dwa razy. Raz w warunku, drugi raz przy przypisaniu wyniku. Jeśli zapytanie jest złożone lub baza jest obciążona, to podwójne marnowanie zasobów.

Rozwiązanie jest równie proste:

$userData = get_user_data($userId);
if ($userData) {
    // ...
}

Przypisujemy zapytanie do zmiennej, a później tylko sprawdzamy jego wynik. Efekt: szybsze działanie, mniejsze obciążenie bazy. To wydaje się oczywiste, ale w gąszczu logiki biznesowej takie rzeczy łatwo przeoczyć, zwłaszcza gdy refaktoruje się kod i zmienia warunki.

2. Wykonywanie operacji, które powinny być wykonane raz, wewnątrz pętli

Pętle to miejsce, gdzie nieefektywność mnoży się z każdą iteracją. Częsty błąd to umieszczanie w ciele pętli operacji, które tak naprawdę nie muszą się tam znajdować:

for (let i = 0; i < items.length; i++) {
    const container = document.querySelector('.container');
    const apiUrl = getApiUrl();
    // ...
}

W powyższym przykładzie za każdym razem odpytujemy DOM o ten sam element oraz wywołujemy funkcję zwracającą ten sam URL. Jeśli pętla iteruje po tysiącu elementów, wykonaliśmy tysiąc niepotrzebnych operacji.

Właściwe podejście:

const container = document.querySelector('.container');
const apiUrl = getApiUrl();

for (let i = 0; i < items.length; i++) {
    // ...
}

Wszystko, co jest stałe dla całej pętli, wyciągamy przed nią. Prosty gest, który może znacząco poprawić wydajność, szczególnie przy dużych zbiorach danych.

3. Nakładanie listenerów na każdy element z osobna oraz ponowne nakładanie listenerów przy dynamicznym dodawaniu elementów

Dodawanie event listenerów bez delegacji zdarzeń prowadzi do nadmiarowej liczby aktywnych funkcji, które muszą być utrzymywane przez przeglądarkę.

document.querySelectorAll('.button').forEach(button => {
    button.addEventListener('click', handleClick);
});

Problem jest wielowarstwowy. Po pierwsze, jeśli mamy 100 przycisków, tworzymy 100 listenerów, dla każdego elementu z osobna, co obciąża pamięć. Po drugie, po dodaniu nowych elementów na stronę bez jej przeładowania (np. za pomocą ajaxa), musimy ponownie przypisać listener każdemu nowo dodanemu przyciskowi.

Rozwiązanie to event delegation:

document.addEventListener('click', (e) => {
    if (e.target.matches('.button')) {           // or e.target.closest('.button')
        handleClick(e);
    }
})

Jeden listener na poziomie dokumentu (lub lepiej – na poziome kontenera), który obsługuje wszystkie przyciski zarówno te istniejące, jak i te dodane dynamicznie. Mniej pamięci, brak problemów z duplikacją, prostsze zarządzanie.

4. Pozostawianie nieużywanego kodu

Nieużywane funkcje, zmienne, fragmenty logiki warunkowej lub zakomentowane bloki nie tylko zanieczyszczają kod, ale również utrudniają późniejsze analizy i debugging. Inni programiści zaczynają zastanawiać się, czy kod jest potrzebny, co z nim zrobić i jaki miał cel. W konsekwencji projekt traci przejrzystość, a koszt jego utrzymania rośnie.

Każdy element, który nie jest wykorzystywany, powinien zostać usunięty przed wysłaniem kodu do review. Jeśli kiedyś będzie potrzebny, zawsze można wrócić do starej wersji w git history. Kod powinien zawierać tylko to, co faktycznie działa i jest używane.

5. Kopiowanie kodu bez zrozumienia

Przenoszenie fragmentów z innych projektów lub kontekstów bez analizy ich logiki i założeń prowadzi do powstawania błędów trudnych do wykrycia. To, że dany fragment działał wcześniej, nie oznacza, że zadziała w innym środowisku, z innymi zależnościami, w innym modelu danych lub innej architekturze. Brak zrozumienia implementacji to także wyższe koszty późniejszego jej utrzymania. Świadomy developer nie wkleja kodu w ciemno, najpierw analizuje jego działanie, a później decyduje, czy pasuje do projektu.

6. Brak walidacji zmiennych

Ten błąd kończy się zazwyczaj komunikatami o undefined variable, powiadomieniami w logach, a w najgorszym przypadku błędami na produkcji:

// We assume that $data['user'] is defined
$userName = $data['user']['name'];

// We assume that $items is an array
foreach ($items as $item) {
    // ...
}

Problem pojawia się, gdy założenia nie są spełnione. Ktoś wywołał funkcję bez przekazania odpowiednich parametrów, dane z API nie zawierają oczekiwanej struktury, użytkownik nie jest zalogowany. Efekt? Błędy, które mogły zostać obsłużone elegancko, zamiast tego kładą część funkcjonalności.

Poprawnie:

if (isset($data['user']['name'])) {
    $userName = $data['user']['name'];
}

if (is_array($items)) {
    foreach ($items as $item) {
        // ...
    }
}

7. Brak testowania własnego kodu przed wysłaniem do review

Wprowadzanie zmian bez uruchomienia strony/aplikacji, bez przejrzenia konsoli oraz bez sprawdzenia ostrzeżeń lintera jest jednym z najbardziej destrukcyjnych nawyków. Takie podejście przerzuca obowiązek testowania na recenzenta, co spowalnia projekt i obniża standard pracy.

Problem nie polega na tym, że kod ma bugi – to normalne. Problem polega na tym, że osoba, która wysłała kod do review, nie sprawdziła czy w ogóle działa. Nie odświeżyła strony, nie sprawdziła konsoli, nie uruchomiła lintera. Code review to nie test funkcjonalny, to przegląd logiki, architektury, potencjalnych problemów. Zanim wyślesz kod do kogoś innego:

  • Odśwież stronę i sprawdź czy działa
  • Zajrzyj do konsoli i sprawdź, czy nie ma błędów
  • Uruchom lintery i testy
  • Kliknij w rzeczy, które zmieniałeś
  • Sprawdź podstawowe scenariusze użycia

To nie tylko szacunek dla czasu reviewera, to również twoja odpowiedzialność za jakość kodu, który wysyłasz. Jeśli sam nie sprawdziłeś czy coś działa, to tak naprawdę przerzucasz testing na kogoś innego. To nie fair i wydłuża cykl review -> poprawki -> ponowny review.

8. Przypisywanie zmiennej do zmiennej, mimo że można użyć wartości bezpośrednio

Czasem w kodzie pojawiają się konstrukcje, które technicznie działają, ale są zbędnym pośrednikiem:

$filter_type = is_category() ? 'category' : 'all';
$args['current_filter'] = $filter_type;

Zmienna $filter_type jest używana dokładnie raz, do przypisania do $args['current_filter']. Nie ma żadnego powodu, by istniała jako osobny byt.

Prościej i czytelniej:

$args['current_filter'] = is_category() ? 'category' : 'all';

Oczywiście są sytuacje, gdzie taka zmienna ma sens – jeśli jest używana wielokrotnie, jeśli jej nazwa niesie istotną informację semantyczną, jeśli poprawia czytelność złożonego wyrażenia. Ale jeśli to tylko pośrednik między jednym wyrażeniem a drugim, to niepotrzebnie zaśmieca kod i utrudnia jego czytanie.

Każda zmienna w kodzie powinna mieć uzasadnienie. Jeśli jedyne co robi to przekazanie wartości dalej, można ją usunąć.

9. Stosowanie różnych konwencji nazewnictwa w jednym projekcie lub nawet w jednym pliku

Niespójność w nazewnictwie utrudnia czytanie, wyszukiwanie i rozumienie relacji między elementami kodu. Wprowadza również chaos w zespołach, gdzie każdy developer zaczyna stosować swoje przyzwyczajenia. Konsekwentne trzymanie się jednej konwencji (np. camelCase, snake_case, PascalCase) stanowi jedną z podstaw profesjonalnej pracy programistycznej.

10. Nierozważne używanie kosztownych funkcji JavaScript

Jeśli event scroll odpala się przy każdym pikselu przewinięcia strony, to mogą to być setki razy na sekundę. Jeśli w każdym z tych wywołań odpytujemy DOM, robimy skomplikowane obliczenia czy zmieniamy style elementów, przeglądarka zaczyna się dusić, strona tnie, użytkownik się frustruje. Problem pogarsza się, gdy kod robi te same rzeczy w kółko, mimo że nic się nie zmieniło. Na przykład przelicza wysokość okna przy każdym scrollu (choć zmienia się tylko przy resize), albo iteruje po wszystkich nagłówkach i aktualizuje im klasy, nawet gdy żaden nie zmienił stanu.

Rozwiązanie:

  1. Debounce lub throttle
    Debounce i throttle ograniczają liczbę wywołań funkcji w czasie, dzięki czemu przeglądarka nie musi wykonywać kosztownych operacji przy każdym pojedynczym zdarzeniu (np. scroll, resize, input). Debounce opóźnia wykonanie funkcji aż użytkownik przestanie wykonywać akcję, natomiast throttle zapewnia, że funkcja wykona się maksymalnie raz na określony interwał czasowy. Obie techniki znacząco odciążają główny wątek i poprawiają płynność interfejsu.
  2. Intersection Observer
    Intersection Observer pozwala reagować tylko wtedy, gdy element faktycznie pojawia się w obszarze widocznego viewportu. Dzięki temu nie ma potrzeby ręcznie śledzić scrolla i analizować pozycji każdego elementu na stronie. To nowocześniejsze i znacznie bardziej wydajne podejście do leniwego ładowania, animacji czy aktywowania sekcji.
  3. RequestAnimationFrame
    Zamiast wywoływać funkcje w odpowiedzi na szybkie, powtarzalne zdarzenia, można skorzystać z requestAnimationFrame, aby wykonywać operacje zsynchronizowane z cyklem odświeżania przeglądarki. Zapobiega to przeciążeniu CPU i sprawia, że animacje lub obliczenia są płynniejsze i bardziej kontrolowane.

Większość błędów, które wychodzą podczas code review, to efekt pośpiechu, zmęczenia, niedopatrzenia albo prób pójścia na skróty. Wystarczy wypracować kilka prostych przyzwyczajeń: sprawdzaj swój kod zanim go wyślesz, pilnuj spójności w projekcie, nie duplikuj tego samego kodu w kilku miejscach, używaj funkcji ze zrozumieniem ich kosztu. Te małe zmiany w podejściu robią realną różnicę, kod jest czytelniejszy, reviewerzy mają mniej uwag, a projekt posuwa się naprzód sprawniej.