Dans [l'épisode précédent](/blog/code-smells-object-orientation-abusers-usage-maladroit-de-l-objet/), j'ai présenté les Object-Orientation Abusers, ces smells où les mécanismes de l'objet sont mal exploités.

Les **Dispensables** forment la dernière famille.
Ce sont des morceaux de code dont on pourrait se passer : si on les retirait, le produit ferait la même chose.

Une ligne inutile ne fait rien, mais elle a un coût : il faut quand même la relire, la tester et la migrer comme les autres.

Pour rappel, un changement coûte cher pour trois raisons, que j'ai détaillées dans [l'épisode 1](/blog/ce-qui-rend-un-changement-couteux/) :
1. Il faut **comprendre** avant de toucher,
2. Il faut toucher à **plusieurs endroits**,
3. On ne sait pas ce qu'on va **casser**.

Un smell est un **symptôme** : il révèle un problème de conception, et il annonce un coût potentiel pour l'une de ces raisons.

Cette famille a une particularité : le remède consiste souvent à **enlever du code** plutôt qu'à en écrire.
Et ça demande parfois plus de courage que d'en ajouter !

## Comments : le déodorant

Le smell **Comments** n'est pas seulement le fait d'avoir un commentaire (encore que...).

C'est le commentaire qui explique ce que fait le code, parce que le code ne le dit pas assez bien.
Martin Fowler et Kent Beck parlent de déodorant : il tente de cacher l'odeur, sans traiter ce qui la provoque.

Les commentaires ne sont pas vérifiables statiquement, par définition. Ni le compilateur ni les tests ne pourront détecter qu'ils ne disent plus la vérité. Il suffit d'un très petit nombre de ces commentaires dans une codebase pour faire perdre la confiance dans l'ensemble des commentaires. 

Quand on en arrive là, on se sent obligé de vérifier que le commentaire et le code sont cohérents. Et là, les commentaires deviennent une source de friction plutôt qu'une aide, carrément contre-productifs pour l'équipe.

J'ai été confronté à ce problème à plusieurs reprises mais un projet me revient particulièrement en tête. Il avait suffi d'une décision très simple pour que tous les commentaires fassent perdre plus de temps qu'ils n'en faisaient gagner : ils avaient été rendus **obligatoires**. (Un jour il faudra qu'on parle de la loi de Goodhart !)

_Un commentaire et un code qui ne disent pas la même chose_

**TypeScript**

```ts
function discountedTotal(customer: Customer, total: number): number {
  // Remise de 5 % pour les clients fidèles
  if (customer.orderCount > 10) {
    return total * 0.93;
  }
  return total;
}
```

**C#**

```csharp compile=comments
public static decimal DiscountedTotal(Customer customer, decimal total)
{
    // Remise de 5 % pour les clients fidèles
    if (customer.OrderCount > 10)
    {
        return total * 0.93m;
    }
    return total;
}
```

Un jour, le métier demande : "on passe la remise fidélité à 10 %".

Tu ouvres le code : 5 % d'un côté, 0,93 de l'autre.
Lequel est le bon ?
Le code s'exécute, le commentaire non, et rien n'oblige l'un à suivre l'autre.

Le premier mauvais remède qui vient en tête : corriger le commentaire.
Ça règle le problème jusqu'au prochain changement.

**Le problème de conception :** une intention qui n'est pas exprimée dans le code, donc écrite à côté, là où rien ne la vérifie.

**Le coût potentiel :** il faut lire le code et le commentaire, puis décider lequel croire. C'est une compréhension difficile, particulièrement dans un contexte où la source de vérité n'est pas facile à identifier.

Le remède : **Extract Method** et un bon nommage, avec une méthode `isLoyal(customer)` et une constante `LOYALTY_DISCOUNT`.
J'ai déjà donné cette astuce dans [l'épisode sur les Bloaters](/blog/code-smells-bloaters-quand-le-code-grossit/) : quand j'ai envie de donner un nom à un bloc de code, c'est souvent le signe qu'il mérite sa propre méthode.

Certes, on entend parfois qu'un commentaire est un aveu d'échec.
Je pense que c'est aller trop loin : le code dit ce qu'il fait, mais il ne dit pas toujours **pourquoi**.

```ts
// Le prestataire répond 500 quand il est saturé : on réessaie au lieu d'abandonner.
```

Ce commentaire-là, aucun nom de méthode ne le remplace facilement.

## Duplicate Code : le sosie

Le **Duplicate Code**, c'est la même connaissance du métier écrite à plusieurs endroits.
Souvent, ça commence par un copier-coller qui rendait service ce jour-là.

L'exemple classique de code qui est dupliqué partout et dont on pense qu'il ne changera jamais, c'est l'âge de majorité. Ou l'âge légal pour conduire, voter, boire de l'alcool, etc.

Apparemment, il y a quelques années, l'âge légal pour conduire a été abaissé à 17 ans. Si la règle métier a été dupliquée, ça peut être très compliqué d'identifier parmi tous les `if(age < 18)` lesquels doivent être modifiés.

Un exemple un peu plus concret, c'est le calcul des pénalités de retard, qui peut être dupliqué dans plusieurs classes comme on va le voir ci-dessous.

_La même règle, écrite deux fois_

**TypeScript**

```ts
class ReminderLetter {
  penalty(invoice: Invoice, today: Date): number {
    const days = daysBetween(invoice.dueDate, today);
    return Math.round((invoice.amount * PENALTY_RATE * days / 365) * 100) / 100;
  }
}

class AccountStatement {
  penalty(invoice: Invoice, today: Date): number {
    const days = daysBetween(invoice.dueDate, today);
    return invoice.amount * PENALTY_RATE * days / 365;
  }
}
```

**C#**

```csharp compile=duplicate
public class ReminderLetter
{
    public decimal Penalty(Invoice invoice, DateOnly today)
    {
        var days = Dates.DaysBetween(invoice.DueDate, today);
        return Math.Round(invoice.Amount * Rates.Penalty * days / 365, 2);
    }
}

public class AccountStatement
{
    public decimal Penalty(Invoice invoice, DateOnly today)
    {
        var days = Dates.DaysBetween(invoice.DueDate, today);
        return invoice.Amount * Rates.Penalty * days / 365;
    }
}
```

Regarde bien : une seule des deux copies arrondit au centime.
Quelqu'un a corrigé un bug dans la première, sans savoir que la seconde existait.

Un jour, la comptabilité demande : "pourquoi le total du relevé ne correspond pas à la somme des relances ?"

Sur une facture, l'écart ne se voit pas.
Sur un relevé de vingt factures, il peut atteindre quelques centimes, et c'est le genre d'écart qu'un service comptable ne laisse pas passer.

**Le problème de conception :** une règle du métier n'a pas d'endroit à elle, donc chaque classe qui en a besoin a sa propre version.

**Le coût potentiel :** chaque évolution de la règle est à reporter dans chaque copie, et les copies divergent sans que rien ne le signale. Ça donne des multiples changements et un risque de régressions.

Le remède : **Extract Method**, puis **Move Method** vers l'objet qui connaît la règle.
Ici, `invoice.latePenalty(today)` donne à la règle un seul endroit.

Avant de factoriser, je te conseille de te poser une question : "si cette règle change, est-ce que les deux copies doivent changer ensemble ?"
Si la réponse est non, ce n'est pas de la duplication, c'est une ressemblance.
J'en ai parlé dans [mon article sur DRY](/blog/dry-mal-applique-piege-du-couplage/) : DRY interdit de dupliquer une connaissance, pas de dupliquer des lignes.

## Dead Code : le zombie

Le **Dead Code**, c'est du code qui ne sert plus, mais qui est toujours là.
Une méthode que plus personne n'appelle, une branche qui ne s'exécute plus, un bloc commenté "au cas où".

J'ai déjà parlé du plus gros fichier sur lequel j'ai eu à travailler. 24 000 lignes, et il y avait des morceaux de code que personne n'osait supprimer, de peur de casser quelque chose. 

Evidemment, parmi les 24 000 lignes, il y avait des morceaux de code qui étaient clairement morts, mais pour lesquels personne ne voulait prendre la responsabilité de les supprimer. 

En pratique, grâce aux outils modernes comme les IDE et les analyseurs statiques, il est plus facile d'identifier le code mort et de le supprimer en toute sécurité. Même quand on n'a pas de tests. 

C'est comme ça que je me suis mis à traquer le code mort dans ce fichier, fonction par fonction, jusqu'à ce que je puisse le supprimer en toute sécurité. Sans surprise, en supprimant une fonction inutilisée, j'ai découvert d'autres fonctions qui ne servaient plus, et le processus s'est enchaîné jusqu'à ce que le fichier soit beaucoup plus propre. 

De mémoire, j'avais supprimé jusqu'à 2000 lignes de code mort en une journée avec cette technique.

C'était le cas facile, la méthode privée inutilisée : ton IDE te la signale.

Le cas qui coûte, c'est celui que le compilateur ne peut pas voir.

_Une branche que plus rien n'active_

**TypeScript**

```ts
function invoiceTotal(invoice: Invoice, flags: Flags): number {
  const gross = invoice.net * (1 + VAT_RATE);

  // Activé le temps de la migration de 2019
  if (flags.legacyRounding) {
    return legacyRound(gross);
  }
  return roundToCents(gross);
}
```

**C#**

```csharp compile=dead
public static decimal InvoiceTotal(Invoice invoice, Flags flags)
{
    var gross = invoice.Net * (1 + Rates.Vat);

    // Activé le temps de la migration de 2019
    if (flags.LegacyRounding)
    {
        return Rounding.Legacy(gross);
    }
    return Rounding.ToCents(gross);
}
```

Le flag vaut `false` dans tous les environnements depuis des années.
Pour le compilateur, les deux branches sont accessibles, et rien ne dit que l'une d'elles est morte.

Un jour, le métier demande : "on arrondit maintenant ligne par ligne".

Te voilà à lire `legacyRound`, à te demander si quelqu'un en dépend encore, et peut-être à l'adapter "pour rester compatible".
Tu viens de payer pour faire évoluer du code qui ne s'exécute pas.

**Le problème de conception :** du code a perdu sa raison d'être, et rien dans le projet ne permet de le savoir.

**Le coût potentiel :** il faut comprendre, maintenir et tester ce code comme s'il servait. C'est une compréhension difficile, pour rien.

Le remède : **Remove Dead Code**.
Et non, le commenter "au cas où" n'est pas un remède. Il y a un endroit spécial pour le code qui a été utile un jour, et qu'on pourrait avoir besoin de ressusciter : l'historique git.

Si tu as un doute, un log ou une métrique sur la branche suspecte, pendant quelques semaines en production, te donnera une réponse basée sur des faits.

## Lazy Class : le figurant

La **Lazy Class**, c'est une classe qui ne rapporte pas ce qu'elle coûte.
Souvent, elle faisait quelque chose avant, et un refactoring l'a vidée. 

Parfois, c'est une classe créée pour un besoin qui n'est jamais arrivé. Le fameux "au cas où". 

_Un fichier, une classe, une ligne de travail_

**TypeScript**

```ts
class InvoiceTitleBuilder {
  build(invoice: Invoice): string {
    return `Facture ${invoice.number}`;
  }
}

const title = new InvoiceTitleBuilder().build(invoice);
```

**C#**

```csharp compile=lazy
public class InvoiceTitleBuilder
{
    public string Build(Invoice invoice) => $"Facture {invoice.Number}";
}

var title = new InvoiceTitleBuilder().Build(invoice);
```

Cette classe n'a pas d'état, pas de variante, et une seule ligne de travail.
Elle n'est pas fausse : elle ajoute juste un nom à connaître et un fichier à ouvrir.

Le mauvais remède qui vient en tête : se dire "tant qu'elle est là, mettons-y aussi le pied de page".
Une classe qui existe déjà attire les nouvelles responsabilités, et c'est comme ça qu'on justifie après coup un découpage qui n'a pas de sens.

**Le problème de conception :** une abstraction qui ne porte plus de décision.

**Le coût potentiel :** un détour de plus à chaque lecture. Pris seul, il est faible. Multiplié par des dizaines de classes, ça devient une compréhension difficile.

Le remède : **Inline Class**.
Ici, `invoice.title()` fait le même travail sans ajouter de fichier.

## Data Class : le sac de champs

La **Data Class**, c'est une classe qui ne contient que des champs, pendant que les règles qui les concernent vivent ailleurs.

Celui-là peut surprendre dans cette famille, parce qu'on ne peut pas supprimer la classe : les données sont utiles.
Ce qui est dispensable, c'est la distance entre les données et les règles.

Un des piliers de la conception orientée objet est que les données et les règles qui les concernent doivent être proches l'une de l'autre. Pendant un temps je l'ai un peu négligé, mais je suis convaincu aujourd'hui que l'encapsulation est le pilier le plus important de la conception orientée objet si on veut que le code reste compréhensible et maintenable.

_Les données ici, les règles là-bas_

**TypeScript**

```ts
class CustomerAccount {
  balance = 0;
  creditLimit = 5000;
}

class OrderService {
  place(account: CustomerAccount, amount: number): void {
    if (account.balance + amount > account.creditLimit) {
      throw new Error('Encours dépassé');
    }
    account.balance += amount;
  }
}

class QuoteService {
  canAccept(account: CustomerAccount, amount: number): boolean {
    return account.balance + amount <= account.creditLimit;
  }
}
```

**C#**

```csharp compile=data
public class CustomerAccount
{
    public decimal Balance { get; set; }
    public decimal CreditLimit { get; set; } = 5000;
}

public class OrderService
{
    public void Place(CustomerAccount account, decimal amount)
    {
        if (account.Balance + amount > account.CreditLimit)
        {
            throw new InvalidOperationException("Encours dépassé");
        }
        account.Balance += amount;
    }
}

public class QuoteService
{
    public bool CanAccept(CustomerAccount account, decimal amount) =>
        account.Balance + amount <= account.CreditLimit;
}
```

Un jour, le métier demande : "les clients premium ont 10 % de marge sur leur encours".

La règle n'est pas dans `CustomerAccount`.
Il faut la chercher dans tous les services qui lisent ses champs, sans liste pour savoir quand on a fini.
Et comme les champs sont publics, rien n'empêche un autre service de modifier le solde sans vérifier l'encours.

**Le problème de conception :** les données et les règles qui les protègent sont séparées, donc l'objet ne peut garantir aucune règle.

**Le coût potentiel :** chaque règle sur ces données peut se retrouver dans plusieurs services. Ça donne des multiples changements et un risque de régressions.

Le remède : **Move Method**, pour rapprocher les règles des données.
`account.canTake(amount)` et `account.charge(amount)` remplacent les champs publics.
C'est le principe [Tell, Don't Ask](/blog/tell-dont-ask-encapsulation-objet/) : plutôt que de demander ses données à un objet pour décider à sa place, on lui dit ce qu'on veut.

## Speculative Generality : le "au cas où"

La **Speculative Generality**, c'est du code écrit pour un besoin qui n'existe pas encore.
Une interface, un paramètre, un mécanisme de plugins, "parce qu'un jour on en aura besoin".

Celui là, je pense que c'est celui dont j'ai été le plus coupable. En particulier au début de ma carrière. J'étais persuadé qu'anticiper l'avenir comme ça était une preuve de prévoyance et de compétence.

C'était avant que je me rende compte du coût réel de cette anticipation : du code inutile, des abstractions compliquées, et des changements à faire à plusieurs endroits quand la réalité évoluait différemment de ce que j'avais prévu. (Et Dieu sait qu'en tant que dev on n'a que très rarement raison, en particulier quand on n'est pas en contact constant avec les utilisateurs de l'outil sur lequel on travaille. )

Prenons un exemple que j'ai vu maintes fois être trop compliqué pour un besoin très simple : une architecture d'export, pour un seul format. J'ai pu voir du excel, du json, du CSV... 

Par simplicité, prenons CSV : 

_Une architecture d'export, pour un seul CSV_

**TypeScript**

```ts
interface Exporter<T> {
  export(rows: T[], options: ExportOptions): string;
}

class ExporterRegistry<T> {
  private exporters = new Map<string, Exporter<T>>();

  register(format: string, exporter: Exporter<T>): void {
    this.exporters.set(format, exporter);
  }

  get(format: string): Exporter<T> | undefined {
    return this.exporters.get(format);
  }
}

// Le seul appel du projet : un seul format, et des options vides
registry.get('csv')?.export(invoices, {});
```

**C#**

```csharp compile=speculative
public interface IExporter<T>
{
    string Export(IEnumerable<T> rows, ExportOptions options);
}

public class ExporterRegistry<T>
{
    private readonly Dictionary<string, IExporter<T>> exporters = new();

    public void Register(string format, IExporter<T> exporter) => exporters[format] = exporter;

    public IExporter<T>? Get(string format) => exporters.GetValueOrDefault(format);
}

// Le seul appel du projet : un seul format, et des options vides
public static string? ExportInvoices(ExporterRegistry<Invoice> registry, List<Invoice> invoices) =>
    registry.Get("csv")?.Export(invoices, new ExportOptions());
```

Un jour, le métier demande : "on veut aussi un export PDF".

C'est le jour que cette architecture attendait, et pourtant elle ne convient pas.
`export` renvoie une `string`, et un PDF n'en est pas une.
Les `rows` à plat ne disent rien de la mise en page, qui est tout le sujet d'un PDF.

L'abstraction a été devinée avec un seul exemple sous les yeux.
Il faut la changer, en plus d'avoir porté son coût depuis le début.

**Le problème de conception :** une abstraction construite à partir d'une hypothèse sur le futur, pas d'un besoin observé.

**Le coût potentiel :** plusieurs niveaux à traverser pour comprendre ce qui se réduit à un CSV, et une abstraction qui peut quand même être à reprendre. C'est une compréhension difficile, et parfois des multiples changements.

Le remède : **Inline Class** et **Collapse Hierarchy**, pour revenir à une fonction `exportInvoicesToCsv(invoices)`.

Le conseil des experts : 
- Sandi Metz : "Duplication is cheaper than the wrong abstraction"
- Martin Fowler : attendre la troisième variante avant de réfléchir à une factorisation. 

Je suis assez d'accord avec lui. Le jour où le troisième format arrive, on aura deux exemples réels pour voir ce qui varie vraiment, et l'abstraction sera bien moins hasardeuse.

L'Extreme Programming a un nom pour cette règle : **YAGNI**, "You Aren't Gonna Need It".

Et parce qu'une image vaut mille mots, je ne résiste pas à te présenter cette illustration qui me vient systématiquement à l'esprit quand j'ai cette pulsion de me dire "et si plus tard ... ":

![YAGNI](/images/yagni-batman.jpg)

## Attention aux faux positifs

- **Deux codes qui se ressemblent par hasard.** S'ils portent deux règles qui évolueront séparément, les fusionner crée un couplage que tu paieras plus tard.
- **Le code public d'une bibliothèque.** Aucun appel dans ton dépôt ne veut pas dire aucun appel tout court.
- **Le petit objet qui porte une règle.** Une classe `Email` de dix lignes n'est pas une Lazy Class : elle garantit qu'une adresse est valide.
- **Les données aux frontières.** Un DTO ou un message d'API n'a pas à contenir de règles. Son rôle est de transporter des données.
- **L'interface à une seule implémentation.** Si elle sert dès aujourd'hui à isoler une dépendance externe dans les tests, ce n'est pas de la spéculation.
- **Le commentaire qui explique le pourquoi.** Une contrainte externe, un texte de loi, un contournement : ce sont des informations que le code ne peut pas porter.

La question à se poser : **"si je supprimais ça demain, qui s'en rendrait compte ?"**.

## Et avec un agent IA ?

Dans [l'épisode 1](/blog/ce-qui-rend-un-changement-couteux/), j'expliquais qu'un agent travaille avec une fenêtre de contexte limitée, et que chaque fichier qu'il ouvre la remplit.
Le code inutile lui coûte donc du contexte, du temps et de l'argent.

Il ne sait pas non plus ce qui est mort.
Face à du Dead Code, il peut le mettre à jour, le tester, ou s'en inspirer pour écrire la suite.

Face à du Duplicate Code, il peut corriger la copie qu'il a ouverte et laisser l'autre telle quelle. Mais si la source de vérité n'est pas clairement définie, gare aux hallucinations ! 

Bien que les modèles aient beaucoup progressé, un commentaire qui ment peut le tromper tout autant qu'un nom qui ment.

Il peut aussi produire lui-même ces smells, si on ne lui demande rien d'autre : une abstraction "au cas où", un commentaire qui paraphrase la ligne d'en dessous.

Pour relire le code d'un agent, "qu'est-ce que je pourrais enlever ?" me semble donc une bonne question de départ.

## En résumé

| Smell | Le problème de conception | Le coût potentiel | Le remède principal |
|---|---|---|---|
| **Duplicate Code** | Une règle du métier sans endroit à elle | Multiples changements, risque de régressions | Extract Method, Move Method |
| **Dead Code** | Du code qui a perdu sa raison d'être | Compréhension difficile | Remove Dead Code |
| **Lazy Class** | Une abstraction qui ne porte plus de décision | Compréhension difficile | Inline Class |
| **Data Class** | Des données séparées de leurs règles | Multiples changements, risque de régressions | Move Method |
| **Speculative Generality** | Une abstraction bâtie sur une hypothèse | Compréhension difficile, multiples changements | Inline Class, Collapse Hierarchy |
| **Comments** | Une intention absente du code | Compréhension difficile | Extract Method, nommage |

Dans [l'épisode 1](/blog/ce-qui-rend-un-changement-couteux/), j'avais rangé toute cette famille du côté de la compréhension difficile.

C'est vrai pour quatre smells sur six.
Duplicate Code et Data Class annoncent surtout des multiples changements.

## La suite

Avec cet épisode, j'ai fait le tour des familles de smells.
Les [Couplers](/blog/code-smells-couplers-detecter-le-couplage/), ces smells qui signalent des classes qui en savent trop les unes sur les autres, je les avais traités avant de commencer cette série.

Dans le prochain épisode, je vais prendre un peu de recul sur tout ça. Même si cette série est terminée, je suis bien conscient que la recette pour écrire du code facile à maintenir ne se résume pas à éviter ces smells. 

J'ai fait le choix de quand même me concentrer sur cet aspect de la maintenabilité car dans mon propre cheminement, j'ai trouvé qu'avoir la capacité à détecter les smells m'a beaucoup aidé à comprendre les problèmes de conception sous-jacents dans le code que j'écrivais.

C'est pour cette raison que j'ai essayé d'insister sur les causes et les conséquences des smells dans cette série d'articles. 

Si ça t'a été utile, je serais très heureux d'avoir ton retour, que ce soit par messagerie, mail ou sur les réseaux sociaux.

Et si tu as réussi à me lire jusque là et que tu trouves qu'il manque quelque chose, je serais tout aussi intéressé d'avoir ton retour à ce sujet ! 

Tu as envie de lire la suite ? Abonne-toi à mon [flux RSS](/rss.xml) pour être tenu au courant en direct de mes publications !

---

*Tu veux creuser ces sujets avec ton équipe ? J'interviens en
[Brown Bag Lunch](/presentations/) dans vos locaux, ou par mail à
[edouard@lacrafterie.tech](mailto:edouard@lacrafterie.tech).*