Otwierasz funkcję, żeby naprawić błąd w jednej linijce, i czterdzieści minut później wciąż śledzisz, która gałąź ustawia którą zmienną. Trzy niepowiązane ze sobą zadania są zespawane w jeden blok, i żadne z nich nie ma nazwy. Pociągasz za jedną nitkę i nie wiesz, które inne się poruszają.

To właśnie spaghetti code, i nazwa nie ma nic wspólnego z wcięciami czy stylem nazewnictwa. Opisuje przepływ sterowania: wykonanie skacze między warunkami tak, jak jeden makaron krzyżuje się z każdym innym w misce, a podążanie jedną ścieżką do końca oznacza trzymanie w głowie czterech poziomów if naraz.

Co naprawdę czyni kod spaghetti code

Cztery objawy pojawiają się razem niemal w każdym przypadku, i każdy da się naprawić osobno:

  • Głębokie zagnieżdżenie. Trzy albo cztery poziomy if/else jeden w drugim. Każdy poziom dodaje wcięcie i warunek, który musisz trzymać w głowie jako prawdziwy, żeby wiedzieć, którą linię czytasz.
  • Funkcje robiące kilka rzeczy naraz. Jedna funkcja waliduje dane wejściowe, liczy wynik, zmienia stan i zapisuje log. Zmieniasz obliczenia i ryzykujesz logowanie; testujesz walidację i musisz udawać stan.
  • Ukryty współdzielony stan. Zmienna globalna albo słownik na poziomie modułu, modyfikowany z wnętrza funkcji, której nazwa niczego nie sugeruje. Dwa wywołania mogą się wzajemnie zderzać, a kod żadnego z nich nie pokazuje dlaczego.
  • Gałęzie kopiuj-wklej. Ta sama logika powtórzona ze zmienioną jedną wartością, zamiast wydzielenia jej raz. Poprawiasz jedną kopię, zapominasz o drugiej, i masz buga, który ujawnia się tylko na połowie danych wejściowych.

Żaden z nich nie wymaga przepisania od zera. Każdy ma nazwany refaktoring, który go usuwa bez zmiany tego, co robi kod. To ograniczenie liczy się bardziej, niż się wydaje: jeśli zauważysz buga podczas porządkowania, nie naprawiaj go w tej samej zmianie. Zrób to osobno, inaczej nie dowiesz się, która zmiana spowodowała którą regresję.

Prawdziwa funkcja ze wszystkimi objawami spaghetti code

Oto process_order, z procesu obsługi zamówień w małym sklepie. Przeczytaj ją raz i zauważ, gdzie gubisz wątek.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35

INVENTORY = {"usb-cable": 42, "laptop-stand": 7, "webcam": 0}

def process_order(order, is_vip):
    total = 0
    if order["items"]:
        for item in order["items"]:
            if item["sku"] in INVENTORY:
                if INVENTORY[item["sku"]] >= item["qty"]:
                    price = item["price"]
                    if is_vip:
                        if item["qty"] > 10:
                            price = price * 0.8
                        else:
                            price = price * 0.9
                    else:
                        if item["qty"] > 10:
                            price = price * 0.95
                        else:
                            price = price
                    total = total + price * item["qty"]
                    INVENTORY[item["sku"]] -= item["qty"]
                    print(f"shipped {item['qty']} x {item['sku']}")
                else:
                    print(f"not enough stock for {item['sku']}")
            else:
                print(f"unknown sku {item['sku']}")
    else:
        print("empty order")

    if total > 100:
        shipping = 0 if order["country"] == "US" else 15
    else:
        shipping = 5 if order["country"] == "US" else 20

    return total + shipping

Wszystkie cztery objawy są tutaj. Linia obciążająca klienta znajduje się cztery poziomy w głąb. Funkcja sprawdza stan magazynowy, liczy ceny, stosuje rabat lojalnościowy, modyfikuje INVENTORY, wysyła (przez print, zastępujący prawdziwe wywołanie wysyłki) i sumuje zamówienie. INVENTORY to słownik na poziomie modułu, który może zmodyfikować też każda inna funkcja, i nic w sygnaturze cię o tym nie ostrzega. Gałęzie VIP i nie-VIP mają ten sam kształt z innymi liczbami, więc trzeci poziom rabatu oznacza trzecią kopię.

Jak zrefaktoryzować spaghetti code, nie psując go

Jedna mała, mechaniczna zmiana na raz. Każda zostawia zachowanie identyczne i dostaje swój własny commit. Jeśli krok 3 coś popsuje, git bisect może znaleźć, który z pięciu małych commitów to zrobił. Jeden commit „zrefaktoryzuj wszystko” nie daje bisectowi niczego do zawężenia.

Zablokuj zachowanie testem charakteryzującym

Zanim zmienisz linię, napisz test, który zapisuje, co funkcja robi dzisiaj. Nie to, co powinna robić. To jest test charakteryzujący: zamrażasz obecne zachowanie, żeby zauważyć moment, w którym „bezpieczna” zmiana taka nie jest.

1
2
3
4
5
6
7
8
9

def test_current_behavior_vip_bulk_order():
    INVENTORY["usb-cable"] = 42
    order = {"items": [{"sku": "usb-cable", "qty": 12, "price": 10.0}], "country": "US"}
    assert process_order(order, is_vip=True) == 101.0  # 12 * 10 * 0.8 = 96, poniżej progu darmowej wysyłki $100, + $5 wysyłki US
    assert INVENTORY["usb-cable"] == 30

def test_current_behavior_unknown_sku():
    order = {"items": [{"sku": "flux-capacitor", "qty": 1, "price": 999}], "country": "DE"}
    assert process_order(order, is_vip=False) == 20  # nic nie wysłano, wysyłka DE poniżej $100

Uruchom oba testy najpierw na jeszcze nietkniętej funkcji, żeby wiedzieć, że oczekiwane liczby pochodzą z kodu, a nie z literówki. Resetuj INVENTORY przed każdym testem. Każdy krok poniżej działa na tej samej parze. Dopóki przechodzą, zachowanie się nie zmieniło.

Wydziel funkcje dla ceny i wysyłki

Wydziel logikę ceny i wysyłki do funkcji, których nazwy mówią, co robią. Sama logika w tym kroku się nie zmienia.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29

def price_line_item(item, is_vip):
    price = item["price"]
    if is_vip:
        price *= 0.8 if item["qty"] > 10 else 0.9
    else:
        price *= 0.95 if item["qty"] > 10 else 1.0
    return price * item["qty"]

def calculate_shipping(total, country):
    if total > 100:
        return 0 if country == "US" else 15
    return 5 if country == "US" else 20

def process_order(order, is_vip):
    total = 0
    if order["items"]:
        for item in order["items"]:
            if item["sku"] in INVENTORY:
                if INVENTORY[item["sku"]] >= item["qty"]:
                    total += price_line_item(item, is_vip)
                    INVENTORY[item["sku"]] -= item["qty"]
                    print(f"shipped {item['qty']} x {item['sku']}")
                else:
                    print(f"not enough stock for {item['sku']}")
            else:
                print(f"unknown sku {item['sku']}")
    else:
        print("empty order")
    return total + calculate_shipping(total, order["country"])

Uruchom testy: te same dwie liczby, ten sam stan magazynowy. Zrób commit z komunikatem, który mówi dokładnie, co się stało: „extract price_line_item and calculate_shipping, no behavior change”.

Zastąp zagnieżdżone warunki guard clause’ami

Sprawdzenie stanu magazynowego w process_order to trzy poziomy zagnieżdżenia wokół jednej linii, która się liczy. Zamień każde sprawdzenie na wczesne wyjście dla przypadku, który nie może kontynuować, a ścieżka sukcesu przestaje być wcięta pod wszystkim innym.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18

def process_order(order, is_vip):
    if not order["items"]:
        print("empty order")
        return 0 + calculate_shipping(0, order["country"])

    total = 0
    for item in order["items"]:
        if item["sku"] not in INVENTORY:
            print(f"unknown sku {item['sku']}")
            continue
        if INVENTORY[item["sku"]] < item["qty"]:
            print(f"not enough stock for {item['sku']}")
            continue
        total += price_line_item(item, is_vip)
        INVENTORY[item["sku"]] -= item["qty"]
        print(f"shipped {item['qty']} x {item['sku']}")

    return total + calculate_shipping(total, order["country"])

Te same testy, te same wyniki. Ciało pętli czyta się teraz od góry do dołu: odrzuć, odrzuć, zrób pracę. To te same gałęzie co wcześniej, jeden poziom głębokości zamiast trzech.

Zastąp argument flagowy tabelą przeglądową

is_vip przełącza, co price_line_item robi wewnętrznie. Każde miejsce wywołania musi wiedzieć, że ten boolean istnieje, a trzeci poziom oznacza trzecią skopiowaną gałąź. Przenieś liczby do danych:

1
2
3
4
5
6
7
8
9

DISCOUNT_RATES = {
    "vip": {"bulk": 0.8, "standard": 0.9},
    "regular": {"bulk": 0.95, "standard": 1.0},
}

def price_line_item(item, tier):
    rates = DISCOUNT_RATES[tier]
    rate = rates["bulk"] if item["qty"] > 10 else rates["standard"]
    return item["price"] * rate * item["qty"]

process_order zmienia się tak samo: jej parametr jest zmieniany z is_vip na tier, a jedyne wywołanie price_line_item(item, is_vip) staje się price_line_item(item, tier). Nic więcej w ciele funkcji się nie zmienia. Wywołanie takie jak process_order(order, tier="vip") czyta się samo, a nowy poziom to jeden nowy wpis w DISCOUNT_RATES zamiast kolejnej gałęzi if/else. To jedyny krok, który zmienia sygnaturę, więc testy potrzebują odpowiedniej zmiany: przekaż tier="vip" zamiast is_vip=True. Asercje zostają takie same, bo wynik dla tego samego zamówienia się nie zmienił.

Przenieś efekty uboczne na brzegi

price_line_item jest teraz czysta: te same dane wejściowe, ten sam wynik, nic nie jest modyfikowane. process_order wciąż modyfikuje INVENTORY i wywołuje print w środku pętli, dlatego testowanie jej wymaga globalnego resetu zamiast zwykłego wywołania funkcji. Oddziel decydowanie od działania. Oblicz, co powinno się stać, zwróć to, i pozwól wywołującemu na tej podstawie działać.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33

def plan_order(order, tier):
    if not order["items"]:
        return {"total": calculate_shipping(0, order["country"]), "shipments": [], "messages": ["empty order"]}

    remaining = dict(INVENTORY)
    total = 0
    shipments, messages = [], []
    for item in order["items"]:
        sku = item["sku"]
        if sku not in remaining:
            messages.append(f"unknown sku {sku}")
            continue
        if remaining[sku] < item["qty"]:
            messages.append(f"not enough stock for {sku}")
            continue
        total += price_line_item(item, tier)
        remaining[sku] -= item["qty"]
        shipments.append((sku, item["qty"]))
        messages.append(f"shipped {item['qty']} x {sku}")

    return {
        "total": total + calculate_shipping(total, order["country"]),
        "shipments": shipments,
        "messages": messages,
    }

def process_order(order, tier):
    plan = plan_order(order, tier)
    for sku, qty in plan["shipments"]:
        INVENTORY[sku] -= qty
    for message in plan["messages"]:
        print(message)
    return plan["total"]

Dwa szczegóły sprawiają, że to dokładne odwzorowanie oryginału, a nie tylko przybliżone. Gałąź pustego zamówienia nadal przechodzi przez calculate_shipping zamiast zwracać stałe 0. Oryginał nalicza wysyłkę nawet przy pustym zamówieniu, więc refaktoring musi nadal ją naliczać. plan_order sprawdza też stan magazynowy względem lokalnej kopii remaining, a nie bezpośrednio względem INVENTORY. Dzięki temu zamówienie z tym samym SKU na dwóch liniach jest nadal sprawdzane względem malejącego stanu magazynowego, dokładnie tak jak robiło to wbudowane zmniejszanie w oryginale. Sprawdzenie obu linii względem tej samej, nietkniętej liczby pozwoliłoby drugiej linii przejść sprawdzenie, które pierwsza powinna była już wyczerpać.

plan_order czyta INVENTORY, ale nigdy do niego nie zapisuje ani nic nie wypisuje, więc możesz wywołać ją tysiąc razy w teście bez resetowania czegokolwiek. process_order jest teraz jedynym miejscem, które dotyka INVENTORY lub print, i mieści się na jednym ekranie z zapasem miejsca.

Co zyskuje zrefaktoryzowana funkcja względem oryginału

Zagnieżdżenie spadło z czterech poziomów do jednego. Każda funkcja robi jedną rzecz, którą opisuje jej nazwa. INVENTORY jest zapisywany z dokładnie jednego miejsca, i nie zostaje żadna skopiowana gałąź do poprawienia dwa razy. Dodanie poziomu „hurtowego” to zmiana jednej linii w DISCOUNT_RATES.

Historia liczy się tak samo jak kod. Masz teraz pięć commitów (test, potem cztery refaktoringi), każdy zielony na tych samych testach charakteryzujących. Jeśli za sześć miesięcy jakiś rabat wyjdzie źle, git blame na linii ceny wskaże commit na tyle mały, żeby przeczytać go w całości. Jeśli przy scalaniu gałęzi zgnieciesz wszystkie pięć w jeden commit „refactor”, tracisz tę przewagę. Git Rebase vs Merge pokazuje, jak każda strategia scalania kształtuje historię, która ci zostaje.

Kiedy nie refaktoryzować spaghetti code

Test charakteryzujący wymaga, żeby obecne zachowanie było obserwowalne i warte zachowania. Jeśli termin jest dziś i nie masz testu, na którym możesz się oprzeć, refaktoryzacja zamienia działający bałagan w zepsuty bałagan z lepszym formatowaniem. Wypuść poprawkę, a potem zaplanuj porządkowanie jako osobne zadanie. Nie przemycaj go w commicie z poprawką buga, gdzie ukrywa prawdziwą zmianę.

Kod przeznaczony do usunięcia też nie zasługuje na refaktoring. Jeśli funkcjonalność zostanie zastąpiona w następnym sprincie, godziny idą w zastąpienie, a nie w polerowanie starej wersji przez dwa tygodnie, które jej zostały.

Restrukturyzacja kodu bez żadnego testu to przepisanie od zera, i nie masz jak się dowiedzieć, co zepsułeś. Jeśli nie możesz przetestować obecnego zachowania, pierwszym krokiem jest sprawienie, żeby dało się je przetestować.

Wybierz jedną funkcję, której unikasz otwierania, napisz jeden test charakteryzujący na to, co robi teraz, i zrób pierwszą ekstrakcję. Zrób commit, zanim dotkniesz czegokolwiek innego.

Powiązane artykuły