5 erreurs de code review qui laissent passer la dette architecturale

Les code reviews sont la dernière ligne de défense avant que les changements n'arrivent sur votre branche principale. Elles attrapent les bugs, font respecter le style et partagent la connaissance au sein de l'équipe. Mais la plupart des code reviews ont un angle mort de la taille d'un immeuble : elles se concentrent sur les arbres et passent à côté de la forêt.

Les développeurs reviewent les lignes de code individuelles avec précision. Ils repèrent les coquilles, suggèrent de meilleurs noms de variables, signalent les oublis de gestion d'erreur. Tout cela a de la valeur. Mais pendant que l'équipe débat pour savoir si une fonction devrait utiliser map ou reduce, les problèmes structurels passent la barrière sans encombre.

La dette architecturale n'arrive pas dans une seule PR dramatique. Elle s'accumule à travers des centaines de petits changements apparemment inoffensifs. Un import ici, un raccourci là. Chacun passe la code review parce que chacun semble correct pris isolément. Ce n'est que des mois plus tard, quand la codebase est devenue un amas enchevêtré, que l'équipe réalise que les reviews vérifiaient les mauvaises choses.

Voici les cinq erreurs de review les plus courantes qui laissent passer la dette architecturale — et quoi faire à la place.

Erreur 1 : Ne reviewer que le code ligne par ligne, pas l'impact au niveau des modules

C'est le problème le plus répandu. Un reviewer ouvre le diff, lit les lignes modifiées, vérifie la logique et approuve. La review est minutieuse au niveau micro mais complètement aveugle au niveau macro.

Le problème

Prenons une PR qui ajoute une nouvelle fonction utilitaire dans un module partagé :

// src/utils/format.ts
import { getUserLocale } from '../services/user-service';

export function formatCurrency(amount: number): string {
  const locale = getUserLocale();
  return new Intl.NumberFormat(locale, {
    style: 'currency',
    currency: 'USD',
  }).format(amount);
}

Une review ligne par ligne pourrait approuver cela sans sourciller. Le code est propre, il gère la localisation, il utilise Intl.NumberFormat. Tout semble correct.

Mais prenez du recul. Un module utilitaire (utils/format.ts) dépend maintenant d'un module de service (services/user-service.ts). Les modules utilitaires devraient être purs — ils prennent des entrées et retournent des sorties. Ils ne devraient pas aller chercher des données dans la couche service. Ce simple import crée une dépendance qui viole l'architecture, et il sera copié par chaque développeur qui le verra comme un pattern à suivre.

La solution

Avant d'approuver toute PR, prenez mentalement du recul et demandez-vous : « Quelle dépendance au niveau module ce changement crée-t-il ? » Si un fichier dans utils/ importe depuis services/, ou si un fichier dans components/ importe directement depuis database/, c'est un signal d'alarme, quelle que soit la propreté des lignes individuelles.

Certaines équipes formalisent cela en exigeant une brève note d'impact architectural dans les PRs qui traversent les frontières de modules. Même une seule phrase — « Ce changement ajoute une dépendance de la couche utils vers la couche services » — oblige l'auteur et le reviewer à reconnaître l'implication structurelle.

Erreur 2 : Ignorer le sens des dépendances

Toute application bien conçue a un sens de dépendance. Dans une architecture en couches typique :

Contrôleurs → Services → Repositories → Base de données
     ↓            ↓           ↓
   DTOs        Modèles     Schémas

Les dépendances vont vers le bas et vers l'intérieur. Les modules de haut niveau dépendent des modules de bas niveau. Un contrôleur peut importer depuis un service, mais un service ne devrait jamais importer depuis un contrôleur.

Le problème

Voici une violation subtile qui passe la plupart des reviews :

// src/services/notification-service.ts
import { OrderController } from '../controllers/order-controller';

export class NotificationService {
  async sendOrderConfirmation(orderId: string) {
    // Réutilisation de la logique de formatage du contrôleur
    const controller = new OrderController();
    const formattedOrder = controller.formatOrderForDisplay(orderId);
    await this.send(formattedOrder);
  }
}

Le développeur avait besoin de la logique de formatage qui se trouvait dans le contrôleur. Au lieu de l'extraire dans un module partagé, il a importé le contrôleur directement dans le service. Le code fonctionne. Les tests passent. La review se concentre sur la logique de notification et passe à côté de la dépendance ascendante.

Maintenant la couche service dépend de la couche contrôleur. Cela crée un cycle : les contrôleurs dépendent des services (normal), et maintenant un service dépend d'un contrôleur (violation). L'ordre de build devient imprévisible. Tester le service en isolation nécessite d'instancier un contrôleur. Refactorer le contrôleur risque de casser le service de notification.

La solution

Les reviewers devraient vérifier la section des imports de chaque fichier modifié — pas seulement le code en dessous. Quand vous voyez un import, demandez-vous : « Ce sens de dépendance est-il correct ? »

Un modèle mental rapide :

• controllers/ peut importer depuis services/, utils/, types/
• services/ peut importer depuis repositories/, utils/, types/
• repositories/ peut importer depuis database/, utils/, types/
• utils/ ne devrait importer que depuis d'autres utils/ ou des packages externes

Si l'import va « vers le haut » dans cette hiérarchie, il doit être signalé. La correction consiste généralement à extraire la logique partagée dans un module de niveau inférieur dont les deux couches peuvent dépendre.

Erreur 3 : Rater les nouvelles dépendances circulaires

Les dépendances circulaires sont l'une des formes les plus destructrices de dette architecturale. Elles rendent le code intestable, cassent les outils de build, provoquent des erreurs runtime et résistent au refactoring. Elles sont aussi étonnamment faciles à introduire et étonnamment difficiles à repérer en review.

Le problème

Une dépendance circulaire apparaît rarement sous la forme directe A-importe-B et B-importe-A dans une seule PR. Plus souvent, elle complète une chaîne déjà partiellement en place :

Avant la PR :
  auth-service.ts → user-service.ts → email-service.ts

La PR ajoute :
  email-service.ts → auth-service.ts  (pour vérifier les permissions avant l'envoi)

Vous avez maintenant un cycle à trois nœuds : auth → user → email → auth. Le reviewer ne voit que le dernier maillon ajouté. Le diff de la PR montre email-service.ts qui importe depuis auth-service.ts, ce qui semble parfaitement raisonnable pris isolément. Personne ne réalise que cela complète un cycle.

Voici à quoi le changement problématique pourrait ressembler :

// src/services/email-service.ts
import { checkPermission } from './auth-service';

export async function sendEmail(userId: string, template: string) {
  const canSend = await checkPermission(userId, 'email:send');
  if (!canSend) throw new Error('Unauthorized');
  // ... envoi de l'email
}

Rien dans ce diff ne crie « danger ». L'import semble légitime. La vérification de permission est une bonne pratique. Mais la dépendance circulaire que cela crée finira par causer des problèmes.

La solution

Détecter les dépendances circulaires manuellement nécessite de connaître le graphe de dépendances complet du projet — ce qui est irréaliste pour toute codebase de taille significative.

La solution pragmatique est l'automatisation. Des outils de linting comme eslint-plugin-import avec la règle no-cycle peuvent détecter les dépendances circulaires au moment du build. Des outils d'analyse d'architecture comme ReposLens peuvent les détecter visuellement sur chaque PR, montrant au reviewer le chemin de dépendance complet plutôt que le seul nouvel import.

Si votre équipe n'a pas de détection automatisée de cycles, établissez au minimum cette habitude de review : quand vous voyez un nouvel import entre deux fichiers de niveau service, tracez la chaîne manuellement. Est-ce que le Service A dépend déjà de ce service via d'autres fichiers ? Si vous n'êtes pas sûr, c'est le signe que vous avez besoin de meilleurs outils.

Erreur 4 : Ne pas vérifier si le changement augmente le couplage

Le couplage mesure à quel point un module connaît un autre module. Un certain couplage est nécessaire — les modules doivent communiquer. Mais le couplage inutile rend le système rigide et fragile.

Le problème

Un pattern courant qui augmente le couplage sans que personne ne le remarque :

// PR : "Ajouter le total de commande au tableau de bord"

// src/components/Dashboard.tsx
import { calculateOrderTotal } from '../../services/order-service';
import { getShippingCost } from '../../services/shipping-service';
import { getTaxRate } from '../../services/tax-service';
import { getDiscount } from '../../services/discount-service';
import { formatCurrency } from '../../utils/format';

export function DashboardOrderSummary({ orderId }: { orderId: string }) {
  const total = calculateOrderTotal(orderId);
  const shipping = getShippingCost(orderId);
  const tax = getTaxRate(orderId);
  const discount = getDiscount(orderId);

  return (
    <div>
      <p>Total : {formatCurrency(total + shipping + tax - discount)}</p>
    </div>
  );
}

Le reviewer voit un composant propre qui affiche un récapitulatif de commande. Le code est lisible, chaque fonction est bien nommée, et le calcul semble correct.

Mais le composant Dashboard dépend maintenant de quatre services distincts. Il connaît les commandes, la livraison, les taxes et les réductions. Tout changement à l'un de ces services risque de casser le tableau de bord. Le composant ne peut pas être testé sans mocker quatre services. Et la logique métier (comment calculer un total final) est embarquée dans un composant UI au lieu de vivre dans la couche service.

La solution

Comptez les imports. Sérieusement. Quand la section d'imports d'un fichier grandit, c'est un signal d'alerte de couplage. Une règle empirique :

• 1-3 imports depuis votre propre codebase : normal
• 4-6 imports : ça mérite d'être questionné — peut-on en consolider certains ?
• 7+ imports : c'est presque certainement un problème de conception

Dans l'exemple ci-dessus, la correction est un seul import :

import { getOrderSummary } from '../../services/order-service';

export function DashboardOrderSummary({ orderId }: { orderId: string }) {
  const summary = getOrderSummary(orderId);
  return <div><p>Total : {summary.formattedTotal}</p></div>;
}

La fonction getOrderSummary encapsule la logique métier. Le composant dépend d'un seul service, pas de quatre. Le couplage est considérablement réduit.

Pendant les reviews, quand vous voyez un fichier importer depuis plusieurs services ou modules, demandez : « Ne faudrait-il pas un intermédiaire qui agrège tout cela ? »

Erreur 5 : Reviewer en isolation sans voir la vue d'ensemble

La dernière erreur, et la plus fondamentale, est de traiter chaque PR comme une unité autonome. Les PR ne sont pas autonomes. Ce sont des changements apportés à un système vivant, et leur impact dépend d'un contexte que le diff seul ne peut pas fournir.

Le problème

Sur six mois, une équipe fait ces changements individuellement raisonnables :

• PR #201 : Ajouter une fonction helper dans shared/utils.ts (approuvée)
• PR #215 : Importer les utils partagés dans le module paiements (approuvée)
• PR #228 : Ajouter des types de paiement dans shared/types.ts (approuvée)
• PR #241 : Importer les types partagés dans le module auth (approuvée)
• PR #256 : Ajouter des helpers auth dans shared/utils.ts (approuvée)
• PR #270 : Importer les utils partagés dans le module notification (approuvée)

Chaque PR est petite, propre et correcte. Mais après six mois, shared/ est devenu un fourre-tout de fonctionnalités sans rapport. Chaque module en dépend. Il ne peut plus être modifié sans risquer de casser l'ensemble de l'application. C'est devenu un « module dieu » — et aucune review individuelle ne l'a créé.

La solution

C'est le problème le plus difficile à résoudre avec des reviews manuelles seules, parce qu'il nécessite de voir des patterns à travers plusieurs PRs sur la durée. Quelques stratégies :

Revues d'architecture périodiques. Une fois par mois ou par trimestre, l'équipe examine le graphe de dépendances complet du projet. Cela attrape l'accumulation progressive de dette que les reviews individuelles de PR manquent.

Propriété des modules. Assignez les modules à des personnes ou des équipes spécifiques. Quand une PR modifie une frontière de module — en ajoutant une dépendance vers ou depuis le module — le propriétaire est ajouté comme reviewer obligatoire.

Vérifications d'architecture automatisées. Des outils qui analysent le graphe de dépendances sur chaque PR peuvent signaler quand un module dépasse un seuil de couplage, quand de nouvelles dépendances inter-modules sont ajoutées, ou quand la complexité architecturale globale augmente. ReposLens fournit ce type d'analyse automatiquement, montrant aux reviewers non seulement ce qui a changé dans le code, mais comment le changement affecte la structure du système.

Architecture Decision Records (ADR). Documentez les frontières architecturales prévues. Quand une PR viole ces frontières, l'ADR fournit une base objective pour contester, au lieu de s'appuyer sur la mémoire individuelle des reviewers quant à ce que l'architecture devrait être.

Construire une meilleure culture de review

Corriger ces cinq erreurs ne nécessite pas une refonte complète de votre processus de review. Cela nécessite d'ajouter une question à chaque review : « Comment ce changement affecte-t-il l'architecture ? »

En pratique, cela signifie :

1. Lisez d'abord les imports. Avant de lire le code, lisez les imports. Ils vous disent quels modules ce fichier touche et si ces dépendances ont du sens.

2. Prenez du recul avant d'approuver. Après avoir reviewé le code, prenez 10 secondes pour réfléchir à l'impact au niveau module. Ce changement ajoute-t-il une dépendance qui n'existait pas avant ? Cette dépendance est-elle appropriée ?

3. Automatisez ce que les humains font mal. Les humains sont excellents pour juger la qualité du code et la logique. Ils sont mauvais pour tracer des graphes de dépendances à travers des centaines de fichiers. Utilisez des outils pour le second.

4. Suivez les métriques d'architecture dans le temps. Nombre de dépendances circulaires, couplage moyen par module, nombre d'imports traversant les frontières. Ces métriques rendent la dette architecturale visible avant qu'elle ne devienne une crise.

5. Faites de l'architecture un critère de review. Ajoutez-la à votre checklist de review au même titre que la correction, les tests et le style. Si votre équipe utilise des labels ou des catégories pour les retours de review, ajoutez « architecture » comme option.

Le coût de détecter un problème d'architecture en code review, c'est une conversation de 5 minutes. Le coût de le corriger six mois plus tard, c'est un effort de refactoring sur plusieurs sprints. Le calcul est simple.