Il y a trois ans, j'ai approuvé une demande de tirage qui a coûté à mon entreprise 47 000 $ de revenus perdus en un seul week-end. Le code semblait correct. Les tests ont réussi. La logique semblait solide. Mais j'ai manqué quelque chose de subtil dans la façon dont nous gérons les conversions de fuseau horaire pour notre système de facturation par abonnement, et les clients dans certaines régions ont été facturés deux fois.
💡 Points Clés
- Les Premières 30 Secondes : Ce Que La Description PR Vous Dit
- La Taille Compte : La Règle des 400 Lignes
- La Couche de Logique Métier : Où se Cachent La Plupart des Bugs
- Gestion des Erreurs : La Différence Entre Bon et Excellent Code
Cet incident a changé ma façon de revoir le code pour toujours. Je suis Sarah Chen, et j'ai été responsable technique senior dans trois entreprises SaaS différentes au cours de la dernière décennie, évaluant en moyenne 15 à 20 demandes de tirage par semaine. Cela représente environ 8 000 PR au cours de ma carrière. J'ai vu du code brillant qui a livré des bugs, et du code désordonné qui a fonctionné à merveille en production pendant des années. J'ai appris que la revue de code ne consiste pas à trouver des erreurs de syntaxe – votre linter s'en occupe. Il s'agit de détecter les problèmes invisibles que seule l'expérience peut révéler.
Cet article est la liste de contrôle que j'aurais aimé avoir lorsque j'ai commencé. Elle n'est pas exhaustive – aucune liste de contrôle ne pourrait l'être – mais elle représente les modèles que j'ai appris à reconnaître, les questions que je m'entraîne à poser, et les modèles mentaux qui ont sauvé mes équipes de nombreux incidents en production.
Les Premières 30 Secondes : Ce Que La Description PR Vous Dit
Avant de regarder une seule ligne de code, je passe 30 secondes à lire la description du PR. Cela peut sembler à l'envers, mais j'ai constaté que 60 % des PR problématiques se révèlent immédiatement par une mauvaise communication. Une bonne description de PR répond à trois questions : qu'est-ce qui a changé, pourquoi cela a changé, et ce qui pourrait mal se passer.
Quand je vois une description qui dit "bug corrigé" ou "composant mis à jour", je sais immédiatement que cette revue prendra plus de temps. L'auteur n'a pas réfléchi aux implications de ses changements, ce qui signifie que je dois faire ce travail pour eux. À l'inverse, lorsque je vois une description qui dit "Refactorisation de l'authentification des utilisateurs pour utiliser des tokens JWT au lieu de cookies de session. Cela réduit la charge sur la base de données de 40 % pendant les heures de pointe mais nécessite que les clients gèrent le rafraîchissement des tokens. Risque : les versions existantes de l'application mobile (< 2.3) devront se réauthentifier," je sais que l'auteur a fait ses devoirs.
Je recherche des métriques spécifiques dans la description. Pas d'améliorations vagues, mais des chiffres réels. "Performance améliorée" ne signifie rien. "Temps de réponse de l'API réduit de 340 ms à 180 ms pour le point de terminaison du profil utilisateur sous 1000 requêtes concurrentes" me dit que l'auteur a mesuré son travail et comprend l'impact. D'après mon expérience, les développeurs qui incluent des métriques dans leurs descriptions de PR écrivent un code plus réfléchi dans l'ensemble.
La description doit également faire référence à un contexte pertinent – le ticket, le document de conception, le fil Slack où l'approche a été discutée. Si j'évalue un PR en isolation, sans comprendre le contexte plus large, je vais manquer des éléments. Une fois, j'ai approuvé une "simple refactorisation" qui a sans le savoir rompu une intégration critique parce que je ne savais pas qu'il y avait une dépendance qui n'était documentée nulle part sauf dans un fil de discussion par email vieux de trois mois.
Les drapeaux rouges dans les descriptions de PR incluent : aucune description, des descriptions plus longues que les changements de code eux-mêmes (indiquant généralement un surdéveloppement), des descriptions disant "WIP" ou "brouillon" mais le PR est marqué comme prêt pour révision, et des descriptions qui incluent des phrases comme "je ne suis pas sûr que ce soit la bonne approche" (alors pourquoi me demandez-vous de le revoir?).
La Taille Compte : La Règle des 400 Lignes
J'ai une règle stricte : je ne passe pas en revue en profondeur les PR plus longues que 400 lignes de changements de code réels (hors code généré, fichiers package-lock et fixtures de test). Ce n'est pas arbitraire. Des recherches de Cisco et SmartBear montrent que l'efficacité de la revue de code chute de manière dramatique après 400 lignes, et ma propre expérience le confirme. Après avoir examiné environ 200 lignes de code, mon cerveau commence à manquer des détails. À 400 lignes, je survole essentiellement les choses.
"La revue de code ne consiste pas à trouver des erreurs de syntaxe – votre linter s'en occupe. Il s'agit de détecter les problèmes invisibles que seule l'expérience peut révéler."
Lorsque quelqu'un soumet un PR de 1 200 lignes, je lui demande de le diviser. Je ne me soucie pas si c'est "tous liés." Les grands PR sont là où se cachent les bugs. J'ai vu de vulnérabilités critiques de sécurité passer inaperçues dans d'immenses PR de refactorisation parce que les examinateurs se sont fatigués et ont cessé de prêter attention autour de la ligne 600. La vulnérabilité était à la ligne 847.
Il y a bien sûr des exceptions. Parfois, vous devez déplacer des fichiers, ou mettre à jour un code généré, ou apporter de grands changements pour répondre à une nouvelle norme de linter. Dans ces cas, je demande à l'auteur de séparer les changements mécaniques des changements logiques. Soumettez les mouvements de fichiers dans un PR, les changements de logique réelle dans un autre. Cela rend les deux PR plus faciles à examiner et plus faciles à annuler si quelque chose ne va pas.
Je fais également attention au ratio de code ajouté par rapport au code supprimé. D'après mon expérience, les meilleurs PR ont un compte de lignes net négatif – ils accomplissent quelque chose tout en réduisant la taille de la base de code. Lorsque je vois un PR qui ajoute 500 lignes et en supprime 50, je deviens méfiant. Ajoutons-nous de la complexité ? Dupliquons-nous des fonctionnalités existantes ? L'auteur sait-il ce qui est déjà dans la base de code ?
La question de la taille s'étend également aux fichiers individuels. Si un PR touche 30 fichiers différents, même si le nombre total de lignes est raisonnable, c'est un drapeau rouge. Cela suggère que le changement est soit mal défini soit que la base de code a des problèmes de couplage. Dans tous les cas, cela mérite une attention supplémentaire.
La Couche de Logique Métier : Où se Cachent La Plupart des Bugs
Après une décennie de revues de code, je peux vous dire où se cachent les bugs : dans la couche de logique métier, spécifiquement dans les cas limites et les transitions d'état. Pas dans les composants UI. Pas dans les requêtes de base de données. Dans le code qui implémente vos règles métier réelles.
| Qualité de la Description PR | Ce Qu'elle Dit | Temps de Revue | Niveau de Risque |
|---|---|---|---|
| Poor | "bug corrigé" ou "composant mis à jour" | 2-3x plus long | Élevé |
| Adequate | Ce qui et pourquoi basiques, aucune analyse de risque | Normal | Moyen |
| Bon | Clair ce qui, pourquoi et risques potentiels identifiés | Efficient | Basse |
| Excellent | Contexte complet, compromis discutés, cas limites notés | Rapide | Très Bas |
Lorsque j'examine la logique métier, je recherche des hypothèses. Chaque hypothèse est un bug potentiel. Une fois, j'ai examiné un code pour un système de calcul de remises qui supposait que toutes les remises étaient des pourcentages. Le code fonctionnait parfaitement jusqu'à ce que le marketing veuille offrir une promotion de "10 $ de réduction". Le système a échoué parce que quelqu'un a divisé par le montant de la remise, et diviser par 10 lorsque vous vous attendiez à un nombre entre 0 et 1 produit des résultats très erronés.
Je me demande : que se passe-t-il aux frontières ? Que se passe-t-il si l'entrée est zéro ? Que se passe-t-il si elle est négative ? Que se passe-t-il si elle est nulle ? Que se passe-t-il si c'est une chaîne vide par rapport à une chaîne nulle ? Que se passe-t-il si le tableau est vide ? Que se passe-t-il si l'utilisateur n'a pas de permissions ? Que se passe-t-il s'il a toutes les permissions ? Que se passe-t-il si la base de données ne renvoie aucun résultat ? Que se passe-t-il si elle renvoie un million de résultats ?
Je cherche les nombres magiques dans la logique métier. Lorsque je vois if (user.loginAttempts > 5), je demande : pourquoi 5 ? Est-ce documenté ? Est-ce configurable ? Que se passe-t-il si nous voulons le changer à 3 pour certains types d'utilisateurs ? Les nombres magiques dans la logique métier sont une dette technique qui attend de se matérialiser.
Les machines à états sont une autre source courante de bugs. Lorsque je vois du code qui gère les transitions d'état — statut de commande, cycle de vie de l'utilisateur, étapes de flux de travail — j'imagine le diagramme d'état dans ma tête. Toutes les transitions sont-elles prises en compte ? Peut-on entrer dans un état invalide ? Une fois, j'ai découvert un bug où un utilisateur pouvait être simultanément "actif" et "suspendu" parce que le code qui définissait un statut ne vérifiait pas l'autre.
🛠 Découvrez Nos Outils
Je fais également attention à la logique métier qui est dispersée sur plusieurs couches. Si je vois une validation dans l'UI, plus de validation dans l'API, et encore plus de validation dans la couche de base de données, je sais que nous allons avoir des incohérences. Les règles métier devraient vivre à un seul endroit, et cet endroit devrait être évident.