Ma checklist de code review .NET :
20 points pour livrer
du code sans regret

La code review est l'activité qui a le retour sur investissement le plus élevé dans le développement logiciel — et probablement la moins bien exécutée. Pas parce que les développeurs sont négligents, mais parce qu'une bonne revue de code demande une grille de lecture précise. Sans ça, on survole le code en cherchant vaguement des "problèmes", on commente les choses qui sautent aux yeux (style, nommage), et on laisse passer les vrais bugs.

Voici ma checklist personnelle, construite sur 15 ans de missions .NET. Chaque point a une histoire : un bug en production, une régression en déploiement, une faille de sécurité détectée juste à temps, une performance catastrophique découverte trop tard. Cette liste n'est pas théorique.

Pourquoi la code review change réellement la qualité

Les études sur le sujet sont cohérentes : une revue de code bien conduite détecte en moyenne 60 à 70% des défauts avant qu'ils atteignent la production — bien plus que les tests automatisés seuls. Pas parce que les tests sont insuffisants, mais parce qu'ils ne capturent que ce que le développeur a pensé à tester. La revue apporte un regard extérieur, sans les angles morts du développeur original.

"Une pull request sans revue, c'est du code qui a été écrit par une seule personne, avec ses habitudes, ses angles morts et ses contraintes du moment. La revue n'est pas un contrôle bureaucratique — c'est le moment où le code devient collectif et où les risques se réduisent vraiment."

Mais l'efficacité d'une revue dépend entièrement de l'approche. Passer 4 heures sur une PR de 2000 lignes est un anti-pattern — la capacité d'attention s'épuise, les vrais problèmes passent à travers. Je reviendrai sur la taille idéale d'une PR à la fin.

La checklist en 20 points

Design & Architecture (points 1–5)

1. Responsabilité unique respectée — Chaque classe, méthode ou module fait une seule chose. Si vous avez du mal à nommer une classe sans utiliser "And" ou "Or", c'est souvent le signe qu'elle en fait trop. Une méthode de 150 lignes est presque toujours une méthode qui fait trop de choses.
2. Les abstractions correspondent au niveau de la couche — Un controller ne doit pas contenir de logique métier. Un handler d'application ne doit pas interroger directement la base de données. Une entité de domaine ne doit pas avoir de dépendance sur l'infrastructure. Les violations de ces frontières créent du couplage invisible.
3. Les dépendances vont dans le bon sens — En Clean Architecture : domaine ← application ← infrastructure. Si vous voyez un projet Application qui référence un projet Infrastructure, c'est une inversion de dépendance à corriger. Les tests doivent en être la preuve structurelle.
4. Le pattern utilisé est adapté au problème — CQRS sur un CRUD simple, Saga sur une transaction qui n'a pas besoin d'être distribuée, Event Sourcing parce que "c'est cool" — l'over-engineering a un coût réel. Vérifiez que la complexité introduite est justifiée par un besoin concret.
5. Les changements sont rétrocompatibles avec les contrats existants — Modifier une signature d'API publique, changer le format d'un événement de domaine, supprimer un champ d'un DTO — ces changements doivent être identifiés explicitement dans la PR et versionnés si nécessaire.

Correctness & Robustesse (points 6–10)

6. Les cas null et les cas limites sont gérés — Toute valeur qui arrive de l'extérieur (requête HTTP, message queue, base de données) peut être null, vide, ou hors des valeurs attendues. Les guard clauses en entrée de méthode ne sont pas du code défensif — c'est de la correctness basique.
7. Les exceptions sont les bonnes au bon niveau — Une exception de domaine (CustomerNotFoundException) ne doit pas se propager jusqu'au controller sans être traduite. Un ArgumentNullException ne doit pas être catchée et ignorée. Le traitement des exceptions doit être explicite et intentionnel, pas accidentel.
8. Les opérations async sont correctement attendues — async void (hors event handlers) est presque toujours un bug. Un appel async non attendu est une course condition silencieuse. Vérifiez que chaque méthode async est attendue, et que les exceptions sont propagées correctement.
9. Les ressources sont correctement libérées — Tout ce qui implémente IDisposable doit être dans un bloc using ou libéré explicitement. Les connexions de base de données, les streams, les clients HTTP — une fuite de ressource ne se manifeste pas immédiatement, elle accumule jusqu'à la rupture.
10. La concurrence est gérée correctement — Les propriétés statiques mutables, les caches sans synchronisation, les accès concurrents à des collections non thread-safe — ces problèmes sont les plus difficiles à reproduire et les plus coûteux à déboguer en production.

Performance (points 11–13)

11. Pas de requêtes N+1 EF Core non identifiées — Toute relation accédée en boucle sans Include préalable est une N+1 potentielle. Sur des listes de 1000 éléments, ça peut générer des milliers de requêtes SQL. Les revues de code sont le bon moment pour détecter ça — avant que le DBA vous appelle en urgence.
12. AsNoTracking() présent sur toutes les queries en lecture — Sans tracking, EF Core ne maintient pas le change tracker en mémoire. Sur des endpoints sous charge, c'est une réduction significative des allocations. L'absence d'AsNoTracking() sur une query de liste est presque toujours un oubli.
13. Pas d'appels HTTP ou I/O dans des boucles — Un appel HTTP par élément d'une liste, une lecture de fichier répétée dans une boucle — ce sont des patterns qui dégradent les performances de façon non linéaire. Si vous voyez un foreach avec un await à l'intérieur, vérifiez qu'il ne s'agit pas d'un appel réseau qui pourrait être batché.

Sécurité (points 14–15)

14. Les données entrantes sont validées et sanitisées — Toute donnée qui vient de l'extérieur (requête HTTP, message, fichier) doit être validée avant d'être utilisée. FluentValidation en entrée de commande, pas de concaténation de chaînes dans des requêtes SQL, pas de chemin de fichier construit depuis l'entrée utilisateur sans nettoyage.
15. Les autorisations sont vérifiées au bon niveau — Un endpoint qui lit des données appartenant à un utilisateur doit vérifier que l'utilisateur courant a le droit de les lire — pas seulement qu'il est authentifié. Les bugs d'autorisation (IDOR — Insecure Direct Object Reference) sont parmi les plus fréquents et les plus graves.

Tests (points 16–18)

16. Les tests couvrent les cas limites, pas seulement le happy path — Un test qui ne vérifie que le cas nominal donne une fausse confiance. Vérifiez que les cas d'erreur (entité inexistante, validation échouée, dépendance indisponible), les cas limites (liste vide, valeur à la frontière) et les cas d'invalidité sont couverts.
17. Les tests sont indépendants et déterministes — Un test qui dépend de l'ordre d'exécution des autres tests, qui utilise DateTime.Now directement, ou qui dépend d'une donnée fixe en base partagée est un test fragile. Un test qui échoue aléatoirement est pire qu'aucun test — il érode la confiance dans la suite entière.
18. Les mocks sont utilisés au bon niveau — Mocker l'infrastructure (repositories, clients HTTP) dans les tests unitaires : oui. Mocker les entités de domaine ou la logique métier : non — vous testez alors le mock, pas le code. Si vous avez besoin de mocker beaucoup de choses pour tester une unité, c'est souvent le signe que l'unité est trop couplée.

Nommage & Lisibilité (points 19–20)

19. Les noms révèlent l'intention, pas l'implémentation — ProcessData() ne dit rien. CalculateMonthlyInvoice() dit tout. Un bon nom rend le commentaire inutile. Si vous devez ajouter un commentaire pour expliquer ce que fait une méthode, c'est souvent le signe que le nom peut être amélioré.
20. Le code reflète le langage du domaine métier — Les noms de classes, méthodes et variables doivent correspondre au vocabulaire utilisé par les experts métier (le "ubiquitous language" du DDD). Un InvoiceGenerationService qui s'appelle BillingHelper crée une friction entre le code et la réalité qu'il modélise. Cette friction s'accumule.

Comment donner et recevoir du feedback

La technique de revue ne représente que la moitié du travail. L'autre moitié est humaine.

Pour donner du feedback : commencez par les points positifs — un code review n'est pas qu'une liste de défauts. Distinguez clairement ce qui bloque l'approbation (must fix) de ce qui est une suggestion (nit:). Proposez toujours une alternative concrète au lieu de simplement signaler un problème. Et commentez le code, jamais la personne — jamais "tu as fait ça", toujours "ce pattern peut causer X parce que Y".

Pour recevoir du feedback : résistez à l'instinct défensif. Une observation sur votre code n'est pas un jugement sur votre valeur. Les reviewers ont des angles de vue que vous n'avez pas. Quand vous ne comprenez pas un commentaire, demandez des clarifications avant de le rejeter. Et si vous n'êtes pas d'accord, argumentez avec des faits — pas des préférences.

"La meilleure revue de code que j'ai reçue en carrière était la plus difficile à lire. Le reviewer avait identifié une faille de sécurité dans ma logique d'autorisation que j'aurais mise en production. Ce jour-là, j'ai compris que la revue de code est un acte de bienveillance, pas de critique."

Taille de PR et revue automatisée vs manuelle

La règle empirique que j'applique : une PR ne devrait pas dépasser 400 lignes nettes modifiées (hors fichiers générés, migrations, fichiers de ressources). Au-delà, la qualité de la revue chute. Le reviewer ne peut pas maintenir le contexte global sur un si grand changement — les problèmes d'architecture passent à travers.

Si votre feature est plus grande, découpez-la :

• Étape 1 : infrastructure et interfaces (souvent la PR la plus simple à reviewer)
• Étape 2 : implémentation du domaine et des handlers
• Étape 3 : tests d'intégration et endpoints

Sur la frontière automatisé/manuel : les outils (SonarQube, Roslyn analyzers, dotnet format) gèrent le style, la complexité cyclomatique, les patterns d'anti-patterne connus. Configurez-les dans votre CI pour qu'ils bloquent avant même que la revue humaine commence. Votre reviewer humain ne devrait jamais commenter un problème de formatting — c'est du temps gaspillé de la part des deux personnes.

Le tableau de bord que j'utilise sur mes missions :

# CI obligatoire avant revue humaine
dotnet format --verify-no-changes    # Style — bloquant
dotnet build --no-incremental        # Compilation — bloquant
dotnet test --no-build               # Tests — bloquant
# SonarQube analysis               # Qualité — bloquant sur critiques

Avec ce pipeline, la revue humaine peut se concentrer sur ce que les outils ne voient pas : la justesse du design, la pertinence des abstractions, les risques métier, la sécurité contextuelle. C'est là que la valeur est réelle.

Intégrer la checklist dans votre workflow

Cette checklist de 20 points peut sembler intimidante. En pratique, avec l'habitude, elle prend 5 à 10 minutes sur une PR de taille normale — la plupart des points se vérifient en un coup d'œil une fois que vous avez l'œil formé. Je la parcours dans l'ordre : design d'abord (si le design est mauvais, les autres points ont moins d'importance), puis correctness, puis performance, puis sécurité, puis tests, puis nommage.

Le plus important n'est pas de cocher tous les points à chaque fois, mais d'avoir un cadre qui empêche les angles morts. Les bugs de production les plus coûteux que j'ai rencontrés en 15 ans étaient presque toujours dans l'une de ces 20 catégories — jamais dans un endroit inattendu.