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.

Scope

• Resource management (IDisposable misuse)
• Async anti-patterns and deadlock detection
• DI lifetime misuse and captive dependencies
• Null-handling mistakes and NRT violations
• LINQ pitfalls and string handling issues

Out of scope

• LLM-specific generation mistakes (wrong NuGet packages, MSBuild errors) -- see [skill:dotnet-agent-gotchas]
• SOLID/DRY design principles -- see [skill:dotnet-solid-principles]
• Naming and style conventions -- see [skill:dotnet-csharp-coding-standards]

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.

---

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.

---

Code Navigation (Serena MCP)

Primary approach: Use Serena symbol operations for efficient code navigation:

1. Find definitions: serena_find_symbol instead of text search
2. Understand structure: serena_get_symbols_overview for file organization
3. Track references: serena_find_referencing_symbols for impact analysis
4. Precise edits: serena_replace_symbol_body for clean modifications

When to use Serena vs traditional tools:

• ✅ Use Serena: Navigation, refactoring, dependency analysis, precise edits
• ✅ Use Read/Grep: Reading full files, pattern matching, simple text operations
• ✅ Fallback: If Serena unavailable, traditional tools work fine

Example workflow:

# Instead of:
Read: src/Services/OrderService.cs
Grep: "public void ProcessOrder"

# Use:
serena_find_symbol: "OrderService/ProcessOrder"
serena_get_symbols_overview: "src/Services/OrderService.cs"

References

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