AGIBuild/dotnet-csharp-code-smells

dotnet-csharp-code-smells

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.

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.

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.

---

1. Resource Management (IDisposable Misuse)

| Smell | Why Harmful | Fix | Rule | |-------|-------------|-----|------| | Missing using on disposable locals | Leaks unmanaged handles (sockets, files, DB connections) | Wrap in using declaration or using block | CA2000 | | Undisposed IDisposable fields | Class holds disposable resource but never disposes it | Implement IDisposable; dispose fields in Dispose() | CA2213 | | Wrong Dispose pattern (no finalizer guard) | Double-dispose or missed cleanup on GC finalization | Follow canonical Dispose(bool) pattern; call GC.SuppressFinalize(this) | CA1816 | | Disposable created in one method, stored in field | Ownership unclear; easy to forget disposal | Document ownership; make the containing class IDisposable | CA2000 | | using on non-owned resource | Premature disposal of shared resource (e.g., injected HttpClient) | Only dispose resources you create; let DI manage injected services | -- |

See details.md for code examples of each pattern.

---

2. Warning Suppression Hacks

| Smell | Why Harmful | Fix | Rule | |-------|-------------|-----|------| | 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 | | Dummy variable assignments to suppress CS0219 | Dead code that confuses readers | Use _ = expression; discard or #pragma warning disable | CS0219 | | Blanket #pragma warning disable without restore | Suppresses ALL warnings for rest of file | Always pair with #pragma warning restore; suppress specific codes only | -- | | [SuppressMessage] without justification | Future maintainers cannot evaluate if suppression is still valid | Always include Justification = "reason" | CA1303 |

See details.md for the CS0067 motivating example (bad pattern to correct fix).

---

3. LINQ Anti-Patterns

| Smell | Why Harmful | Fix | Rule | |-------|-------------|-----|------| | Premature .ToList() mid-chain | Forces full materialization; wastes memory | Keep chain lazy; materialize only at the end | CA1851 | | Multiple enumeration of IEnumerable<T> | Re-executes query or DB call on each enumeration | Materialize once with .ToList() then reuse | CA1851 | | 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 | -- | | .Count() > 0 instead of .Any() | Enumerates entire collection instead of short-circuiting | Use .Any() for existence checks | CA1827 | | Nested foreach instead of .Join() or .GroupJoin() | O(n*m) when O(n+m) is possible | Use LINQ join operations or Dictionary lookup | -- | | .Where().First() instead of .First(predicate) | Creates unnecessary intermediate iterator | Pass predicate directly to .First() or .FirstOrDefault() | CA1826 |

---

4. Event Handling Leaks

| Smell | Why Harmful | Fix | Rule | |-------|-------------|-----|------| | Not unsubscribing from events | Memory leak: publisher holds reference to subscriber | Unsubscribe in Dispose() or use weak event pattern | -- | | Raising events in constructor | Subscribers may not be attached yet; derived class not fully constructed | Raise events only from fully initialized instances | CA2214 | | 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 | -- | | Event handler not checking for null | NullReferenceException when no subscribers | Use event?.Invoke() null-conditional pattern | -- | | Static event without cleanup | Rooted references prevent GC for application lifetime | Prefer instance events or use WeakEventManager | -- |

Cross-reference: [skill:dotnet-csharp-async-patterns] covers async void fire-and-forget patterns in depth.

---

5. Design Smells

| Smell | Threshold | Why Harmful | Fix | |-------|-----------|-------------|-----| | God class | >500 lines | Too many responsibilities; hard to test and maintain | Extract cohesive classes using SRP | | Long method | >30 lines | Hard to understand, test, and review | Extract helper methods with descriptive names | | Long parameter list | >5 parameters | Indicates missing abstraction | Introduce parameter object or builder | | Feature envy | Method uses another class's data more than its own | Misplaced responsibility; tight coupling | Move method to the class it envies | | Primitive obsession | Domain concepts represented as raw string/int | No type safety; validation scattered | Introduce value objects or strongly-typed IDs | | Deep nesting | >3 levels of indentation | Hard to follow control flow | Use guard clauses (early return) and extract methods |

---

6. Exception Handling Gaps

| Smell | Why Harmful | Fix | Rule | |-------|-------------|-----|------| | Empty catch block | Silently swallows errors; masks bugs | At minimum, log the exception; prefer letting it propagate | CA1031 | | Catching base Exception | Catches OutOfMemoryException, StackOverflowException, etc. | Catch specific exception types | CA1031 | | Log-and-swallow (catch { log; }) | Caller never learns operation failed | Re-throw after logging, or return error result | -- | | Throwing in finally | Masks original exception with the new one | Use try/catch inside finally; never throw from finally | -- | | throw ex; instead of throw; | Resets stack trace; hides original failure location | Use bare throw; to preserve stack trace | CA2200 | | Not including inner exception | Loses causal chain when wrapping exceptions | Pass original as innerException parameter | -- |

Cross-reference: [skill:dotnet-csharp-async-patterns] covers exception handling in fire-and-forget and async void scenarios.

---

Quick Reference: CA Rules

| Rule | Description | |------|-------------| | CA1031 | Do not catch general exception types | | CA1816 | Call GC.SuppressFinalize correctly | | CA1826 | Do not use Enumerable methods on indexable collections | | CA1827 | Do not use Count()/LongCount() when Any() can be used | | CA1851 | Possible multiple enumerations of IEnumerable collection | | CA2000 | Dispose objects before losing scope | | CA2200 | Rethrow to preserve stack details | | CA2213 | Disposable fields should be disposed | | CA2214 | Do not call overridable methods in constructors |

Enable these via <AnalysisLevel>latest-all</AnalysisLevel> in your project. See [skill:dotnet-csharp-coding-standards] for analyzer configuration.

---

References

• Microsoft Code Quality Rules
• Framework Design Guidelines
• David Fowler Async Guidance