Siedem ostrzegawczych znaków w recenzjach, które wskazują na kosztowne zmiany w przyszłości

Najbardziej problematyczny kod nie wygląda na uszkodzony. Kompiluje się, przechodzi testy i zachowuje się dobrze w demonstracji, jednak doświadczeni recenzenci często potrafią już po minucie stwierdzić, że kolejna zmiana, kolejny incydent lub kolejne przekazanie kodu spowoduje problemy. Ten przewodnik omawia siedem sygnałów, na które reagują recenzenci, wyjaśnia, co przewiduje każdy z nich, oraz pokazuje, jak odróżnić prawdziwy problem od wzorca, który jedynie wydaje się podejrzany, abyś mógł zastosować tę samą ocenę do swoich własnych recenzji.

Co tak naprawdę mówi o kodzie „zapach”

Doświadczony recenzent rzadko musi czytać cały moduł, aby wyczuć problemy. Argument logiczny, którego przeznaczenie jest niewidoczne w miejscu wywołania funkcji, narzędzie, które nabrało tuzina opcjonalnych zachowań, blok catch, który zamienia każdą awarię w ten sam, pozornie niewinny wartość – nic z tego dziś nie powoduje awarii. Problemem jest to, czego będą wymagać jutro.

To instynkt wynikający z doświadczenia, a nie z bardziej rygorystycznego przewodnika stylu. Ludzie, którzy od lat utrzymują systemy, widzieli, jak jedna skopiowana zasada przekształca się w trzy sprzeczne wersje, jak przydatna abstrakcja staje się miejscem rozwiązywania każdego szczególnego przypadku, a domyślna opcja awaryjna maskuje przerwę w działaniu systemu przez tygodnie. Nauczyli się rozpoznawać momenty, gdy kod gromadzi niepewność.

Pomaga traktować zapach jako wskazówkę, a nie wyrok. Nie dowodzi to błędu w implementacji; jedynie wskazuje, że kod wymaga dokładniejszej analizy:

• Która osoba lub moduł jest odpowiedzialna za tę decyzję?
• Jak to będzie wyglądać, gdy wymagania się zmienią?
• Czy awaria nadal będzie widoczna, gdy się pojawi?
• Czy następny programista będzie mógł zrozumieć, dlaczego kod jest tak skonstruowany, bez konieczności ustalania, kto go napisał?

Poniższe siedem sygnałów to te, które najskuteczniej przekształcają się w koszty, jeśli nikt nie zadaje tych pytań wcześnie.

1. Jedna funkcja wykonująca kilka niepowiązanych zadań

Długie funkcje przyciągają krytykę, ale ich długość rzadko jest tym, co martwi doświadczonego recenzenta. Funkcja może liczyć wiele wierszy, ponieważ wykonywa jedną jasno określoną operację w sekwencji czytelnych kroków. Inna może zmieścić się w dwudziestu wierszach, a mimo to być trudna do obsługi, ponieważ jednocześnie zajmuje się walidacją danych wejściowych, sprawdzaniem uprawnień, zapisywaniem do bazy danych, kształtowaniem odpowiedzi, uruchamianiem powiadomień i tłumaczeniem błędów.

Prawdziwym problemem jest to, że mieszane obowiązki dają funkcji wiele niezależnych powodów do zmian. Nowa zasada dostępu ją dotyka, zmiana formatu odpowiedzi ją dotyka, migracja schematu ją dotyka, nowe wymagania dotyczące powiadomień ją dotykają. Kwestie, które powinny rozwijać się oddzielnie, ostatecznie łączą się w jednym miejscu, a nawet niewielka edycja zmusza do zrozumienia całego procesu.

Cennym wskaźnikiem jest nazwa, która nie potrafi uczciwie opisać funkcji. Coś o nazwie processOrder może sprawdzać klienta, obliczać ceny, zarządzać zapasami, tworzyć zamiar płatności, przechowywać zamówienie i wysyłać potwierdzenie e-mailem. Nazwa ta pozostaje niejasna, ponieważ dokładna wersja wymagałaby całego akapitu.

Dlaczego mechaniczne wydobywanie nie jest rozwiązaniem

Dzielenie funkcji na mniejsze części nie przynosi automatycznie pomocy. Wyodrębnianie fragmentów na podstawie liczby wierszy może rozprzestrzenić tę samą niejasność na więcej plików, przy czym wszystkie ukryte zależności pozostają nienaruszone. Lepszym podejściem jest szukanie granic, które mają konkretne znaczenie:

• Gdzie podejmowana jest decyzja biznesowa?
• Która część stanowi efekt uboczny wobec zewnętrznego systemu?
• Która część to czysta transformacja danych?
• Która część odpowiada za transakcję?

Gdy te szwy staną się widoczne, kod można zorganizować w sposób odpowiadający rzeczywistej procedurze działania. Podstawowym problemem nie była ilość pracy wykonywanej przez funkcję, lecz to, że nikt nie mógł zmienić pojedynczej decyzji bez jednoczesnego uwzględniania kilku niepowiązanych ze sobą decyzji.

2. Parametry logiczne, które włączają ukryte tryby

Argument logiczny wydaje się nieszkodliwy, ponieważ zawiera jeden bit. W praktyce często ukrywa cały alternatywny proces pracy pod istniejącą nazwą funkcji. Rozważmy wywołanie takie jak poniżej, które prawie nic nie mówi czytelnikowi o tym, co się wydarzy:

await updateAccount(accountId, true, false);

Aby to zrozumieć, trzeba otworzyć implementację i sprawdzić, czy pierwszy true zmusza do aktualizacji, uruchamia powiadomienie, omija weryfikację, przechowuje historię czy aktywuje konto. Osoba, która napisała tę funkcję, może o tym pamiętać, ale wszyscy inni muszą odtworzyć jej znaczenie na podstawie pozycji argumentów, a zamieniona para liter zawsze przejdzie każdą weryfikację typu.

Kombinacje, których nikt nie zaprojektował

Problem się pogłębia w miarę gromadzenia się flag. Trzy wartości logiczne dają osiem możliwych kombinacji, z których wiele nigdy nie zostało wziętych pod uwagę. Jeśli funkcja przyjmuje parametry forceUpdate, skipValidation i sendNotification, co powinno się stać, gdy przymusowa aktualizacja pomija weryfikację, ale mimo to wysyła powiadomienie? System typów chętnie dopuszcza taką próbę wywołania, mimo że w kontekście biznesowym nie powinno to mieć miejsca.

Recenzenci zaznaczają to, ponieważ zestaw flag zwykle oznacza, że jedna funkcja realizuje kilka operacji. Mogą one dzielić wspólne elementy wewnętrzne, ale nie mają tego samego celu. Edycja danych kontaktowych, weryfikacja konta oraz zastosowanie uprawnień administracyjnych mogą wszystkie zapisać dane do tej samej wiersza, jednak żaden użytkownik nie powinien musieć pamiętać, który wzorzec flag wybiera dany proces pracy.

Wybór odpowiedniego refaktoryzowania

W niektórych przypadkach wystarczy przejść na obiekt z jednym argumentem, którego właściwości mają nazwy, ponieważ updateAccount(id, { notify: true }) przynajmniej sama się wyjaśnia w miejscu wywołania. W innych przypadkach oddzielne funkcje, takie jak verifyAccount i overrideAccount, sprawiają, że intencja biznesowa staje się jasna, a niemożliwe kombinacje są całkowicie eliminowane. To, która ścieżka jest odpowiednia, zależy od tego, czy te zachowania stanowią rzeczywiste warianty jednej operacji, czy odrębne operacje, które przypadkowo dzielą kod.

Liczby logiczne same w sobie nie są złe. Opcja z prostą nazwą, tak jak includeArchived, jest w pełni uzasadniona. Sygnałem ostrzegawczym jest flaga, która zmienia znaczenie funkcji, zamiast modyfikować dobrze zrozumiany detal.

3. Ta sama zasada biznesowa wyrażona w różnych formach

Tekst skopiowany i wklejony łatwo jest zauważyć. Powtarzająca się wiedza jest bardziej niebezpieczna, ponieważ każda kopia może wyglądać zupełnie inaczej.

Weźmy regułę uprawnień. Może ona pojawić się jako ukryty przycisk w interfejsie użytkownika, sprawdzenie roli w kontrolerze, warunek stanu w usłudze lub klauzulę WHERE w zapytaniu. Każda warstwa wydaje się chronić system, ale każda z nich może zawierać nieco inne zasady dotyczące tego, kto ma prawo działać.

Wszystko funkcjonuje poprawnie aż do zmiany polityki. Jeden punkt końcowy przyjmuje nową regułę, inny nadal stosuje stary algorytm, a zadanie zaplanowane omija oba, kierując się bezpośrednio do wewnętrznej usługi. Interfejs teraz oferuje działanie, którego serwer odrzuca. Jeden element aplikacji traktuje użytkownika jako upoważnionego ze względu na jego rolę, podczas gdy inny wymaga, aby posiadał on odpowiedni rekord.

Zdolni do oceny eksperci zauważają powtarzające się kombinacje stanów, ról, flag i dat, ponieważ takie warunki często reprezentują to samo znaczenie biznesowe opisane więcej niż raz. Zadają pytanie, gdzie znajduje się ostateczna decyzja oraz czy pozostałe warstwy ją egzekwują, czy też po cichu ją przedefiniowują.

Warstwy o różnych celach są w porządku

To nie oznacza, że reguła może występować tylko w jednej linii kodu w całym systemie. Interfejs użytkownika może odzwierciedlać uprawnienia w celu poprawy użyteczności, podczas gdy warstwa backend musi je egzekwować ze względów bezpieczeństwa. Ograniczenie bazy danych może chronić tę samą niezmienniczość na poziomie przechowywania. Te warstwy pełnią różne funkcje. Problem pojawia się, gdy każda z nich odpowiada na pytania biznesowe na własnych warunkach.

Zdrowy projekt przypisuje regułie jednego, wyraźnie określonego właściciela i pozwala pozostałym komponentom korzystać z jej rezultatów, je egzekwować lub wyświetlać. Gdy prawa własności nie są jasne, każda przyszła zmiana polityki zamienia się w zadanie poszukiwawcze: trzeba odnaleźć wszystkie miejsca, w których reguła jest ukryta, i mieć pewność, że niczego nie przeoczyło się. Każdy, kto doświadczył zmian w politykach, wie, że powtarzanie wiedzy zwykle kosztuje więcej niż powtarzanie kodu.

4. Wspólny pomocnik z rosnącym obiektem opcji

Wiele problematycznych abstrakcji zaczyna swoją drogę jako sensowne pomocniki. Gdy w dwóch miejscach pojawia się podobna logika, wyodrębnia się wspólną część, co sprawia, że rozwiązanie staje się małe, skoncentrowane i łatwe do zrozumienia.

Następnie obaj użytkownicy zaczynają żądać różnych rzeczy. Jeden potrzebuje ponownych prób; drugi nie może nigdy powtarzać swojej prośby. Jeden czerpie korzyści z buforowania; drugi nie może widzieć przestarzałych wartości. Jeden zwraca kompletny pakiet danych; drugi przesyła je strumieniowo. Zamiast dzielić te procesy, narzędzie dodaje nową opcję dla każdej różnicy.

Niedługo później obiekt opcji obejmuje tłumaczenie błędów, kształtowanie wyników, ustawienia buforowania, liczbę prób ponownych, obsługę autoryzacji, funkcje hook oraz kilka przełączników logicznych. Większość użytkowników korzysta z dwóch lub trzech z nich, ale każda zmiana niesie ryzyko wpłynięcia na wszystkie.

To jest problem spowodowany tym, że abstrakcja przestała usuwać złożoność i zaczęła ją gromadzić. Metody wywołujące wydają się proste, ale zrozumienie systemu oznacza teraz wiedzę o tym, jak każda opcja oddziałuje na wszystkie pozostałe. Duża liczba wywołań nie jest dowodem na poprawność abstrakcji; czasami wskazuje jedynie na to, że użycie narzędzia pomocniczego stało się nieuniknione.

Syntakta podobna versus zachowanie wspólne

Kluczowe pytanie brzmi, czy metody wywołujące mają stabilne zachowanie, czy tylko kod o podobnym wyglądzie. Dwie operacje mogą wysyłać żądania HTTP, ale różnie radzić sobie z błędami i próbami ponownego wysłania. Para form może mieć te same pola, ale różną weryfikację, przetwarzanie danych po wysłaniu oraz obsługę wersji roboczych. Ich połączenie eliminuje widoczną duplikację, ale utrudnia wyrażenie każdej rzeczywistej różnicy.

Zazwyczaj lepszym rozwiązaniem jest wprowadzenie w późniejszym etapie węższej abstrakcji. Zamiast dzielić się całym procesem pracy, należy udostępnić pojedynczy, stabilny element, takiego jak jedna transformacja, jedna zasada lub jeden wywołanie niskopoziomowe, i jasno określić odrębne procesy pracy. Ponowne wykorzystanie pozostaje cenne; powodem do zaniepokojenia jest sytuacja, w której każde nowe wymaganie dodaje kolejną opcję, której istniejący użytkownicy ani nie potrzebują, ani nie rozumieją. Aby zapoznać się z perspektywą skupioną na interfejsie użytkownika tego samego problemu, zobacz dlaczego abstrakcje frontendu po cichu przekształcają się w dług techniczny.

5. Obsługa błędów, która maskuje niepowodzenie jako sukces

Blok catch może sprawić, że aplikacja będzie wyglądać na solidną, jednocześnie po cichu niszcząc zaufanie do niej.

Załóżmy warstwę dostępu do danych, która w przypadku błędu w bazie danych zwraca []. Albo usługę, której żądanie HTTP zawodzi i która po prostu zwraca null. Frontend mapuje każde odrzucone żądanie na stan pusty. Nic się nie zawala, ale system nie jest już w stanie odróżnić prawdziwej braki danych od awarii.

Przeglądający szybko to wykrywają, ponieważ wiedzą, jak to wygląda podczas incydentu. Panel sterowania pokazuje zero rekordów, gdy baza danych jest niedostępna. Klient widzi „Brak zamówień” po tym, jak jego sesja bezgłośnie wygasła. Zadanie w tle oznacza element jako zakończone, ponieważ zależność nie zwróciła nic użytecznego. Interfejs pozostaje spokojny, ponieważ zastąpił prawdę wiarygodną odpowiedzią.

Pytania do zadawania o każdym bloku obsługi błędów

Przy przeglądaniu mechanizmów obsługi błędów sprawdź trzy rzeczy:

• Czy oryginalna przyczyna jest zachowana, na przykład jako cause nowego błędu, lub przynajmniej w dziennikach?
• Czy osoba wywołująca funkcję może odróżnić oczekiwany wynik biznesowy od awarii technicznej?
• Czy warstwa, która łapie błąd, faktycznie odpowiada za kolejną decyzję?

Gdy kod na tym poziomie nie ma możliwości odzyskania sytuacji, ponownego spróbowania, cofnięcia częściowej pracy lub dostarczenia sensownej alternatywy, łapanie błędu może nic nie dać poza zniszczeniem dowodów.

Żadne z tego nie oznacza, że surowe wyjątki powinny trafiać do użytkowników. Komunikaty pokazywane ludziom powinny być bezpieczne i zrozumiałe, a błędy wewnętrzne często wymagają przekształcenia w stabilne kategorie domeny. Wymaganie polega na tym, aby tłumaczenie zachowało wystarczająco dużo znaczenia, aby zarówno aplikacja, jak i deweloperzy mogli prawidłowo zareagować.

Pusty wynik powinien oznaczać, że operacja zakończyła się pomyślnie i nic nie zostało znalezione. Nieudana operacja powinna pozostać nieudana, chyba że system celowo zdefiniował bezpieczne rozwiązanie awaryjne. Łączenie tych dwóch przypadków dla wygody spowalnia debugowanie i pokazuje użytkownikom mylące informacje. Dobre radzenie sobie z błędami uwzględnia niepowodzenie, natomiast złe je ukrywa.

6. Małe wymaganie z długim, niespowiązanym zestawieniem różnic

Doświadczeni recenzenci często przyglądają się strukturze prośby o połączenie kodu, zanim przeczytają poszczególne linie. Gdy niewielkie wymaganie rozciąga się na długą listę niepowiązanych plików, zazwyczaj wskazuje to na ukryte powiązania lub na zadanie, które odbiegło od swojego celu.

Dodanie pojedynczego pola może uzasadnionie wpłynąć na trwałość danych, usługi, odpowiedzi API oraz interfejs użytkownika; jest to spójna zmiana obejmująca różne warstwy oprogramowania. Problem pojawia się, gdy ten sam pull request jednocześnie przepisuje wspólne funkcje pomocnicze, zmienia nazwy niepowiązanych typów, modyfikuje mechanizmy obsługi błędów, przesuwa moduły i aktualizuje duże pliki z kopiami stanu.

Każda z tych modyfikacji może być ulepszeniem, ale łącznie tworzą one jedną całość składającą się z kilku powodów zmiany. Recenzenci muszą jednocześnie ocenić nową funkcjonalność, refaktoryzację, zmiany w kontrakcie oraz czynności porządkowe. Jeśli przejdzie niezauważona regresja, trudniej jest zlokalizować odpowiednią modyfikację. Jeśli funkcjonalność musi zostać cofnięta, towarzyszą jej również niepowiązane ulepszenia.

Dlaczego kodowanie wspomagane przez AI sprawia, że jest to coraz częstsze

Narzędzia do generowania kodu utrudniają wpadnięcie w ten schemat, ponieważ pełne przepisywanie nie kosztuje prawie nic. Poproś o poprawkę jednego błędu, a możesz otrzymać udoskonaloną funkcję z nowymi nazwami, innym przepływem sterowania, świeżymi abstrakcjami oraz zmienionym zachowaniem w przypadku błędów. Jej stworzenie kosztowało niemal nic, natomiast weryfikacja nadal wymaga od zespołu tych samych wysiłków w postaci przeglądania co zawsze.

Dobrą zasadą jest utrzymywanie rozmiaru zmiany w proporcji do tego, na jak jasny sposób można ją wyjaśnić. Wąska diagnoza zazwyczaj wymaga wąskiego rozwiązania, natomiast szersze korekty mogą zostać umieszczone w osobnym pull requestu, gdzie można je przeanalizować i przetestować pod kątem ich własnych zalet. Nie chodzi tu o wymaganie tworzenia małych pull requestów we wszystkich przypadkach; niektóre prace architektoniczne są z konieczności bardziej zakrojone. Pytanie brzmi, czy każda istotna edycja prowadzi do jednego spójnego rezultatu. Gdy różnica między stanem przed i po edycji jest większa niż sam problem, zazwyczaj rozpatruje się kilka kwestii jednocześnie, bez ich oddzielenia, co utrudnia wyznaczenie przyczyn i skutków.

7. Kod, który działa, ale opiera się weryfikacji

Część implementacji powstaje szybko właśnie dlatego, że ich zachowanie jest trudne do zaobserwowania lub kontrolowania. Funkcja odczytuje bezpośrednio zegar systemowy, tworzy własne zależności, modyfikuje stan globalny, wykonywa kilka połączeń z zewnętrznymi systemami, tłumi własne błędy i prawie nic nie zwraca. Przypadek udanego działania może być w porządku, ale pokazanie, jak funkcja zawodzi, staje się kosztowne.

Zdolność do testowania odzwierciedla zazwyczaj jakość zdefiniowanych granic. Jeśli reguła biznesowa nie może zostać sprawdzona bez uruchomienia całego aplikacji, wypełnienia rekordów w bazie danych, wywołania rzeczywistych usług i oczekiwania na rzeczywiste zegary czasowe, reguła ta prawdopodobnie jest powiązana z zbyt wieloma efektami ubocznymi.

Pracy asynchroniczne ujawniają tę lukę

Problem jest najbardziej dotkliwy w wieloetapowych, asynchronicznych procesach. Wyobraźmy sobie funkcję, która zapisuje zmianę, wysyła wydarzenie domenowe, powiadamia użytkownika i dodaje wiersz do dziennika audytu. Jeśli trzeci krok się nie powiedzie, czy test może określić, które kroki zostały już zakończone? Czy można wymusić timeout na zależności? Czy można symulować dwukrotne uruchomienie funkcji? Czy można odtworzyć przestarzałą aktualizację bez polegania na przypadkowym czasie wykonywania?

Kod, który jest łatwy do napisania, ale trudny do weryfikacji, zwiększa zaufanie do testowania ręcznego. Ktoś przechodzi ścieżkę powodzenia, widzi prawidłowy wynik i dochodzi do wniosku, że funkcjonalność jest gotowa. W środowisku produkcyjnym pojawia się natomiast konkurencja, ponawiane próby, dziwne dane oraz nieukończone operacje, których nikt nie testował lokalnie.

Weryfikuj na poziomie, gdzie istnieje ryzyko

Czystość we wszystkim oraz kpinienie z wszystkiego również nie są rozwiązaniem. Testy oparte głównie na mockach mają tendencję do zamrażania wewnętrznych szczegółów, ignorując jednocześnie to, jak faktycznie zachowuje się baza danych, sieć czy transakcje. Lepszym podejściem jest określenie, które gwarancje są ważne, i sprawdzenie każdej z nich w miejscu, gdzie faktycznie występuje ryzyko:

• Czyste transformacje można testować jednostkowo w izolacji.
• Niezmienniki bazy danych często wymagają testów integracyjnych przeciwko rzeczywistej bazie danych.
• Grenice usług mogą wymagać testów kontraktowych.
• Zachowanie w środowisku rozproszonem może zależeć od ustrukturyzowanych logów i identyfikatorów korelacji, ponieważ nie każdą warunku produkcji można wiernie odtworzyć.

Wprowadzenie zegara lub zależności zamiast korzystania z zmiennych globalnych to często najmniejsza zmiana, która umożliwia kontrolowanie kluczowych decyzji. Problemem nie jest sama niska stopa pokrycia, lecz kod, w którym kluczowe decyzje nie mogą być kontrolowane, obserwowane ani wyjaśnione po tym, jak „szczęśliwa ścieżka” przestaje działać poprawnie.

Główne wnioski

Żaden z tych sygnałów nie stanowi automatycznego werdyktu. Długa funkcja może reprezentować jedną, jasno zdefiniowaną operację, opcja logiczna może być całkowicie oczywista, wspólny helper może odzwierciedlać rzeczywiście stabilne zachowanie, a szeroka zmiana może być nieunikniona. Traktuj tę listę jako sposób na określenie, gdzie należy pogłębić analizę, a nie jako listę kontrolną do mechanicznego wypełniania.

Podczas przeglądania istotne są następujące pytania:

• Czy obowiązki są pomieszane, tak że jedna zmiana pociąga za sobą decyzje niespowiązane z nią?
• Czy osoby korzystające z funkcji mogą ją zrozumieć bez dekodowania flag pozycyjnych?

Czy każda zasada biznesowa ma jednego jasno określonego właściciela?

Czy wspólna abstrakcja ukrywa rzeczywiste różnice za opcjami?

Czy obsługa błędów zachowuje prawdę o tym, co się nie udało?

Czy różnica jest proporcjonalna do wymagania, które realizuje?

Czy ważne zachowanie można zweryfikować, gdy coś idzie nie tak?

To, co dostrzegają doświadczeni recenzenci, to w rzeczywistości odroczona niepewność: szansa, że następne wymaganie zmusi kogoś do zrozumienia niespowiązanej logiki, że kolejny incydent pozostawi zbyt mało dowodów do zbadania, że następny programista nie będzie wiedział, które zachowanie było zamierzone, lub że kolejny użytkownik doda jeszcze jedną opcję do abstrakcji, która i tak już niesie zbyt wiele. Szybko rozpoznają te problemy, ponieważ już za nie płacili – rozplątują przeładowane funkcje pomocnicze, śledzą pochłonięte wyjątki, pogodzają sprzeczne zasady oraz analizują „małą” funkcjonalność, która wpłynęła na połowę kodu. Celem nie jest eliminacja kodu, który wygląda nieuporządkowanie; chodzi o wykrycie kodu, który sprawi, że przyszła praca będzie trudniejsza, niż to konieczne.

Literatura pokrewna

• Siedem powszechnych idiomów JavaScriptu, które cicho wprowadzają przyszłe błędy — Wyjaśnia, jak codzienne wzorce w JavaScriptie, takie jak sprawdzanie wartości prawdziwych, łańcuchowanie opcjonalne i składnia rozszerzania, ukrywają założenia, które w miarę rozwoju kodu powodują ciche awarie.
• Dziewięć nawyków na poziomie kodu, które ułatwiają zaufanie do pracy seniorowych inżynierów — Omawia dziewięć konkretnych praktyk programistycznych – od klauzul ochronnych po ścisłe modelowanie danych – które sprawiają, że kod jest bardziej odporny, czytelny i łatwiejszy do debugowania w sytuacjach stresowych.