Vous ouvrez une fonction pour corriger un bug d’une ligne et quarante minutes plus tard, vous êtes encore en train de suivre quelle branche définit quelle variable. Trois tâches sans rapport entre elles sont soudées dans un seul bloc, et aucune n’a de nom. Vous tirez un fil et vous ne pouvez pas dire lesquels des autres bougent.

C’est ça, le spaghetti code, et le nom n’a rien à voir avec l’indentation ou le style de nommage. Il décrit le flux de contrôle : l’exécution saute entre les conditions comme un brin de pâtes qui croise tous les autres dans le bol, et suivre un seul chemin jusqu’au bout signifie garder quatre niveaux de if en tête à la fois.

Ce qui fait vraiment du spaghetti code

Quatre symptômes apparaissent ensemble dans presque tous les cas, et chacun se corrige indépendamment :

  • Imbrication profonde. Trois ou quatre niveaux de if/else les uns dans les autres. Chaque niveau ajoute de l’indentation et une condition qu’il faut garder vraie en tête pour savoir quelle ligne on lit.
  • Fonctions qui font plusieurs travaux. Une fonction valide l’entrée, calcule un résultat, modifie l’état et écrit un log. Vous changez le calcul et vous risquez le log ; vous testez la validation et vous devez simuler l’état.
  • État partagé caché. Une variable globale ou un dictionnaire au niveau du module, modifié depuis l’intérieur d’une fonction dont le nom ne le laisse pas deviner. Deux appelants peuvent se marcher dessus sans que le code de l’un ou l’autre montre pourquoi.
  • Branches copiées-collées. La même forme de logique répétée en changeant une seule valeur, au lieu d’être extraite une fois. Vous corrigez une copie, vous oubliez l’autre, et vous avez un bug qui n’apparaît que sur la moitié des entrées.

Aucun de ces problèmes ne nécessite une réécriture. Chacun a un refactoring nommé qui l’élimine sans changer ce que fait le code. Cette contrainte compte plus qu’il n’y paraît : si vous repérez un bug pendant le nettoyage, ne le corrigez pas dans le même changement. Faites-le séparément, sinon vous ne pourrez pas dire quel changement a causé quelle régression.

Une fonction réelle avec tous les symptômes du spaghetti code

Voici process_order, tirée du flux de commande d’une petite boutique. Lisez-la une fois et notez où vous perdez le fil.

 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

Les quatre symptômes sont là. La ligne qui facture le client se trouve à quatre niveaux de profondeur. La fonction vérifie le stock, calcule les prix, applique une remise de fidélité, modifie INVENTORY, expédie (via print, à la place d’un vrai appel d’expédition) et totalise la commande. INVENTORY est un dictionnaire au niveau du module que n’importe quelle autre fonction peut aussi modifier, et rien dans la signature ne vous prévient. Les branches VIP et non-VIP ont la même forme avec des chiffres différents, donc un troisième palier de remise veut dire une troisième copie.

Comment refactoriser du spaghetti code sans le casser

Un changement petit et mécanique à la fois. Chacun laisse le comportement identique et a son propre commit. Si l’étape 3 casse quelque chose, git bisect peut trouver lequel des cinq petits commits en est responsable. Un seul commit « tout refactoriser » ne donne rien à bisect pour réduire la recherche.

Figez le comportement avec un test de caractérisation

Avant de changer une ligne, écrivez un test qui enregistre ce que fait la fonction aujourd’hui. Pas ce qu’elle devrait faire. C’est un test de caractérisation : vous figez le comportement actuel pour remarquer le moment où un changement « sûr » ne l’est pas.

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, sous le seuil de livraison gratuite de $100, + $5 de livraison 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  # rien expédié, livraison DE sous $100

Exécutez les deux d’abord contre la fonction encore intacte, pour être sûr que les nombres attendus viennent du code et pas d’une faute de frappe. Réinitialisez INVENTORY avant chaque test. Chaque étape ci-dessous s’exécute contre cette même paire. Tant qu’ils passent, le comportement n’a pas bougé.

Extrayez des fonctions pour le prix et la livraison

Extrayez la logique de prix et de livraison dans des fonctions dont le nom dit ce qu’elles font. La logique elle-même ne change pas à cette étape.

 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"])

Exécutez les tests : mêmes deux nombres, même compte d’inventaire. Faites le commit avec un message qui dit exactement ce qui s’est passé : « extract price_line_item and calculate_shipping, no behavior change ».

Remplacez les conditions imbriquées par des guard clauses

La vérification du stock dans process_order représente trois niveaux d’imbrication autour de la seule ligne qui compte. Transformez chaque vérification en sortie anticipée pour le cas qui ne peut pas continuer, et le chemin de succès cesse d’être indenté sous tout le reste.

 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"])

Mêmes tests, mêmes résultats. Le corps de la boucle se lit maintenant de haut en bas : rejeter, rejeter, faire le travail. Ce sont les mêmes branches qu’avant, à un niveau de profondeur au lieu de trois.

Remplacez l’argument booléen par une table de correspondance

is_vip change ce que fait price_line_item en interne. Chaque point d’appel doit savoir que ce booléen existe, et un troisième palier veut dire une troisième branche copiée-collée. Déplacez les chiffres dans des données :

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 change de la même façon : son paramètre est renommé de is_vip à tier, et l’unique appel qu’elle fait à price_line_item(item, is_vip) devient price_line_item(item, tier). Rien d’autre ne change dans le corps de la fonction. Un appel comme process_order(order, tier="vip") se lit tout seul, et un nouveau palier est une nouvelle entrée dans DISCOUNT_RATES au lieu d’un nouveau bloc if/else. C’est la seule étape qui change une signature, donc les tests ont besoin d’un ajustement correspondant : passez tier="vip" au lieu de is_vip=True. Les assertions restent telles quelles, parce que la sortie pour la même commande n’a pas changé.

Repoussez les effets de bord vers les extrémités

price_line_item est maintenant pure : mêmes entrées, mêmes sorties, rien n’est modifié. process_order modifie encore INVENTORY et appelle print au milieu d’une boucle, ce qui explique pourquoi la tester nécessite une réinitialisation globale plutôt qu’un simple appel de fonction. Séparez la décision de l’action. Calculez ce qui devrait se passer, renvoyez-le, et laissez l’appelant agir en conséquence.

 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"]

Deux détails font de ceci une correspondance exacte avec l’original, pas seulement approchée. La branche de la commande vide passe toujours par calculate_shipping au lieu de renvoyer un 0 fixe : l’original facture la livraison même sur une commande vide, donc le refactoring doit continuer à la facturer aussi. Et plan_order vérifie le stock contre une copie locale remaining qu’elle décrémente au fur et à mesure, pas directement contre INVENTORY. Une commande avec le même SKU sur deux lignes est donc toujours vérifiée contre un stock décroissant, exactement comme le faisait la décrémentation en ligne de l’original — vérifier les deux lignes contre le même nombre intact aurait permis à une seconde ligne de passer une vérification que la première avait déjà épuisée.

plan_order lit INVENTORY mais n’y écrit jamais et n’imprime rien, donc vous pouvez l’appeler mille fois dans un test sans rien réinitialiser. process_order est maintenant le seul endroit qui touche INVENTORY ou print, et elle tient sur un seul écran avec de la marge.

Ce que la fonction refactorisée gagne par rapport à l’originale

L’imbrication est passée de quatre niveaux à un. Chaque fonction fait l’unique travail que son nom décrit. INVENTORY est écrit depuis exactement un seul endroit, et il ne reste plus aucune branche copiée-collée à corriger deux fois. Ajouter un palier « grossiste » est un changement d’une ligne dans DISCOUNT_RATES.

L’historique compte autant que le code. Vous avez maintenant cinq commits (le test, puis quatre refactorings), chacun vert contre les mêmes tests de caractérisation. Si une remise se révèle fausse dans six mois, git blame sur la ligne du prix atterrit sur un commit assez petit pour être lu en entier. Écrasez les cinq en un seul commit « refactor » à la fusion de la branche et vous perdez cet avantage. Git Rebase vs Merge explique comment chaque stratégie de fusion façonne l’historique qu’il vous reste.

Quand ne pas refactoriser du spaghetti code

Un test de caractérisation suppose un comportement actuel observable, qui mérite d’être préservé. Si l’échéance est aujourd’hui et qu’il n’y a aucun test sur lequel s’appuyer, refactoriser transforme un désordre qui fonctionne en un désordre cassé avec un meilleur formatage. Livrez le correctif, puis planifiez le nettoyage comme une tâche à part. Ne le glissez pas dans un commit de correction de bug où il cache le vrai changement.

Le code destiné à la suppression ne mérite pas non plus de refactoring. Si la fonctionnalité doit être remplacée au prochain sprint, les heures vont au remplacement, pas au polissage de l’ancienne version pour les deux semaines qu’il lui reste.

Restructurer du code sans aucun test est une réécriture, et vous n’avez aucun moyen de savoir ce que vous avez cassé. Si vous ne pouvez pas tester le comportement actuel, la première étape est de le rendre testable.

Choisissez une fonction que vous évitez d’ouvrir, écrivez un seul test de caractérisation contre ce qu’elle fait actuellement, et faites la première extraction. Committez avant de toucher à quoi que ce soit d’autre.

Articles associés