Refactoring : repérer les code smells et améliorer le code

Le refactoring (en français, refactorisation ou réusinage) consiste à modifier la structure interne d'un code sans changer son comportement visible, pour le rendre plus lisible et plus facile à faire évoluer. Il avance par petites transformations sûres (renommer, extraire une fonction, remplacer un nombre magique par une constante), chacune vérifiée par les tests. Ce chapitre présente les code smells, ces signes qui indiquent qu'un code mérite d'être retravaillé, les tests de caractérisation qui sécurisent le travail, puis un refactoring complet pas à pas, en JavaScript et en PHP.

Refactoring, correction, évolution : trois gestes à ne pas mélanger

Martin Fowler, qui a popularisé le terme, parle de « deux casquettes » : quand tu ajoutes une fonctionnalité ou corriges un bug, tu changes le comportement ; quand tu refactorises, tu ne le changes pas. Tu peux passer de l'une à l'autre plusieurs fois dans la journée, mais jamais porter les deux en même temps.

La règle a une raison pratique. Si un commit mélange un refactoring et une correction, et qu'un test casse, tu ne sais plus lequel des deux en est la cause. Et le relecteur doit démêler, ligne par ligne, ce qui change le comportement de ce qui ne fait que déplacer du code. Refactorise avant une correction pour rendre le code plus simple à corriger, ou après pour nettoyer, mais dans un commit à part.

Les code smells

Un code smell (« mauvaise odeur ») n'est pas un bug : c'est un signe qu'un code sera pénible à comprendre ou à modifier, et qu'un défaut s'y cache peut-être. L'expression vient de Kent Beck, et le catalogue de Fowler est devenu la référence. Voici les odeurs que tu rencontreras le plus en maintenance, avec le remède habituel.

Avant de refactoriser : les tests de caractérisation

Un refactoring sans tests n'est pas un refactoring, c'est une modification à l'aveugle. Or le code qui en a le plus besoin est souvent celui qui n'a pas de tests. La solution : écrire d'abord des tests de caractérisation. Ils ne vérifient pas que le code est juste ; ils figent ce qu'il fait aujourd'hui, résultats faux compris.

Voici la fonction de calcul des remboursements de Sorbier Frais, celle que le linter signalait au chapitre 7. Elle est modifiée à chaque changement de barème, et personne n'aime y toucher :

// services/remboursement.js — avant
function calc(l, u) {
  let r = 0;
  if (l.cat == 'km') {
    if (u.cv <= 4) {
      r = l.qte * 0.3;
    } else {
      r = l.qte * 0.4;
    }
  } else if (l.cat == 'repas') {
    if (l.mt > 20) {
      r = 20;
    } else {
      r = l.mt;
    }
  } else if (l.cat == 'hotel') {
    if (u.ville == 'Paris') {
      if (l.mt > 120) {
        r = 120;
      } else {
        r = l.mt;
      }
    } else {
      if (l.mt > 90) {
        r = 90;
      } else {
        r = l.mt;
      }
    }
  } else {
    r = l.mt;
  }
  return Math.round(r * 100) / 100;
}

module.exports = { calc };

Le barème interne (fictif) se devine : 0,30 € du kilomètre pour un véhicule de 4 CV ou moins, 0,40 € au-delà ; repas plafonnés à 20 € ; nuit d'hôtel plafonnée à 120 € à Paris et 90 € ailleurs ; les autres frais remboursés au réel. Tu comptes les odeurs : noms obscurs (calc, l, u, r), nombres magiques, conditions imbriquées sur trois niveaux, == au lieu de ===. Complexité cyclomatique : 9.

Pour écrire les tests de caractérisation, tu appelles la fonction avec des entrées qui passent par chaque branche, en ajoutant les valeurs limites (20 € pile, 4 CV pile), et tu notes ce qu'elle renvoie :

// tests/remboursement.test.js — tests de caractérisation, écrits AVANT le refactoring
const { calc } = require('../services/remboursement');

const cas = [
  // [ligne, salarié, montant remboursé constaté]
  [{ cat: 'km', qte: 100 }, { cv: 4 }, 30],
  [{ cat: 'km', qte: 100 }, { cv: 5 }, 40],
  [{ cat: 'km', qte: 37 }, { cv: 3 }, 11.1],
  [{ cat: 'repas', mt: 18.4 }, {}, 18.4],
  [{ cat: 'repas', mt: 20 }, {}, 20],
  [{ cat: 'repas', mt: 27.5 }, {}, 20],
  [{ cat: 'hotel', mt: 135 }, { ville: 'Paris' }, 120],
  [{ cat: 'hotel', mt: 110 }, { ville: 'Paris' }, 110],
  [{ cat: 'hotel', mt: 110 }, { ville: 'Lyon' }, 90],
  [{ cat: 'hotel', mt: 75 }, { ville: 'Lyon' }, 75],
  [{ cat: 'autre', mt: 12.3 }, {}, 12.3],
];

describe('montant remboursé', () => {
  for (const [ligne, salarie, attendu] of cas) {
    test(`${ligne.cat} ${ligne.qte ?? ligne.mt} → ${attendu}`, () => {
      expect(calc(ligne, salarie)).toBe(attendu);
    });
  }
});

Les onze tests passent, par construction. Ils sont désormais ton filet : à chaque étape du refactoring, tu les relances, et ils doivent rester verts.

Un refactoring pas à pas

On avance par petites étapes. Après chacune, on relance les tests ; s'ils restent verts, on commite (ou au moins on garde un point de retour) et on passe à la suivante. S'ils passent au rouge, on annule la dernière étape, qui est petite, au lieu de chercher l'erreur dans un gros changement.

Étape 1 : renommer. calc devient montantRembourse, l devient ligne, u devient salarie, r devient montant. Dans VS Code, place le curseur sur le nom et appuie sur F2 (Renommer le symbole) : l'éditeur renomme toutes les occurrences, y compris dans les autres fichiers et dans les tests, sans toucher aux autres variables qui s'appelleraient aussi l.

Étape 2 : comparer avec ===. Les catégories et les villes sont toujours des chaînes, donc === donne les mêmes résultats que ==, sans conversion implicite. Les tests le confirment.

Étape 3 : nommer les nombres magiques. Chaque valeur du barème devient une constante, avec son unité en commentaire :

const TAUX_KM_PETITE_CYLINDREE = 0.3;      // €/km, véhicule de 4 CV ou moins
const TAUX_KM_GRANDE_CYLINDREE = 0.4;      // €/km, au-delà de 4 CV
const PUISSANCE_MAX_PETITE_CYLINDREE = 4;  // CV
const PLAFOND_REPAS = 20;                  // €
const PLAFOND_HOTEL_PARIS = 120;           // € par nuit
const PLAFOND_HOTEL_PROVINCE = 90;         // € par nuit

Le prochain changement de barème se fera en une ligne, sans chercher où traîne le 20.

Étape 4 : remplacer les plafonds imbriqués par Math.min. Le motif « si le montant dépasse le plafond, prendre le plafond, sinon le montant » s'écrit Math.min(montant, plafond). Trois blocs de cinq lignes deviennent trois expressions.

Étape 5 : extraire des fonctions. Le calcul kilométrique et le choix du plafond d'hôtel deviennent deux fonctions qui portent un nom métier. Sélectionne le bloc, puis Ctrl+. (ou l'ampoule) et Extraire vers une fonction dans la portée du module : VS Code crée la fonction et remplace le bloc par son appel.

Étape 6 : simplifier l'aiguillage. La cascade de else if sur la catégorie devient un switch, et chaque branche renvoie directement son résultat.

Le résultat, toujours vérifié par les mêmes onze tests :

// services/remboursement.js — après
const TAUX_KM_PETITE_CYLINDREE = 0.3;      // €/km, véhicule de 4 CV ou moins
const TAUX_KM_GRANDE_CYLINDREE = 0.4;      // €/km, au-delà de 4 CV
const PUISSANCE_MAX_PETITE_CYLINDREE = 4;  // CV
const PLAFOND_REPAS = 20;                  // €
const PLAFOND_HOTEL_PARIS = 120;           // € par nuit
const PLAFOND_HOTEL_PROVINCE = 90;         // € par nuit

const arrondirAuCentime = (montant) => Math.round(montant * 100) / 100;

function indemniteKilometrique(kilometres, puissanceFiscale) {
  const taux = puissanceFiscale <= PUISSANCE_MAX_PETITE_CYLINDREE
    ? TAUX_KM_PETITE_CYLINDREE
    : TAUX_KM_GRANDE_CYLINDREE;
  return kilometres * taux;
}

function plafondHotel(ville) {
  return ville === 'Paris' ? PLAFOND_HOTEL_PARIS : PLAFOND_HOTEL_PROVINCE;
}

function montantRembourse(ligne, salarie) {
  switch (ligne.cat) {
    case 'km':
      return arrondirAuCentime(indemniteKilometrique(ligne.qte, salarie.cv));
    case 'repas':
      return arrondirAuCentime(Math.min(ligne.mt, PLAFOND_REPAS));
    case 'hotel':
      return arrondirAuCentime(Math.min(ligne.mt, plafondHotel(salarie.ville)));
    default:
      return arrondirAuCentime(ligne.mt);
  }
}

module.exports = { montantRembourse };

Le code est plus long en lignes, mais chaque fonction se lit d'un coup d'œil, et la complexité de montantRembourse tombe de 9 à 4. Surtout, il dit ce qu'il fait : on lit « le montant remboursé d'un repas est le montant, plafonné au plafond repas ». Au-delà des onze tests, les deux versions ont été comparées sur plus de 16 000 combinaisons d'entrées : elles renvoient exactement les mêmes résultats.

Les refactorings de base

Une dizaine de transformations couvrent l'essentiel du travail quotidien.

Les clauses de garde méritent un exemple, car elles transforment la lisibilité d'un code imbriqué. En PHP, sur Sorbier Véhicules :

<?php
// Avant : la règle principale est enfouie au troisième niveau
public function peutAnnuler(Reservation $reservation, Utilisateur $utilisateur): bool
{
    if ($reservation->statut() !== 'annulee') {
        if ($reservation->debut() > new DateTimeImmutable()) {
            if ($utilisateur->estResponsableParc() || $reservation->demandeur()->id() === $utilisateur->id()) {
                return true;
            }
        }
    }
    return false;
}

// Après : les cas qui interdisent l'annulation sortent tout de suite
public function peutAnnuler(Reservation $reservation, Utilisateur $utilisateur): bool
{
    if ($reservation->statut() === 'annulee') {
        return false;
    }
    if ($reservation->debut() <= new DateTimeImmutable()) {
        return false;                       // la réservation a déjà commencé
    }
    return $utilisateur->estResponsableParc()
        || $reservation->demandeur()->id() === $utilisateur->id();
}

Chaque condition se lit seule, et la règle principale (le responsable du parc ou le demandeur peuvent annuler) apparaît en dernière ligne, sans indentation. Attention à inverser correctement les conditions : le contraire de > est <=, pas <.

Supprimer la duplication, en PHP

La duplication est l'odeur la plus coûteuse en maintenance : un bug corrigé dans une copie reste dans les autres. Sur Sorbier Véhicules, deux fonctions formatent une période :

<?php
// Avant
function libelleReservation(array $r): string
{
    $debut = (new DateTimeImmutable($r['debut']))->format('d/m/Y H:i');
    $fin = (new DateTimeImmutable($r['fin']))->format('d/m/Y H:i');
    return $r['immatriculation'] . ' du ' . $debut . ' au ' . $fin;
}

function libelleEntretien(array $e): string
{
    $debut = (new DateTimeImmutable($e['debut']))->format('d/m/Y H:i');
    $fin = (new DateTimeImmutable($e['fin']))->format('d/m/Y H:i');
    return 'Entretien ' . $e['garage'] . ' du ' . $debut . ' au ' . $fin;
}

On extrait la partie commune dans une fonction nommée, et les deux fonctions l'appellent :

<?php
// Après
const FORMAT_DATE_HEURE = 'd/m/Y H:i';

function periode(string $debut, string $fin): string
{
    $format = fn (string $date): string => (new DateTimeImmutable($date))->format(FORMAT_DATE_HEURE);
    return 'du ' . $format($debut) . ' au ' . $format($fin);
}

function libelleReservation(array $r): string
{
    return $r['immatriculation'] . ' ' . periode($r['debut'], $r['fin']);
}

function libelleEntretien(array $e): string
{
    return 'Entretien ' . $e['garage'] . ' ' . periode($e['debut'], $e['fin']);
}

Le jour où l'on voudra afficher « du 19/11 au 20/11 » sans l'année, ou en toutes lettres, on modifiera un seul endroit. Les deux libellés produisent exactement les mêmes chaînes qu'avant : un test de caractérisation sur chacun le prouve.

Quand ne pas refactoriser

Le refactoring n'est pas toujours le bon moment.

• Quand tu ne peux pas tester. Sans tests existants ni possibilité d'en écrire, commence par rendre le code testable (en extrayant une fonction pure, par exemple), ou abstiens-toi.
• Pendant une correction urgente. Le hotfix contient le correctif, et seulement lui. Le nettoyage viendra après, à tête reposée.
• Sur un code qui va disparaître, ou que personne ne modifiera plus : ses intérêts de dette sont nuls.
• En un seul gros commit. Un refactoring de 2 000 lignes ne se relit pas et ne s'annule pas proprement. Si le chantier est grand, découpe-le en étapes livrables.
• Pour le plaisir. Refactoriser un code qui marche, qu'on ne touchera pas, juste parce qu'il ne correspond pas à tes goûts, c'est prendre un risque sans gain.

À l'E6 SLAM, la commission apprécie un refactoring ciblé et justifié (une fonction dupliquée remplacée par une seule, avec ses tests), c'est même un indicateur de la sous-compétence « évaluer la qualité ». Mais l'expression des besoins passe d'abord.

Exercices

// calcule
function f(d, tab, x, y, z) {
  let res = [];
  for (let i = 0; i < tab.length; i++) {
    if (tab[i].s != 'annulee') {
      if (tab[i].v == x) {
        if (new Date(tab[i].d) >= d) {
          res.push(tab[i]);
        }
      }
    }
  }
  // ancien filtre, ne plus utiliser
  // res = res.filter(r => r.km > 0);
  return res;
}

function reservationsAVenir(depuis, reservations, idVehicule) {
  return reservations.filter((r) =>
    r.statut !== 'annulee'
    && r.idVehicule === idVehicule
    && new Date(r.debut) >= depuis);
}

function peutEtreValidee(note, utilisateur) {
  if (note) {
    if (note.etat === 'cloturee') {
      if (utilisateur.role === 'comptable') {
        if (note.idUtilisateur !== utilisateur.id) {
          return true;
        } else {
          return false;
        }
      } else {
        return false;
      }
    } else {
      return false;
    }
  } else {
    return false;
  }
}

function peutEtreValidee(note, utilisateur) {
  if (!note) return false;
  if (note.etat !== 'cloturee') return false;
  if (utilisateur.role !== 'comptable') return false;
  return note.idUtilisateur !== utilisateur.id;   // un comptable ne valide pas sa propre note
}

const comptable = { id: 1, role: 'comptable' };
const salarie = { id: 2, role: 'salarie' };

test('pas de note', () => {
  expect(peutEtreValidee(null, comptable)).toBe(false);
});
test('note pas encore clôturée', () => {
  expect(peutEtreValidee({ etat: 'saisie', idUtilisateur: 2 }, comptable)).toBe(false);
});
test('un salarié ne valide pas', () => {
  expect(peutEtreValidee({ etat: 'cloturee', idUtilisateur: 2 }, salarie)).toBe(false);
});
test('un comptable ne valide pas sa propre note', () => {
  expect(peutEtreValidee({ etat: 'cloturee', idUtilisateur: 1 }, comptable)).toBe(false);
});
test('un comptable valide la note clôturée d\'un autre', () => {
  expect(peutEtreValidee({ etat: 'cloturee', idUtilisateur: 2 }, comptable)).toBe(true);
});