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:
- Find definitions:
serena_find_symbol instead of text search
- Understand structure:
serena_get_symbols_overview for file organization
- Track references:
serena_find_referencing_symbols for impact analysis
- 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
1---2name: dotnet-csharp-code-smells3category: fundamentals4subcategory: diagnostics5description: Detects C# code smells during review. Anti-patterns, async misuse, DI mistakes, fixes.6license: MIT7targets: ['*']8tags: [csharp, dotnet, skill]9version: '0.0.1'10author: 'dotnet-agent-harness'11invocable: true12claudecode:13 allowed-tools: ['Read', 'Grep', 'Glob', 'Bash', 'Write', 'Edit']14codexcli:15 short-description: '.NET skill guidance for csharp tasks'16opencode:17 allowed-tools: ['Read', 'Grep', 'Glob', 'Bash', 'Write', 'Edit']18copilot: {}19geminicli: {}20antigravity: {}21---2223# dotnet-csharp-code-smells2425Proactive code-smell and anti-pattern detection for C# code. This skill triggers during all workflow modes -- planning,26implementation, and review. Each entry identifies the smell, explains why it is harmful, provides the correct fix, and27references the relevant CA rule or cross-reference.2829## Scope3031- Resource management (IDisposable misuse)32- Async anti-patterns and deadlock detection33- DI lifetime misuse and captive dependencies34- Null-handling mistakes and NRT violations35- LINQ pitfalls and string handling issues3637## Out of scope3839- LLM-specific generation mistakes (wrong NuGet packages, MSBuild errors) -- see [skill:dotnet-agent-gotchas]40- SOLID/DRY design principles -- see [skill:dotnet-solid-principles]41- Naming and style conventions -- see [skill:dotnet-csharp-coding-standards]4243Cross-references: [skill:dotnet-csharp-async-patterns] for async gotchas, [skill:dotnet-csharp-coding-standards] for44naming and style, [skill:dotnet-csharp-dependency-injection] for DI lifetime misuse,45[skill:dotnet-csharp-nullable-reference-types] for NRT annotation mistakes.4647---4849## 1. Resource Management (IDisposable Misuse)5051| Smell | Why Harmful | Fix | Rule |52| ------------------------------------------------- | ------------------------------------------------------------------- | -------------------------------------------------------------------------- | ------ |53| Missing `using` on disposable locals | Leaks unmanaged handles (sockets, files, DB connections) | Wrap in `using` declaration or `using` block | CA2000 |54| Undisposed `IDisposable` fields | Class holds disposable resource but never disposes it | Implement `IDisposable`; dispose fields in `Dispose()` | CA2213 |55| Wrong Dispose pattern (no finalizer guard) | Double-dispose or missed cleanup on GC finalization | Follow canonical `Dispose(bool)` pattern; call `GC.SuppressFinalize(this)` | CA1816 |56| Disposable created in one method, stored in field | Ownership unclear; easy to forget disposal | Document ownership; make the containing class `IDisposable` | CA2000 |57| `using` on non-owned resource | Premature disposal of shared resource (e.g., injected `HttpClient`) | Only dispose resources you create; let DI manage injected services | -- |5859See `details.md` for code examples of each pattern.6061---6263## 2. Warning Suppression Hacks6465| Smell | Why Harmful | Fix | Rule |66| ------------------------------------------------- | ---------------------------------------------------------------- | --------------------------------------------------------------------------------------- | ------ |67| 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 |68| Dummy variable assignments to suppress CS0219 | Dead code that confuses readers | Use `_ = expression;` discard or `#pragma warning disable` | CS0219 |69| Blanket `#pragma warning disable` without restore | Suppresses ALL warnings for rest of file | Always pair with `#pragma warning restore`; suppress specific codes only | -- |70| `[SuppressMessage]` without justification | Future maintainers cannot evaluate if suppression is still valid | Always include `Justification = "reason"` | CA1303 |7172See `details.md` for the CS0067 motivating example (bad pattern to correct fix).7374---7576## 3. LINQ Anti-Patterns7778| Smell | Why Harmful | Fix | Rule |79| ------------------------------------------------------- | -------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------ |80| Premature `.ToList()` mid-chain | Forces full materialization; wastes memory | Keep chain lazy; materialize only at the end | CA1851 |81| Multiple enumeration of `IEnumerable<T>` | Re-executes query or DB call on each enumeration | Materialize once with `.ToList()` then reuse | CA1851 |82| 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 | -- |83| `.Count() > 0` instead of `.Any()` | Enumerates entire collection instead of short-circuiting | Use `.Any()` for existence checks | CA1827 |84| Nested `foreach` instead of `.Join()` or `.GroupJoin()` | O(n\*m) when O(n+m) is possible | Use LINQ join operations or `Dictionary` lookup | -- |85| `.Where().First()` instead of `.First(predicate)` | Creates unnecessary intermediate iterator | Pass predicate directly to `.First()` or `.FirstOrDefault()` | CA1826 |8687---8889## 4. Event Handling Leaks9091| Smell | Why Harmful | Fix | Rule |92| ------------------------------------ | -------------------------------------------------------------------------------------------- | ------------------------------------------------------------ | ------ |93| Not unsubscribing from events | Memory leak: publisher holds reference to subscriber | Unsubscribe in `Dispose()` or use weak event pattern | -- |94| Raising events in constructor | Subscribers may not be attached yet; derived class not fully constructed | Raise events only from fully initialized instances | CA2214 |95| `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 | -- |96| Event handler not checking for null | `NullReferenceException` when no subscribers | Use `event?.Invoke()` null-conditional pattern | -- |97| Static event without cleanup | Rooted references prevent GC for application lifetime | Prefer instance events or use `WeakEventManager` | -- |9899Cross-reference: [skill:dotnet-csharp-async-patterns] covers `async void` fire-and-forget patterns in depth.100101---102103## 5. Design Smells104105| Smell | Threshold | Why Harmful | Fix |106| ------------------- | -------------------------------------------------- | ---------------------------------------------------- | ---------------------------------------------------- |107| God class | >500 lines | Too many responsibilities; hard to test and maintain | Extract cohesive classes using SRP |108| Long method | >30 lines | Hard to understand, test, and review | Extract helper methods with descriptive names |109| Long parameter list | >5 parameters | Indicates missing abstraction | Introduce parameter object or builder |110| Feature envy | Method uses another class's data more than its own | Misplaced responsibility; tight coupling | Move method to the class it envies |111| Primitive obsession | Domain concepts represented as raw `string`/`int` | No type safety; validation scattered | Introduce value objects or strongly-typed IDs |112| Deep nesting | >3 levels of indentation | Hard to follow control flow | Use guard clauses (early return) and extract methods |113114---115116## 6. Exception Handling Gaps117118| Smell | Why Harmful | Fix | Rule |119| ---------------------------------- | -------------------------------------------------------------- | ---------------------------------------------------------- | ------ |120| Empty catch block | Silently swallows errors; masks bugs | At minimum, log the exception; prefer letting it propagate | CA1031 |121| Catching base `Exception` | Catches `OutOfMemoryException`, `StackOverflowException`, etc. | Catch specific exception types | CA1031 |122| Log-and-swallow (`catch { log; }`) | Caller never learns operation failed | Re-throw after logging, or return error result | -- |123| Throwing in `finally` | Masks original exception with the new one | Use try/catch inside finally; never throw from finally | -- |124| `throw ex;` instead of `throw;` | Resets stack trace; hides original failure location | Use bare `throw;` to preserve stack trace | CA2200 |125| Not including inner exception | Loses causal chain when wrapping exceptions | Pass original as `innerException` parameter | -- |126127Cross-reference: [skill:dotnet-csharp-async-patterns] covers exception handling in fire-and-forget and async void128scenarios.129130---131132## Quick Reference: CA Rules133134| Rule | Description |135| ------ | ----------------------------------------------------------- |136| CA1031 | Do not catch general exception types |137| CA1816 | Call `GC.SuppressFinalize` correctly |138| CA1826 | Do not use `Enumerable` methods on indexable collections |139| CA1827 | Do not use `Count()`/`LongCount()` when `Any()` can be used |140| CA1851 | Possible multiple enumerations of `IEnumerable` collection |141| CA2000 | Dispose objects before losing scope |142| CA2200 | Rethrow to preserve stack details |143| CA2213 | Disposable fields should be disposed |144| CA2214 | Do not call overridable methods in constructors |145146Enable these via `<AnalysisLevel>latest-all</AnalysisLevel>` in your project. See [skill:dotnet-csharp-coding-standards]147for analyzer configuration.148149---150151## Code Navigation (Serena MCP)152153**Primary approach:** Use Serena symbol operations for efficient code navigation:1541551. **Find definitions**: `serena_find_symbol` instead of text search1562. **Understand structure**: `serena_get_symbols_overview` for file organization1573. **Track references**: `serena_find_referencing_symbols` for impact analysis1584. **Precise edits**: `serena_replace_symbol_body` for clean modifications159160**When to use Serena vs traditional tools:**161162- ✅ **Use Serena**: Navigation, refactoring, dependency analysis, precise edits163- ✅ **Use Read/Grep**: Reading full files, pattern matching, simple text operations164- ✅ **Fallback**: If Serena unavailable, traditional tools work fine165166**Example workflow:**167168```text169# Instead of:170Read: src/Services/OrderService.cs171Grep: "public void ProcessOrder"172173# Use:174serena_find_symbol: "OrderService/ProcessOrder"175serena_get_symbols_overview: "src/Services/OrderService.cs"176```177178## References179180- [Microsoft Code Quality Rules](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/)181- [Framework Design Guidelines](https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/)182- [David Fowler Async Guidance](https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md)
rudironsoni