Les Object-Orientation Abusers : ces code smells qui signalent un usage maladroit de l'objet

Dans l’épisode précédent, j’ai présenté les Change Preventers, ces smells qu’on voit au moment de modifier le code : une petite demande, et des dizaines de fichiers à toucher.

Les Object-Orientation Abusers forment la quatrième famille. Le nom est barbare, mais l’idée est simple : un langage orienté objet offre des mécanismes (le polymorphisme, l’héritage, l’encapsulation). Les smells apparaissent quand on les utilise de travers, ou on ne les utilise pas là où ils seraient utiles.

Pour rappel, un changement coûte cher pour trois raisons, que j’ai détaillées dans l’épisode 1 :

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.

Dans cette famille, trois smells sur quatre ont souvent un point commun : ajouter un cas oblige à rouvrir du code qui marchait. Le quatrième est plus sournois, parce qu’on ne le voit dans aucun fichier.

Switch Statements : le switch qui se répète

Le Switch Statements n’est pas le switch en lui-même. C’est le même switch (ou la même suite de if) recopié à plusieurs endroits, tous sur la même valeur.

Le pire exemple que j’ai rencontré était dans un moteur d’impressions où chaque partie d’un document commençait par un switch sur le type du document.

Au début, ça semblait une bonne idée : tous les documents n’ont pas besoin des mêmes parties, et ce type de décision semblait logique à l’endroit où elle était prise.

Mais à la fin, l’énum des types de document comptait près de 2000 cas possibles. Chaque nouvelle partie ou chaque nouveau type obligeait à modifier le même point de décision, dans plusieurs endroits, avec la même logique à répliquer.

On sentait bien le symptôme du Switch Statements : le code fonctionnait, mais l’ajout d’un cas devenait une opération à haut risque, où le vrai changement n’était plus dans le métier, mais dans les vastes switch qui permettaient de remplir les documents.

Voici une version simplifiée de ce moteur : trois types de document, deux parties, et une valeur par défaut qui cache un oubli.

type DocumentType = 'invoice' | 'quote' | 'deliveryNote';

function renderHeader(type: DocumentType, doc: PrintableDocument): string {
  switch (type) {
    case 'invoice':
      return `FACTURE ${doc.number}`;
    case 'quote':
      return `DEVIS ${doc.number}, valable 30 jours`;
    case 'deliveryNote':
      return `BON DE LIVRAISON ${doc.number}`;
  }
}

function renderFooter(type: DocumentType): string {
  switch (type) {
    case 'invoice':
      return 'Paiement à 30 jours';
    case 'quote':
      return 'Bon pour accord';
    default:
      return '';
  }
}

public enum DocumentType { Invoice, Quote, DeliveryNote }

public static string RenderHeader(DocumentType type, PrintableDocument doc) => type switch
{
    DocumentType.Invoice => $"FACTURE {doc.Number}",
    DocumentType.Quote => $"DEVIS {doc.Number}, valable 30 jours",
    DocumentType.DeliveryNote => $"BON DE LIVRAISON {doc.Number}",
    _ => throw new ArgumentOutOfRangeException(nameof(type)),
};

public static string RenderFooter(DocumentType type) => type switch
{
    DocumentType.Invoice => "Paiement à 30 jours",
    DocumentType.Quote => "Bon pour accord",
    _ => "",
};

Un jour, le métier demande : “on ajoute les avoirs”.

Tu ajoutes un cas à DocumentType, et tu cherches les switch à compléter. En TypeScript, le compilateur t’en signale un : renderHeader ne renvoie plus rien dans tous les cas. Mais renderFooter compile sans broncher, grâce à son default, et l’avoir sort sans pied de page, sans que personne l’ait décidé. En C#, le _ => fait taire le compilateur dans les deux méthodes : l’oubli se découvre à l’exécution, quand quelqu’un imprime un avoir.

Le problème de conception : un concept du métier (le type de document) est représenté par une simple valeur, et ses comportements sont répartis dans des switch au lieu d’appartenir au concept.

Le coût potentiel : chaque nouveau cas oblige à modifier du code “battle tested” en production, à chaque endroit, sans garantie de les avoir tous trouvés. Ceci vient avec un risque de régressions, et des multiples changements.

Le remède s’appelle Replace Conditional with Polymorphism : chaque cas devient une classe qui porte ses comportements.

En d’autres termes, il faut créer une classe pour chaque type de document, et y placer ses comportements spécifiques. C’est l’alliance des patterns Factory et Strategy. Au lieu d’avoir des classes qui connaissent comment faire une partie de document pour tout type de document, on a des classes qui ont la responsabilité d’éditer un seul type de document dans son intégralité.

interface DocumentType {
  header(doc: PrintableDocument): string;
  footer(): string;
}

class CreditNote implements DocumentType {
  header(doc: PrintableDocument): string {
    return `AVOIR ${doc.number}`;
  }

  footer(): string {
    return 'À déduire de la prochaine facture';
  }
}

public interface IDocumentType
{
    string Header(PrintableDocument doc);
    string Footer();
}

public class CreditNote : IDocumentType
{
    public string Header(PrintableDocument doc) => $"AVOIR {doc.Number}";
    public string Footer() => "À déduire de la prochaine facture";
}

Le compilateur te force maintenant à écrire les deux comportements, et renderHeader ne s’ouvre plus à chaque nouveau type de document. Il reste un switch quelque part : celui qui transforme la valeur lue en base ("creditNote") en objet.

Un seul, à la frontière, et ça me va comme ça.

Refused Bequest : l’héritier qui refuse l’héritage

Le Refused Bequest, c’est une sous-classe qui hérite de méthodes qu’elle ne veut pas, ou ne peut pas, assumer. Elle les redéfinit pour lever une exception, ou pour ne rien faire.

class Invoice {
  protected lines: InvoiceLine[] = [];

  addLine(line: InvoiceLine): void {
    this.lines.push(line);
  }

  total(): number {
    return this.lines.reduce((sum, l) => sum + l.price * l.quantity, 0);
  }
}

class ArchivedInvoice extends Invoice {
  addLine(): never {
    throw new Error('Une facture archivée ne se modifie pas');
  }
}

function addShippingFee(invoices: Invoice[], fee: InvoiceLine): void {
  for (const invoice of invoices) {
    invoice.addLine(fee);
  }
}

public class Invoice
{
    protected readonly List<InvoiceLine> lines = new();

    public virtual void AddLine(InvoiceLine line) => lines.Add(line);

    public decimal Total() => lines.Sum(l => l.Price * l.Quantity);
}

public class ArchivedInvoice : Invoice
{
    public override void AddLine(InvoiceLine line) =>
        throw new InvalidOperationException("Une facture archivée ne se modifie pas");
}

public static void AddShippingFee(IEnumerable<Invoice> invoices, InvoiceLine fee)
{
    foreach (var invoice in invoices)
    {
        invoice.AddLine(fee);
    }
}

Le métier demande : “on ajoute des frais de port sur toutes les factures du mois”.

Le code compile, les tests passent sur des factures neuves, et la première facture archivée de la liste fait tomber tout le traitement. ArchivedInvoice a bien le type Invoice, donc le compilateur la laisse passer, mais elle ne se comporte pas comme une Invoice.

Le problème de conception : l’héritage a servi à réutiliser du code (le calcul du total), pas à exprimer un vrai “est un”. Une sous-classe doit pouvoir remplacer sa classe mère partout, c’est le principe de substitution de Liskov, et ici elle ne le peut pas.

Le coût potentiel : le code qui manipule des Invoice ne peut plus se fier à leur type, et il faut connaître toutes les sous-classes pour savoir ce qui peut casser. C’est le risque de régressions, et une compréhension plus difficile.

Le remède : séparer ce qui se lit de ce qui se modifie. Avec deux types, ReadableInvoice (qui sait donner son total) et EditableInvoice (qui sait aussi ajouter une ligne), la facture archivée n’implémente que le premier. Passer une facture archivée à addShippingFee ne compile alors plus : l’erreur arrive chez toi, pas chez le client.

Temporary Field : le champ fantôme

Le Temporary Field, c’est un champ de la classe qui n’a de valeur que dans certains cas. Le reste du temps, il est vide, et le lecteur doit deviner quand il peut s’y fier.

C’est un smell que j’ai rencontré plusieurs fois dans des projets où l’état d’un objet déterminait quels champs étaient valides. Je l’ai observé quasi systématiquement dans les projets où la conception du modèle se faisait en premier lieu par la base de données.

Dans ma toute première mission en tant que développeur, la plupart des tables avaient tous leurs champs en nullable, avec des groupes de valeurs quasi systématiquement null et il fallait deviner à partir de l’état de l’objet quels champs étaient effectivement renseignés.

Evidemment, cela rend le code plus fragile et plus difficile à comprendre, et ça oblige à mettre du code défensif partout, car il faut constamment vérifier l’état de l’objet avant d’accéder à certains champs.

class Order {
  status: 'pending' | 'shipped' | 'cancelled' = 'pending';
  trackingNumber?: string; // renseigné seulement si status === 'shipped'
  cancelReason?: string; // renseigné seulement si status === 'cancelled'

  trackingUrl(): string {
    return `https://suivi.example.com/${this.trackingNumber}`;
  }
}

public class Order
{
    public string Status { get; set; } = "pending";
    public string? TrackingNumber { get; set; } // seulement si "shipped"
    public string? CancelReason { get; set; } // seulement si "cancelled"

    public string TrackingUrl() => $"https://suivi.example.com/{TrackingNumber}";
}

Le métier demande : “affiche le lien de suivi sur toutes les commandes”.

Sur une commande en attente, trackingUrl() ne plante pas. Elle renvoie https://suivi.example.com/undefined en TypeScript, et un lien amputé de son numéro en C#. Le compilateur n’y voit rien à redire.

Le problème de conception : plusieurs états différents partagent la même classe, alors que chacun a ses propres données. La règle “ce champ n’a de sens que si le statut vaut shipped” n’existe que dans un commentaire.

Le coût potentiel : il faut comprendre quels champs vont avec quel état avant de toucher à quoi que ce soit, et rien n’empêche de lire un champ au mauvais moment. C’est une compréhension difficile, et un risque de régressions.

Le remède : un type par état. Une ShippedOrder possède un numéro de suivi qui existe forcément, et une PendingOrder n’a pas de champ à deviner. J’ai détaillé cette technique, appelée typestate, dans l’article Typestate : laisse le compilateur interdire tes états invalides.

Il existe une autre forme de ce smell : des champs qu’une seule méthode renseigne pour que ses méthodes privées les relisent, comme des paramètres cachés. Dans ce cas, le remède est plus simple : passer ces valeurs en paramètres.

Alternative Classes with Different Interfaces : les jumeaux qui ne se parlent pas

Les Alternative Classes with Different Interfaces, ce sont deux classes qui rendent le même service, avec des noms de méthodes et des paramètres différents.

Contrairement aux autres smells de cette famille, tu ne le vois dans aucun fichier. Chaque classe, prise seule, est correcte. Il faut avoir lu les deux pour s’apercevoir qu’elles font le même travail.

Quand je travaillais au sein d’un département R&D, sur un produit qui avait plus de vingt ans, il est arrivé plusieurs fois qu’on développe un service pendant tout un sprint. Et à la review, quelqu’un qui était là depuis longtemps nous expliquait qu’on venait de développer quelque chose déjà couvert à 80 % par un service existant. Il y avait tellement de services différents qu’on n’avait aucun moyen simple de savoir ce qui existait déjà.

Voici une version miniature de cette situation.

class ReminderMailer {
  constructor(private smtp: Smtp) {}

  sendReminder(customer: Customer, order: Order): void {
    this.smtp.send(customer.email, 'Paiement en retard', `Commande ${order.reference} à régler`);
  }
}

class OverdueNotifier {
  constructor(private smtp: Smtp, private customers: CustomerRepository) {}

  notifyLate(customerId: string, amount: number, daysLate: number): boolean {
    const customer = this.customers.find(customerId);
    return this.smtp.send(customer.email, 'Retard de paiement', `${amount} € dus depuis ${daysLate} jours`);
  }
}

public class ReminderMailer(Smtp smtp)
{
    public void SendReminder(Customer customer, Order order) =>
        smtp.Send(customer.Email, "Paiement en retard", $"Commande {order.Reference} à régler");
}

public class OverdueNotifier(Smtp smtp, CustomerRepository customers)
{
    public bool NotifyLate(string customerId, decimal amount, int daysLate)
    {
        var customer = customers.Find(customerId);
        return smtp.Send(customer.Email, "Retard de paiement", $"{amount} € dus depuis {daysLate} jours");
    }
}

Le métier demande : “on relance aussi par SMS”.

Où faut-il l’ajouter ? Dans l’une, dans l’autre, dans les deux ? Et si un écran appelle ReminderMailer et une tâche planifiée OverdueNotifier, la relance par SMS n’arrive que d’un côté.

Le premier mauvais remède qui vient en tête : écrire une troisième classe, une façade qui appelle les deux. Ça ajoute un service de plus que personne ne connaît, exactement comme dans mon histoire de sprint.

Le problème de conception : une même responsabilité (relancer un client) existe en double, et rien dans le code ne le dit. Le vocabulaire (reminder, notifyLate) n’est pas le même d’une classe à l’autre.

Le coût potentiel : on développe ce qui existe déjà, on corrige un bug à un endroit et pas à l’autre, et on ne sait pas quoi supprimer. C’est une compréhension difficile avant d’écrire la moindre ligne, puis des multiples changements.

Le remède : choisir une des deux interfaces, rapprocher l’autre avec Rename Method et Move Method, puis supprimer la classe devenue inutile. Pour ne pas retomber dans la même situation, le nom du service doit dire ce qu’il fait pour le métier : PaymentReminder se trouve en cherchant “relance”, OverdueNotifier beaucoup moins.

Attention aux faux positifs

• Un seul switch sur une valeur. À la frontière du système, pour transformer une chaîne reçue en objet, c’est normal. Le smell apparaît quand un switch sur la même valeur se répète.
• Un ensemble de valeurs fermé. Les jours de la semaine ne gagneront pas de nouveau cas. Un switch dessus ne coûtera rien.
• Une sous-classe qui n’utilise pas tout. Un Refused Bequest se reconnaît à ce que la sous-classe refuse : exception, méthode vide, comportement contraire. Ne pas utiliser une méthode héritée n’est pas un problème.
• Un champ vraiment facultatif. Un deuxième prénom, un numéro de téléphone : le champ est vide parce que l’information n’existe pas, pas parce que l’état est différent.
• Deux classes semblables dans deux contextes. Un Customer côté facturation et un Customer côté support ne représentent pas la même chose. Les fusionner créerait un autre problème.

La question à se poser : “si j’ajoute un cas, est-ce que je dois rouvrir du code qui marche, ou retrouver un code que je ne connais pas ?”.

Et avec un agent IA ?

J’ai expliqué dans l’épisode 1 qu’un agent travaille avec ce qu’il a lu. Ce qui n’est pas dans son contexte, pour lui, n’existe pas.

Face à un switch, il ajoute souvent le cas à l’endroit qu’il vient d’ouvrir. Les autres switch du projet, il ne les connaît pas forcément, surtout derrière un default.

Face à Alternative Classes, il fait ce que mon équipe faisait pendant ses sprints : il écrit le service demandé, sans savoir qu’un équivalent existe sous un autre nom. Et il le fait en quelques minutes plutôt qu’en un sprint, donc le nombre de doublons peut croître plus vite qu’avant.

Un type par état, une interface commune et un bon nommage sont très utiles pour donner à l’agent ce dont il a besoin. Surtout, le compilateur proposera une feedback loop très rapide et explicite.

En résumé

La suite

Dans le prochain épisode, je vais parler de la dernière famille de smells : dispensables. Le code dont on pourrait se passer.

Tu as envie de lire la suite ? Abonne-toi à mon flux RSS pour être tenu au courant en direct de mes publications !

Tu veux creuser ces sujets avec ton équipe ? J’interviens en Brown Bag Lunch dans vos locaux, ou par mail à edouard@lacrafterie.tech.

Version markdown de cet article