tunahanaliozturk/secure-dotnet-skills

Revue de conception SOLID

Ordonne à l'agent de parcourir une classe ou un module C# à travers chaque principe SOLID en utilisant des lentilles concrètes propres à .NET, en nommant le code smell, le principe violé et le refactoring minimal — en priorisant les violations qui causent un couplage ou une maintenance réelle, et non des points techniques théoriques.

Quand l'utiliser

• Une classe devient difficile à manier — elle mêle E/S, logique métier, mapping et effets de bord dans un seul fichier.
• Une PR introduit un nouveau variant de type et nécessite la modification d'une chaîne switch ou if-else existante.
• Les constructeurs instancient (new) leurs propres dépendances ou référencent directement DateTime.Now / des helpers static.
• Une interface a accumulé des membres pour satisfaire plusieurs appelants non liés, forçant des implémentations vides.

Processus

1. Identifier la classe/module sous revue. Nommer sa responsabilité déclarée. Si le nom nécessite un « et » ou « ou » pour le décrire, c'est déjà un signal.
2. Parcourir chaque principe SOLID dans l'ordre en utilisant les lentilles C# ci-dessous. Traiter chaque principe comme un passage séparé ; ne pas s'arrêter après la première constatation.
3. Nommer le code smell et le principe violé. « Constructeur volumineux assemblant son propre HttpClient et SqlConnection » → DIP ; « substitution de base levant NotSupportedException » → LSP. Être précis.
4. Proposer le refactoring minimal avec le mécanisme C#. Nommer le type, le pattern ou le changement d'enregistrement DI — pas simplement « appliquer le principe ». Un conseil générique « suivez SOLID » n'est pas une constatation.
5. Prioriser en fonction de l'impact réel. Un service monolithique qui mélange la persistance EF avec l'envoi d'e-mails cause une maintenance à chaque sprint ; un IOrderService avec 12 membres qu'aucun appelant n'utilise complètement est une réelle violation ISP. Une classe sealed qui satisfait par hasard deux cas d'utilisation faiblement liés n'est pas une violation SRP si elle n'a qu'une seule raison de changer. Distinguer les violations qui nuisent de celles qui sont simplement impures.

Indications .NET / Azure

SRP — Principe de responsabilité unique

• Services / classes monolithiques. Une classe unique qui touche à EF Core (_context.SaveChangesAsync), envoie des e-mails (_emailSender.SendAsync), valide des règles métier et mappe des DTOs a plusieurs raisons de changer. Extraire un IOrderValidator, un IOrderRepository (ou utiliser le DbContext directement dans une classe de persistance dédiée) et un IOrderNotifier comme collaborateurs séparés.
• Contrôleurs volumineux. Un contrôleur ASP.NET Core qui contient des if/logique de validation, appelle directement la base de données et formate les payloads de réponse fait trois tâches. Les contrôleurs doivent orchestrer : appeler un service, mapper le résultat, retourner un IActionResult. Les règles métier appartiennent à la couche application/domaine.
• « Et » dans le nom du type ou de la responsabilité. OrderValidationAndPersistenceService est un code smell de nom qui signale une violation SRP. Si vous ne pouvez décrire la classe qu'avec un « et », divisez-la.
• Plusieurs raisons de changer. Demander : « Cette classe devrait-elle changer si nous changions de fournisseur d'e-mails ? Si nous changions le schéma de base de données ? Si les règles métier changeaient ? » Plus d'un « oui » signifie plus d'une responsabilité.

OCP — Principe ouvert/fermé

• switch / if-else croissant sur un discriminateur de type. Un bloc switch(order.Type) dans un service qui doit être rouvert pour chaque nouvelle valeur d'énumération OrderType viole OCP. Remplacer par une stratégie : définir IOrderHandler (ou IOrderProcessor) et enregistrer une implémentation par type ; résoudre via IEnumerable<IOrderHandler> injecté dans le coordinateur, en dispatchant sur handler.Handles(order.Type).
• Extension de comportement codée en dur. Si l'ajout d'une nouvelle règle de remise, d'un canal de notification ou d'un format d'exportation nécessite de modifier une classe existante, plutôt que d'ajouter un nouveau IDiscountRule ou IExporter, la classe n'est pas fermée à la modification. Introduire l'abstraction avant que le deuxième cas n'arrive.
• Anti-patron de discriminateur de type. if (order is PremiumOrder) ou if (typeof(T) == typeof(FooOrder)) à l'intérieur d'une classe de traitement est un code smell équivalent au switch ci-dessus. Le polymorphisme ou un registre de stratégies élimine ces cas.

LSP — Principe de substitution de Liskov

• Substitutions qui throw new NotSupportedException() / NotImplementedException(). Un sous-type qui ne peut pas honorer un contrat de base (par exemple, ReadOnlyCollection<T> levant une exception sur Add) viole LSP. Si la substitution ne peut pas être implémentée de manière significative, la hiérarchie est erronée — préférer la composition ou des interfaces séparées.
• Sous-types affaiblissant les postconditions. Si la classe de base garantit que GetOrders() ne retourne jamais null, une substitution retournant null viole le contrat sur lequel les appelants comptent. Documenter les invariants (XML doc ou contrats) et les appliquer dans les substitutions.
• Sous-types renforçant les préconditions. Une substitution qui ajoute une vérification null absente de la base, ou qui rejette une plage de paramètres valide acceptée par la base, casse le code qui transmet une entrée valide selon le contrat de base. Le sous-type doit accepter tout ce que la base acceptait.
• Downcasts is/as omniprésents dans le code appelant. if (handler is SpecialHandler sh) sh.SpecialMethod() chez l'appelant signifie que l'abstraction est brisée — soit la méthode appartient à l'interface de base, soit les types ne devraient pas partager une hiérarchie. Les downcasts fréquents sont un signal d'échec LSP / d'abstraction.

ISP — Principe de ségrégation des interfaces

• Interfaces volumineuses (10+ membres) avec des préoccupations non liées. Un IOrderService exposant CreateOrder, CancelOrder, GetInvoice, SendConfirmation, ExportToCsv, CalculateTax, ValidateAddress et GetShippingRates force chaque implémenteur et double de test à implémenter des méthodes qu'il n'utilise pas. Scinder en interfaces de rôle : IOrderWriter, IOrderReader, IOrderNotifier, IOrderExporter.
• Implémentations vides / stubs NotImplementedException. Si un mock ou une implémentation de test retourne throw new NotImplementedException() pour la moitié des membres de l'interface, ces membres appartiennent à une interface de rôle différente. La présence d'implémentations vides est une preuve directe de violation ISP.
• Clients important des méthodes qu'ils n'appellent jamais. Si PaymentService dépend de IOrderService mais n'appelle que GetOrder, il devrait dépendre de IOrderReader. Réduire la dépendance rend le couplage explicite et les tests moins coûteux.

DIP — Principe d'inversion des dépendances

• new de dépendances concrètes à l'intérieur d'une classe. La construction de new HttpClient() à l'intérieur d'un service crée une dépendance cachée, contourne le conteneur DI, fuit les sockets (pas de pooling de connexions) et rend la classe non testable sans astuces de réflexion. Injecter IHttpClientFactory (client typé) ou un client nommé via le constructeur. Même motif pour new SqlConnection(connectionString) — injecter IDbConnectionFactory ou laisser EF Core / Dapper gérer la connexion via le DbContext enregistré.
• Dépendre de classes concrètes plutôt que d'abstractions. Un constructeur typé SqlOrderRepository plutôt que IOrderRepository couple la classe à la technologie de stockage. Toute classe qui doit être réutilisable ou testable de manière isolée doit dépendre uniquement d'interfaces ou de types abstraits.
• Couplage caché à DateTime.Now / état statique. var now = DateTime.Now dans une méthode métier rend la classe non déterministe et non testable sans astuces de voyage dans le temps. Introduire ISystemClock (ou utiliser TimeProvider de .NET 8+) et l'injecter. La même chose s'applique à Environment.MachineName, Random.Shared, ou toute autre statique ambiante.
• Singletons mutables statiques. Un static Dictionary<string, Order> _cache dans une classe de service est un état partagé caché qui contourne la gestion de durée de vie DI et n'est pas sûr pour une utilisation concurrente. Remplacer par IMemoryCache / IDistributedCache injecté via DI, qui prend en charge un cadrage et une éviction appropriés.

Note de jugement : tous les new ne sont pas une violation DIP. new OrderDto(order) (un simple sac de données) ou new List<T>() (une collection standard) sont acceptables. DIP est important lorsque l'objet construit a un comportement, des E/S externes ou une configuration — c'est-à-dire lorsqu'il serait utile de le remplacer par un double de test ou une implémentation alternative.

Signaux d'alerte

Signal	Pourquoi c'est important
new HttpClient() construit dans une méthode ou un constructeur de service	Contourne le pooling de connexions IHttpClientFactory (épuisement des sockets sous charge), cache la dépendance et rend la classe non testable sans E/S de production.
switch(order.Type) ou if (x is ConcreteType) dans une classe coordinatrice	Doit être rouvert pour chaque nouveau variant ; les appelants en savent trop sur la hiérarchie de types. Remplacer par IEnumerable<IHandler> résolu via DI.
throw new NotImplementedException() ou throw new NotSupportedException() dans une substitution non abstraite	Le sous-type ne peut pas honorer le contrat de base — violation LSP. Soit implémenter la méthode correctement, soit restructurer la hiérarchie.
Une interface avec 10+ membres dont les appelants utilisent 2–3	Force des doubles de test volumineux ; signale que plusieurs interfaces de rôle ont été fusionnées en une seule.
Une classe construisant son propre DbContext via new AppDbContext(options)	Contourne la gestion de durée de vie DI (Scoped vs Transient vs Singleton), empêche le pooling de connexions et casse la testabilité. Injecter AppDbContext ou IDbContextFactory<AppDbContext>.
DateTime.Now appelé directement dans une méthode métier	Non déterministe dans les tests ; le couplage à l'horloge système cache une dépendance logique. Injecter TimeProvider (.NET 8+) ou ISystemClock.
Une action de contrôleur contenant de la logique métier if/switch ou des requêtes EF	Le contrôleur a pris des responsabilités qui appartiennent à la couche application/domaine ; un simple changement de règle métier nécessite de toucher au contrôleur.
Champ mutable static utilisé comme cache en processus	Non thread-safe sans verrouillage explicite ; contourne le cadrage DI ; ne peut pas être évincé ou observé. Utiliser IMemoryCache.

Exemple

Voir examples/solid-review/.