$npx -y skills add wshaddix/dotnet-skills --skill dotnet-csharp-code-smellsReviewing C# for logic issues. Anti-patterns, common pitfalls, async misuse, DI mistakes.
| 1 | # dotnet-csharp-code-smells |
| 2 | |
| 3 | Proactive code-smell and anti-pattern detection for C# code. This skill triggers during all workflow modes -- planning, implementation, and review. Each entry identifies the smell, explains why it is harmful, provides the correct fix, and references the relevant CA rule or cross-reference. |
| 4 | |
| 5 | Cross-references: [skill:dotnet-csharp-async-patterns] for async gotchas, [skill:dotnet-csharp-coding-standards] for naming and style, [skill:dotnet-csharp-dependency-injection] for DI lifetime misuse, [skill:dotnet-csharp-nullable-reference-types] for NRT annotation mistakes. |
| 6 | |
| 7 | **Out of Scope:** LLM-specific generation mistakes (wrong NuGet packages, bad project structure, MSBuild errors) are covered by [skill:dotnet-agent-gotchas]. This skill covers general .NET code smells that any developer -- human or AI -- should avoid. |
| 8 | |
| 9 | --- |
| 10 | |
| 11 | ## 1. Resource Management (IDisposable Misuse) |
| 12 | |
| 13 | | Smell | Why Harmful | Fix | Rule | |
| 14 | |-------|-------------|-----|------| |
| 15 | | Missing `using` on disposable locals | Leaks unmanaged handles (sockets, files, DB connections) | Wrap in `using` declaration or `using` block | CA2000 | |
| 16 | | Undisposed `IDisposable` fields | Class holds disposable resource but never disposes it | Implement `IDisposable`; dispose fields in `Dispose()` | CA2213 | |
| 17 | | Wrong Dispose pattern (no finalizer guard) | Double-dispose or missed cleanup on GC finalization | Follow canonical `Dispose(bool)` pattern; call `GC.SuppressFinalize(this)` | CA1816 | |
| 18 | | Disposable created in one method, stored in field | Ownership unclear; easy to forget disposal | Document ownership; make the containing class `IDisposable` | CA2000 | |
| 19 | | `using` on non-owned resource | Premature disposal of shared resource (e.g., injected `HttpClient`) | Only dispose resources you create; let DI manage injected services | -- | |
| 20 | |
| 21 | See `details.md` for code examples of each pattern. |
| 22 | |
| 23 | --- |
| 24 | |
| 25 | ## 2. Warning Suppression Hacks |
| 26 | |
| 27 | | Smell | Why Harmful | Fix | Rule | |
| 28 | |-------|-------------|-----|------| |
| 29 | | Invoking event with `null` to suppress CS0067 | Creates misleading runtime behavior; masks real bugs | Use `#pragma warning disable CS0067` or explicit event accessors `{ add {} remove {} }` | CS0067 | |
| 30 | | Dummy variable assignments to suppress CS0219 | Dead code that confuses readers | Use `_ = expression;` discard or `#pragma warning disable` | CS0219 | |
| 31 | | Blanket `#pragma warning disable` without restore | Suppresses ALL warnings for rest of file | Always pair with `#pragma warning restore`; suppress specific codes only | -- | |
| 32 | | `[SuppressMessage]` without justification | Future maintainers cannot evaluate if suppression is still valid | Always include `Justification = "reason"` | CA1303 | |
| 33 | |
| 34 | See `details.md` for the CS0067 motivating example (bad pattern to correct fix). |
| 35 | |
| 36 | --- |
| 37 | |
| 38 | ## 3. LINQ Anti-Patterns |
| 39 | |
| 40 | | Smell | Why Harmful | Fix | Rule | |
| 41 | |-------|-------------|-----|------| |
| 42 | | Premature `.ToList()` mid-chain | Forces full materialization; wastes memory | Keep chain lazy; materialize only at the end | CA1851 | |
| 43 | | Multiple enumeration of `IEnumerable<T>` | Re-executes query or DB call on each enumeration | Materialize once with `.ToList()` then reuse | CA1851 | |
| 44 | | Client-side evaluation in EF Core | Loads entire table into memory; silent perf bomb | Rewrite query as translatable LINQ or use `AsAsyncEnumerable()` with explicit intent | -- | |
| 45 | | `.Count() > 0` instead of `.Any()` | Enumerates entire collection instead of short-circuiting | Use `.Any()` for existence checks | CA1827 | |
| 46 | | Nested `foreach` instead of `.Join()` or `.GroupJoin()` | O(n*m) when O(n+m) is possible | Use LINQ join operations or `Dictionary` lookup | -- | |
| 47 | | `.Where().First()` instead of `.First(predicate)` | Creates unnecessary intermediate iterator | Pass predicate directly to `.First()` or `.FirstOrDefault()` | CA1826 | |
| 48 | |
| 49 | --- |
| 50 | |
| 51 | ## 4. Event Handling Leaks |
| 52 | |
| 53 | | Smell | Why Harmful | Fix | Rule | |
| 54 | |-------|-------------|-----|------| |
| 55 | | Not unsubscribing from events | Memory leak: publisher holds reference to subscriber | Unsubscribe in `Dispose()` or use weak event pattern | -- | |
| 56 | | Raising events in constructor | Subscribers may not be attached yet; derived class not fully constructed | Raise events only from fully initialized instances | CA2214 | |
| 57 | | `async void` event handler (misused) | `async void` is the only valid signature for event handlers, but exceptions are unobservable | Wrap body in try/catch; log and handle exceptions explicitly | -- | |
| 58 | | Event handler not checking for null | `NullReferenceException` when no subscribers | Use `event?.Invoke()` null-conditional pattern | -- | |
| 59 | | Static event without cleanup | Rooted references prevent GC for application lifetime | Prefer instance events or use `WeakEventManager` | -- | |
| 60 | |
| 61 | Cross-reference: [skill:dotnet-csharp-async-patterns] covers `async void` fire-and-forget patterns in depth. |
| 62 | |
| 63 | --- |
| 64 | |
| 65 | ## 5. |