From e29b1a838f54a3c8a84b403f06352f351f0a3f1f Mon Sep 17 00:00:00 2001 From: artl Date: Thu, 12 Feb 2026 09:55:15 -0800 Subject: [PATCH 1/5] Add .NET API design reviewer agent and api-design-cop skill --- agents/dotnet-api-design-reviewer.agent.md | 126 ++++++ skills/dotnet-api-design-cop/SKILL.md | 360 ++++++++++++++++++ .../references/api-review-checklist.md | 166 ++++++++ .../references/error-handling-patterns.md | 195 ++++++++++ .../references/member-design-patterns.md | 266 +++++++++++++ .../references/naming-conventions.md | 177 +++++++++ .../references/type-design-patterns.md | 199 ++++++++++ 7 files changed, 1489 insertions(+) create mode 100644 agents/dotnet-api-design-reviewer.agent.md create mode 100644 skills/dotnet-api-design-cop/SKILL.md create mode 100644 skills/dotnet-api-design-cop/references/api-review-checklist.md create mode 100644 skills/dotnet-api-design-cop/references/error-handling-patterns.md create mode 100644 skills/dotnet-api-design-cop/references/member-design-patterns.md create mode 100644 skills/dotnet-api-design-cop/references/naming-conventions.md create mode 100644 skills/dotnet-api-design-cop/references/type-design-patterns.md diff --git a/agents/dotnet-api-design-reviewer.agent.md b/agents/dotnet-api-design-reviewer.agent.md new file mode 100644 index 0000000000..9c83ead569 --- /dev/null +++ b/agents/dotnet-api-design-reviewer.agent.md @@ -0,0 +1,126 @@ +--- +description: "Use this agent when the user wants to review, design, or improve .NET API surfaces for consistency with established C# conventions.\n\nTrigger phrases include:\n- 'review my API design'\n- 'is this API consistent with .NET conventions?'\n- 'check my public API surface'\n- 'help me design this .NET API'\n- 'review my naming conventions'\n- 'should this be a class or struct?'\n- 'is this a breaking change?'\n- 'prepare an API proposal'\n- 'check my exception design'\n- 'review my library API'\n\nExamples:\n- User asks 'Does my API follow .NET naming conventions?' → invoke this agent to review naming against C# conventions\n- User shares a class and asks 'Is this API well-designed?' → invoke this agent to perform a full API design review\n- User asks 'Should I use a class or struct for this type?' → invoke this agent to analyze type design\n- User says 'I need to add a new overload without breaking existing callers' → invoke this agent to assess breaking change risk\n- User asks 'Review this API proposal before I submit it' → invoke this agent to apply the full API review checklist" +name: dotnet-api-design-reviewer +tools: ['shell', 'read', 'search', 'edit', 'task', 'skill', 'web_search', 'web_fetch', 'ask_user'] +--- + +# dotnet-api-design-reviewer instructions + +You are a senior .NET API design reviewer with deep expertise in established C# conventions and patterns. You care deeply about naming precision, type design choices, member design patterns, and the developer experience of consuming APIs. + +## Your Mission + +Help developers design and review .NET API surfaces that are consistent with established C# conventions by: +- Reviewing public API surfaces for naming, type design, member design, and pattern conformance +- Guiding type design decisions (class vs struct vs interface vs enum) +- Evaluating member design (properties vs methods, overloading patterns, parameter design) +- Checking error handling patterns +- Assessing extensibility mechanisms +- Validating collection usage in public APIs +- Identifying breaking changes and versioning risks +- Preparing and reviewing API proposals + +You do NOT cite or reference the Pearson-licensed "Framework Design Guidelines" book or the learn.microsoft.com/en-us/dotnet/standard/design-guidelines/ pages. + +## Core Design Philosophy + +1. **Caller-first design**: Always start with the code a developer will write. If the calling code is awkward, the API needs work — regardless of how clean the implementation is. + +2. **Consistency**: Follow established C# conventions so developers can transfer their existing knowledge to new APIs without surprises. + +3. **Progressive disclosure**: Simple things should be simple. Advanced things should be possible. The simplest overload handles 80% of use cases. + +4. **Hard to misuse**: The correct usage should be easier than the incorrect usage. The "pit of success" should be wide. + +5. **Additive evolution**: APIs can only be added, never removed without breaking consumers. Be conservative in what you expose — you can always add later. + +## Review Methodology + +### 1. Scenario Assessment +Write calling code for the top scenarios. If the code is clean in 3-5 lines, the design is on track. + +### 2. Naming Review +Check against established C# naming conventions: +- PascalCase types/methods/properties, camelCase parameters +- Verbs for methods, nouns for properties +- I-prefix interfaces, T-prefix type parameters +- Standard suffixes (Exception, Attribute, EventArgs, Collection) +- No abbreviations, no Hungarian notation + +### 3. Type Design Review +Apply established type choice conventions: +- Structs: small, immutable, value semantics (like DateTime, TimeSpan, Guid) +- Classes: identity, complex behavior, inheritance (like Stream, HttpClient) +- Interfaces: cross-hierarchy contracts (like IDisposable, IEnumerable) +- Enums: singular for non-flags, plural with [Flags] for flags + +### 4. Member Design Review +Check against established member patterns: +- Properties for state, methods for operations +- Consistent overloading patterns +- EventHandler for events +- No public fields +- CancellationToken always last + +### 5. Error Handling Review +Check error handling conventions: +- Standard exception types with paramName +- Try-Parse pattern for commonly-failing operations +- ThrowIf helpers (.NET 6+) +- Synchronous argument validation in async methods + +### 6. Collection Review +Check collection conventions: +- Collection/ReadOnlyCollection for public APIs (not List) +- IEnumerable for parameters +- Empty, not null, for empty collections + +### 7. Breaking Change Assessment +Identify anything that would break existing consumers. + +## Output Format + +When reviewing APIs: + +1. **Summary Assessment**: Brief overall evaluation +2. **Issues Found**: Categorized by severity + - **Critical**: Patterns that contradict established conventions (mutable structs, List in public API, bare Exception throwing) + - **Warning**: Deviations from common C# patterns + - **Suggestion**: Polish improvements +3. **For each issue**: What the convention is, what the code does, recommended fix with code example +4. **Strengths**: What's done well +5. **Scenario Test**: Sample calling code to validate usability + +When designing new APIs: + +1. **Scenario Code First**: Show calling code +2. **Proposed API Surface**: Type/member listing +3. **Design Rationale**: Why specific choices were made +4. **Breaking Change Risk**: If modifying existing APIs + +## Skills + +- **dotnet-api-design-cop**: Load for the comprehensive API design review workflow, checklists, and reference materials covering naming conventions, type design patterns, member design patterns, error handling patterns, and the API review checklist. + +## When to Ask for Clarification + +- If you don't know whether the API is for a library, framework, or application +- If the target .NET version is unclear +- If the review scope is ambiguous +- If multiple valid designs exist and you need to understand priorities +- If you need the existing API surface to assess breaking changes + +## Escalation + +Acknowledge when the question is better handled by another agent: +- Implementation performance → `dotnet-jit-expert` +- Async pattern correctness → `dotnet-async-patterns` / `dotnet-concurrency-expert` +- Synchronization primitives → `dotnet-sync-primitives` + +## Tone + +- **Precise**: Every recommendation references established conventions +- **Consistent**: Same standards applied uniformly +- **Pragmatic**: Conventions have context; explain when deviation is reasonable +- **Educational**: Explain the reasoning so developers internalize the patterns +- **Constructive**: Praise good choices alongside identifying issues diff --git a/skills/dotnet-api-design-cop/SKILL.md b/skills/dotnet-api-design-cop/SKILL.md new file mode 100644 index 0000000000..26730a5557 --- /dev/null +++ b/skills/dotnet-api-design-cop/SKILL.md @@ -0,0 +1,360 @@ +--- +name: dotnet-api-design-cop +description: Reviews .NET API designs for consistency with established C# conventions. Use when reviewing public API surfaces, designing new library APIs, or checking naming, type choices, member design, error handling, and extensibility patterns against established C# conventions. +--- + +# .NET API Design Review + +This skill provides actionable guidance for reviewing .NET API designs against established C# conventions. It covers naming conventions, type design choices, member design, error handling, extensibility, collection usage, and resource management patterns. + +## When to Use + +- Reviewing a public API surface for consistency with C# conventions +- Designing new types, methods, properties, or events for a .NET library +- Preparing an API proposal for review +- Checking naming consistency across a namespace or assembly +- Evaluating type design choices (class vs struct vs interface) +- Reviewing error handling and exception usage +- Assessing extensibility mechanisms +- Validating collection usage in public APIs +- Checking IDisposable implementation patterns +- Evaluating breaking change risk in API modifications + +## When Not to Use + +- Performance optimization (use `dotnet-jit-optimization` skill) +- Async/await pattern correctness (use `dotnet-async-patterns` skill) +- Synchronization primitive selection (use `dotnet-sync-primitives` skill) +- Internal/private code review (these conventions target public API surfaces) +- REST/HTTP API endpoint design (different domain) + +## Inputs + +| Input | Required | Description | +|-------|----------|-------------| +| Code under review | Yes | The API surface to analyze (types, members, signatures) | +| Context | Recommended | Library, framework, or application? NuGet package or internal? | +| .NET version | Recommended | Target framework version | +| Scope | Recommended | Full review or focused area (naming, types, errors, etc.) | + +## Workflow + +### Step 1: Classify the API surface and write caller code + +First, classify what you are reviewing: + +| Surface type | Focus areas | Key risk | +|-------------|-------------|----------| +| New library API | Full review — naming, types, members, errors | Getting the shape wrong before anyone depends on it | +| Extension to existing API | Consistency with existing surface, overload patterns | Inconsistency with the established conventions of the library | +| Modification to existing API | Breaking change assessment first, then design | Breaking existing consumers | +| API proposal | Scenario code, then full review | Approving a surface that is hard to use | + +Then write sample code that uses the API for its top 2-3 scenarios. + +Ask: +1. Can a developer accomplish the main task in a few lines? +2. Is there one obvious type where usage starts? +3. Can users create an instance, set properties, and call methods without complex initialization? +4. Would IntelliSense lead developers to the right type? + +If the calling code is awkward, the API design needs work — regardless of how clean the implementation is. + +### Step 2: Review naming conventions + +These are established C# naming conventions. + +**Casing rules:** + +| Element | Convention | Examples | +|---------|-----------|-------------| +| Types | PascalCase | `StreamReader`, `StringBuilder`, `HttpClient` | +| Methods | PascalCase | `ReadLine()`, `GetHashCode()`, `ToString()` | +| Properties | PascalCase | `Length`, `Count`, `IsReadOnly` | +| Events | PascalCase | `Click`, `PropertyChanged`, `Closed` | +| Parameters | camelCase | `buffer`, `index`, `cancellationToken` | +| Constants | PascalCase | `MaxValue`, `Empty` | +| Interfaces | `I` + PascalCase | `IDisposable`, `IEnumerable`, `IComparable` | +| Type parameters | `T` + PascalCase | `TKey`, `TValue`, `TResult` (or just `T` for single param) | + +**Naming patterns:** + +| Pattern | Convention | Examples | +|---------|-----------|-------------| +| Methods | Verb or verb phrase | `Read`, `Write`, `Parse`, `CompareTo` | +| Properties | Noun, noun phrase, or adjective | `Name`, `Count`, `IsReadOnly`, `HasValue` | +| Boolean properties | Affirmative phrasing, often `Is`/`Can`/`Has` prefix | `IsEnabled`, `CanRead`, `HasValue` | +| Events | Verb tense (gerund for pre, past for post) | `Closing`/`Closed`, `Validating`/`Validated` | +| Exception types | End with `Exception` | `ArgumentNullException`, `IOException` | +| Attribute types | End with `Attribute` | `SerializableAttribute`, `ObsoleteAttribute` | +| EventArgs types | End with `EventArgs` | `CancelEventArgs`, `PropertyChangedEventArgs` | +| Enum (non-flag) | Singular noun | `ConsoleColor`, `DayOfWeek`, `FileMode` | +| Enum (flag) | Plural noun with `[Flags]` | `FileAttributes`, `BindingFlags` | + +**Common violations to flag:** +- Abbreviations or contractions in public names +- Hungarian notation (`strName`, `iCount`) +- Underscores in public member names +- Language-specific type names in method names (`GetInt` instead of `GetInt32`) +- Method names using nouns instead of verbs +- Property names using verbs instead of nouns + +**Before/after example — naming review correction:** +```csharp +// BEFORE: Multiple naming violations +public class data_processor // underscore, vague name +{ + public int GetInt(string s) { ... } // language-specific type name, cryptic parameter + public bool process() { ... } // lowercase, but also: does this return success? + public string strName { get; set; } // Hungarian notation +} + +// AFTER: Consistent naming +public class DataParser // PascalCase, specific noun +{ + public int GetInt32(string text) { ... } // framework type name, descriptive parameter + public DataResult Parse() { ... } // PascalCase verb, clear return type + public string Name { get; set; } // no prefix, noun +} +``` + +### Step 3: Review type design choices + +**When to use structs:** +`DateTime`, `TimeSpan`, `Guid`, `Point`, `Color`, `Decimal`, `Int32` — small, immutable types representing single values. + +Struct suitability checklist: +- Logically represents a single value +- Instance size ≤ 16 bytes +- Immutable (or `readonly struct`) +- No need for inheritance +- Frequently allocated (value type avoids heap allocation) + +**When to use classes:** +`String`, `Stream`, `HttpClient`, `List` — types with identity, complex behavior, inheritance, or large size. + +**When to use interfaces:** +`IEnumerable`, `IDisposable`, `IComparable` — cross-hierarchy contracts that both classes and structs implement. + +**Check for:** +- Mutable structs (these cause subtle bugs with value-copy semantics) +- Interfaces without any implementation in the library +- Enums missing `[Flags]` when values are combinable +- Overly sealed types (`String` is sealed, `Stream` is not — seal deliberately) + +**Before/after example — type design correction:** +```csharp +// BEFORE: Mutable struct with reference semantics +public struct Connection +{ + public string Host { get; set; } // mutable + public int Port { get; set; } // mutable + public List Tags { get; set; } // reference type in struct + public void Connect() { ... } // side-effecting method on value type +} + +// AFTER: Class (has identity, side effects, reference-type fields) +public class Connection : IDisposable +{ + public Connection(string host, int port) { ... } + public string Host { get; } + public int Port { get; } + public void Connect() { ... } + public void Dispose() { ... } +} +``` + +### Step 4: Review member design + +**Properties vs methods:** +Use properties for cheap, idempotent state access and methods for operations, conversions, or expensive work. + +- `stream.Length` — property (cheap, idempotent) +- `stream.Read(buffer, offset, count)` — method (operation) +- `object.ToString()` — method (conversion) +- `list.ToArray()` — method (creates new object) + +**Overload patterns (as in `StringBuilder`, `Console`, etc.):** +- Consistent parameter order across overloads +- Simplest overload delegates to the most complete one +- The most-parameter overload contains the core logic + +```csharp +// Standard overload pattern +public void Write(string value) => Write(value, 0, value.Length); +public void Write(string value, int startIndex) => Write(value, startIndex, value.Length - startIndex); +public void Write(string value, int startIndex, int count) { /* core */ } +``` + +**Constructor patterns:** +- Default constructors enable simple instantiation +- Parameterized constructors for required initialization +- `CancellationToken` always last when present + +**Event patterns:** +- Use `EventHandler` +- Raise through `protected virtual void On(EventArgs e)` +- Custom EventArgs derive from `System.EventArgs` + +### Step 5: Review error handling + +**Standard error reporting patterns:** + +| Situation | Convention | Example | +|-----------|------------|---------| +| Null argument | `ArgumentNullException` with `paramName` | `ArgumentNullException.ThrowIfNull(path)` | +| Out-of-range value | `ArgumentOutOfRangeException` with `paramName` | `ArgumentOutOfRangeException.ThrowIfNegative(count)` | +| Invalid state | `InvalidOperationException` | Calling `Read` on a closed stream | +| Not supported | `NotSupportedException` | Calling `Write` on a read-only stream | +| After disposal | `ObjectDisposedException` | Using a disposed `HttpClient` | +| Parse failure | Try-Parse pattern | `int.Parse` throws, `int.TryParse` returns bool | + +**Check for:** +- Throwing `Exception` or `SystemException` directly +- Missing `paramName` on argument exceptions +- Vague exception messages +- No Try-Parse pattern for commonly-failing operations +- Exceptions thrown from `Equals`, `GetHashCode`, or `ToString` + +### Step 6: Review collection usage + +**Established conventions for collections in APIs:** + +| Position | Preferred type | Avoid | Example | +|----------|---------------|-------|-------------| +| Return type (writable) | `Collection` | `List` | `HttpHeadersCollection` | +| Return type (read-only) | `ReadOnlyCollection` | `T[]` (mutable) | `ReadOnlyCollection` | +| Parameter (input) | `IEnumerable` | `List`, `T[]` | `AddRange(IEnumerable)` | +| Property (writable) | `Collection`, get-only | settable `List` | `Items { get; }` | +| Empty collection | `Array.Empty()` or empty instance | `null` | `Enumerable.Empty()` | + +For deeper collection guidance, see [references/](references/). + +### Step 7: Review resource management + +If the type manages resources, check for proper `IDisposable` implementation. + +**Standard dispose pattern (as in `Stream`, `DbConnection`, etc.):** +```csharp +public class ResourceHolder : IDisposable +{ + private bool _disposed; + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) { /* release managed resources */ } + _disposed = true; + } + } +} +``` + +### Step 8: Review extensibility + +**Established extensibility patterns:** + +| Pattern | Example | Purpose | When to use | +|---------|------------|---------|-------------| +| Abstract methods | `Stream.Read`, `Stream.Write` | Forced customization points | Subclass MUST provide behavior | +| Virtual methods | `HttpMessageHandler.SendAsync` | Optional customization | Subclass MAY override default behavior | +| Events | `FileSystemWatcher.Changed` | Notification extensibility | External observers, no subclassing needed | +| Delegates/Func | `List.Find(Predicate)` | Caller-supplied logic | One-off customization at call site | +| Sealed types | `String`, `AesGcm` | Prevent inheritance | Type is not designed for extension | + +**Check for:** +- Unsealed types with no virtual members (suggests sealing was forgotten) +- Virtual members with no base implementation and no documentation of expected behavior +- Extensibility via inheritance where events or delegates would be simpler + +For deeper extensibility guidance, see [references/](references/). + +### Step 9: Assess breaking change risk + +If modifying existing APIs, check that no existing consumer code would break: + +| Change | Breaking? | Mitigation | +|--------|-----------|------------| +| Remove/rename a public type or member | Yes | Add new member, `[Obsolete]` the old one | +| Change a method's return type | Yes | Add new method with different name | +| Add a required parameter | Yes | Add an overload instead; keep the old signature | +| Add a member to an interface | Yes (pre-DIM) | Use default interface methods (.NET 5+) or add a new interface | +| Change parameter type (e.g. `string` → `ReadOnlySpan`) | Yes | Add overload, keep original | +| Add a new overload | Usually no | Can break if overload resolution becomes ambiguous | +| Add a new optional parameter | Usually no | Can break binary compat (recompile required) | +| Add a new type or member | No | Safe — additive change | +| Seal a previously unsealed class | Yes | Cannot be undone | +| Change exception type thrown | Yes (behavioral) | Document and version-gate | + +**Rule of thumb**: If you are unsure whether a change is breaking, treat it as breaking. + +## Output Format + +Structure every review as: + +1. **Surface classification**: New / Extension / Modification, library vs application context +2. **Scenario code**: Calling code for top 2-3 scenarios (written in Step 1) +3. **Issues found** (grouped by severity): + - **Critical**: Contradicts established conventions (mutable struct, `List` in public API, bare `Exception`, missing `IDisposable`) + - **Warning**: Deviates from common C# patterns (naming mismatch, inconsistent overloads, missing Try-Parse) + - **Suggestion**: Polish improvements (more descriptive parameter names, better IntelliSense ordering) +4. **For each issue**: What the convention is → what the code does → recommended fix with before/after code +5. **Strengths**: What the API does well (always include this) +6. **Breaking change assessment**: If modifying existing APIs + +## Failure Modes and Recovery + +| Situation | Recovery | +|-----------|----------| +| Insufficient context (don't know if it's a library or app) | Ask the developer — guidance differs (e.g. `Collection` matters for libraries, less so for app-internal code) | +| Reviewing internal/private code | Clarify scope — these conventions target public API surfaces. Internal code has more latitude. | +| Conflicting conventions | Acknowledge the inconsistency, recommend the more recent pattern | +| Performance vs. API purity tradeoff | Defer to the `dotnet-jit-expert` agent for runtime-level optimization or the `dotnet-performance-patterns-reviewer` for API usage patterns; keep the public API clean | +| Breaking change is unavoidable | Document the break, suggest `[Obsolete]` transition period, recommend a major version bump | + +## Validation + +- [ ] All public type and member names use PascalCase +- [ ] All parameters use camelCase +- [ ] Interface names start with `I` +- [ ] Methods use verb names; properties use noun/adjective names +- [ ] Boolean properties use affirmative phrasing +- [ ] Events use verb tense naming (gerund/past) +- [ ] Flag enums have `[Flags]` and plural names +- [ ] No `List` or `Dictionary` in public API surface +- [ ] Collections return empty (not null) when empty +- [ ] Standard exception types used with `paramName` set +- [ ] `IDisposable` pattern correctly implemented where needed +- [ ] Overloaded methods have consistent parameter ordering +- [ ] No breaking changes to existing consumers +- [ ] Calling code for top scenarios is clean and intuitive + +## Common Pitfalls + +| Pitfall | Solution | +|---------|----------| +| `List` in public API | Use `Collection` or `ReadOnlyCollection` | +| Public fields | Convert to properties | +| Returning null from collection properties | Return empty collection | +| Method names using nouns | Methods are actions — use verbs | +| Throwing bare `Exception` | Use specific types (`ArgumentNullException`, `InvalidOperationException`) | +| Mutable structs | Make value types readonly/immutable | +| Missing Try-Parse variant | Provide `TryParse` alongside `Parse` for commonly-failing ops | +| No `paramName` on argument exceptions | Always set via constructor or `ThrowIfNull` | +| Inconsistent overload parameter order | Align all overloads; simplest delegates to most complete | + +## References + +For deeper guidance on specific topics, see: +- [Naming Conventions](references/naming-conventions.md) — C# naming patterns and coding style +- [Type Design Patterns](references/type-design-patterns.md) — Class, struct, interface, and enum patterns +- [Member Design Patterns](references/member-design-patterns.md) — Properties, methods, events, constructors, operators +- [Error Handling Patterns](references/error-handling-patterns.md) — Exception types, Try-Parse, argument validation +- [API Review Checklist](references/api-review-checklist.md) — Checklist for API reviews and proposal preparation diff --git a/skills/dotnet-api-design-cop/references/api-review-checklist.md b/skills/dotnet-api-design-cop/references/api-review-checklist.md new file mode 100644 index 0000000000..829eeae865 --- /dev/null +++ b/skills/dotnet-api-design-cop/references/api-review-checklist.md @@ -0,0 +1,166 @@ +# API Review Checklist Reference + +A structured checklist for .NET API reviews, based on established C# conventions. + +## API Review Process + +Every public API should be reviewed before release. A structured process ensures consistency and quality. + +### Process Flow + +1. **File a proposal** containing an API sketch and usage samples +2. **Area owner reviews** and engages in discussion +3. **Owner marks** the proposal ready for review when actionable +4. **Review panel evaluates** the proposal in a scheduled meeting +5. **Outcome**: approved, needs work, or rejected + +## Proposal Template + +When proposing APIs, write a proposal that shows usage before listing signatures: + +```markdown +## API Proposal: [Feature Name] + +### Background and motivation + +[Why is this needed? What problem does it solve?] + +### API Usage + +```csharp +// Show the top 2-3 scenarios as calling code +var processor = new DataProcessor(); +processor.Configure("setting"); +var result = await processor.ProcessAsync(data, cancellationToken); +``` + +### Proposed API + +```csharp +namespace System.Data +{ + public class DataProcessor + { + public DataProcessor(); + public void Configure(string setting); + public Task ProcessAsync( + ReadOnlyMemory data, + CancellationToken cancellationToken = default); + } +} +``` + +### Alternative designs + +[What else was considered and why was it rejected?] + +### Risks + +[Breaking changes, compatibility, performance implications] +``` + +## Pre-Review: Scenario Validation + +- [ ] Top 2-3 scenarios defined with sample calling code +- [ ] Calling code is clean and fits in a few lines +- [ ] There is one clear entry-point type for the feature +- [ ] Simple instantiation works (default constructor or minimal params) +- [ ] API would be discoverable through IntelliSense +- [ ] An unfamiliar developer could use it without reading documentation + +## Naming Review + +### Types +- [ ] PascalCase for all type names +- [ ] Classes/structs use nouns or noun phrases +- [ ] Interfaces start with `I` + adjective/noun +- [ ] Exception types end with `Exception` +- [ ] Attribute types end with `Attribute` +- [ ] Collection types end with `Collection` or `Dictionary` +- [ ] EventArgs types end with `EventArgs` +- [ ] Non-flag enums singular, flag enums plural with `[Flags]` + +### Members +- [ ] Methods use verbs or verb phrases +- [ ] Properties use nouns, noun phrases, or adjectives +- [ ] Boolean properties use `Is`/`Can`/`Has` prefix where appropriate +- [ ] Events use gerund (pre) / past tense (post) naming +- [ ] Async methods end with `Async` +- [ ] No public fields — properties used instead + +### Parameters +- [ ] camelCase for all parameters +- [ ] Consistent names across overloads and interface implementations +- [ ] `CancellationToken` is last parameter +- [ ] `paramName` set on all argument exceptions + +### General +- [ ] No abbreviations or contractions +- [ ] No Hungarian notation +- [ ] No underscores in public names +- [ ] No names differing only by case +- [ ] Acronyms: 2-letter uppercase, 3+ PascalCase + +## Type Design Review + +- [ ] Struct choice justified (small, immutable, value semantics) +- [ ] No mutable structs +- [ ] Structs implement `IEquatable` with value equality +- [ ] Interfaces have implementations and consumers +- [ ] Abstract classes have concrete implementations +- [ ] Types unsealed unless specific reason to seal +- [ ] Each type is a cohesive set of related members + +## Member Design Review + +- [ ] Properties are cheap and idempotent +- [ ] Methods used for operations, conversions, expensive work +- [ ] Overloads have consistent parameter order +- [ ] Simplest overload delegates to most complete +- [ ] Constructors support simple instantiation +- [ ] Events use `EventHandler` pattern +- [ ] Operators come in pairs with named equivalents +- [ ] `GetHashCode` overridden alongside `Equals` + +## Error Handling Review + +- [ ] Standard exception types used +- [ ] No direct `Exception` or `SystemException` throwing +- [ ] `paramName` set on argument exceptions +- [ ] Exception messages are clear and actionable +- [ ] Try-Parse pattern for commonly-failing operations +- [ ] Arguments validated synchronously in async methods +- [ ] `Equals`/`GetHashCode`/`ToString` don't throw + +## Collection Review + +- [ ] No `List` or `Dictionary` in public API surface +- [ ] `Collection`/`ReadOnlyCollection` for return types +- [ ] `IEnumerable` for input parameters +- [ ] Collection properties are get-only +- [ ] Empty returned instead of null +- [ ] Collections preferred over arrays + +## Resource Management Review + +- [ ] `IDisposable` implemented if holding disposable/unmanaged resources +- [ ] `Dispose(bool)` pattern used correctly +- [ ] `GC.SuppressFinalize(this)` called in Dispose() +- [ ] `ObjectDisposedException` thrown from post-disposal usage +- [ ] `IAsyncDisposable` considered for async resource cleanup + +## Breaking Change Assessment + +**Breaking (avoid):** +- [ ] No types/members removed or renamed +- [ ] No method signatures changed +- [ ] No members added to interfaces +- [ ] No return types changed +- [ ] No exception types changed for existing conditions + +**Safe:** +- Adding new types ✔️ +- Adding new members to classes ✔️ +- Adding new overloads ✔️ +- Adding new enum values ⚠️ (can break switch statements) +- Adding optional parameters ⚠️ (can cause source-level breaks in some cases) diff --git a/skills/dotnet-api-design-cop/references/error-handling-patterns.md b/skills/dotnet-api-design-cop/references/error-handling-patterns.md new file mode 100644 index 0000000000..3654e1cf08 --- /dev/null +++ b/skills/dotnet-api-design-cop/references/error-handling-patterns.md @@ -0,0 +1,195 @@ +# Error Handling Patterns Reference + +Established C# error handling conventions. + +## Standard Exception Types + +Use a consistent set of exception types. Custom exception types should be rare. + +| Exception Type | When It Is Thrown | Example | +|---------------|----------------------|---------| +| `ArgumentNullException` | A null argument was passed | `File.Open(null, ...)` | +| `ArgumentOutOfRangeException` | A value is outside the valid range | `new List(-1)` | +| `ArgumentException` | General argument validation failure | `new Uri("not a valid uri")` | +| `InvalidOperationException` | Object state doesn't support the call | `enumerator.Current` before `MoveNext` | +| `NotSupportedException` | Operation is inherently unsupported | `readOnlyStream.Write(...)` | +| `ObjectDisposedException` | Object has been disposed | Using a disposed `HttpClient` | +| `OperationCanceledException` | Operation was canceled | `cancellationToken.ThrowIfCancellationRequested()` | +| `FormatException` | String is not in the expected format | `int.Parse("abc")` | +| `IOException` | I/O operation failed | File not found, disk full | +| `UnauthorizedAccessException` | Caller lacks permission | Accessing a protected file | +| `KeyNotFoundException` | Key not found in dictionary | `dict["missing_key"]` | +| `IndexOutOfRangeException` | Array/span index invalid | Runtime-thrown, not user code | +| `NullReferenceException` | Null dereference | Runtime-thrown, not user code | + +**What to avoid:** +- Throws `Exception` directly +- Throws `SystemException` directly +- Throws `ApplicationException` (legacy, unused) +- Throws `NullReferenceException` or `IndexOutOfRangeException` from library code (these are runtime errors) + +## Argument Validation Patterns + +### Modern Pattern (.NET 6+) +Use static `ThrowIf` methods for argument validation: + +```csharp +public void SetName(string name) +{ + ArgumentNullException.ThrowIfNull(name); + ArgumentException.ThrowIfNullOrWhiteSpace(name); + _name = name; +} + +public void SetCount(int count) +{ + ArgumentOutOfRangeException.ThrowIfNegative(count); + ArgumentOutOfRangeException.ThrowIfGreaterThan(count, MaxCount); + _count = count; +} + +public void DoWork() +{ + ObjectDisposedException.ThrowIf(_disposed, this); + // ... +} +``` + +### Property Setter Validation +```csharp +public string Name +{ + get => _name; + set + { + ArgumentException.ThrowIfNullOrEmpty(value); + _name = value; + } +} +``` + +Note: Use `value` as the parameter name in property setter exceptions (it's the implicit parameter name). + +### Legacy Pattern (pre-.NET 6) +```csharp +public void SetName(string name) +{ + if (name is null) + throw new ArgumentNullException(nameof(name)); + if (string.IsNullOrWhiteSpace(name)) + throw new ArgumentException("Name cannot be empty.", nameof(name)); + _name = name; +} +``` + +## The Try-Parse Pattern + +The convention is to provide both throwing and non-throwing variants for operations that commonly fail: + +```csharp +// Throwing variant — for when failure is exceptional +public static int Parse(string s); +public static DateTime Parse(string s); +public static IPAddress Parse(string ipString); + +// Non-throwing variant — for when failure is expected +public static bool TryParse(string s, out int result); +public static bool TryParse(string s, out DateTime result); +public static bool TryParse(string ipString, out IPAddress? address); +``` + +**Modern Try-Parse pattern (.NET 7+):** +```csharp +// IParsable interface standardizes this pattern +public interface IParsable where TSelf : IParsable +{ + static abstract TSelf Parse(string s, IFormatProvider? provider); + static abstract bool TryParse(string? s, IFormatProvider? provider, out TSelf result); +} +``` + +### When to provide Try-Parse: +- Parsing user input (strings to values) +- Dictionary lookups (`TryGetValue`) +- Any operation where failure is a common, non-exceptional scenario + +## Exception Messages + +Established convention: exception messages describe what went wrong and often hint at what to do: + +```csharp +// Good messages +"Stream does not support reading." +"Non-negative number required. (Parameter 'count')" +"Collection was modified; enumeration operation may not execute." +"Index was out of range. Must be non-negative and less than the size of the collection." +``` + +**Characteristics of good exception messages:** +- Complete sentences with proper punctuation +- State the problem clearly +- Include relevant values when possible +- Don't expose internal implementation details + +## Async Exception Patterns + +Validate arguments synchronously (before the first `await`) so callers get immediate feedback: + +```csharp +public Task ReadFileAsync(string path, CancellationToken cancellationToken) +{ + // Validate BEFORE async work — throws immediately + ArgumentNullException.ThrowIfNull(path); + return ReadFileCoreAsync(path, cancellationToken); +} + +private async Task ReadFileCoreAsync(string path, CancellationToken ct) +{ + ct.ThrowIfCancellationRequested(); + return await File.ReadAllTextAsync(path, ct); +} +``` + +## Exception Builder Pattern + +Use helper methods to throw exceptions, keeping call sites small enough for JIT inlining: + +```csharp +// Common pattern for hot paths +private static void ThrowInvalidOperation() + => throw new InvalidOperationException("Enumeration already finished."); + +public bool MoveNext() +{ + if (_index >= _count) + { + ThrowInvalidOperation(); // Keeps MoveNext small for inlining + } + // ... +} +``` + +## Methods That Should Not Throw + +Established convention: these methods avoid throwing exceptions: + +| Method | Why | +|--------|-----| +| `Equals(object)` | Used in comparisons, hash tables — must be safe | +| `GetHashCode()` | Used in dictionaries, hash sets — must be safe | +| `ToString()` | Used in debugging, logging — must be safe | +| `Dispose()` | Cleanup should never fail | +| `==` / `!=` operators | Must behave like `Equals` | +| Static constructors | Exception causes `TypeInitializationException`, unrecoverable | + +## Error Handling Checklist + +- [ ] Standard exception types used (not `Exception` or `SystemException`) +- [ ] `paramName` set on all `ArgumentException` subtypes +- [ ] `ThrowIfNull`, `ThrowIfNegative`, etc. used where available (.NET 6+) +- [ ] Exception messages are clear and describe the problem +- [ ] Try-Parse pattern provided for commonly-failing operations +- [ ] Arguments validated synchronously in async methods +- [ ] `Equals`, `GetHashCode`, `ToString` do not throw +- [ ] Exception builder methods used in hot paths (for JIT inlining) +- [ ] `CancellationToken.ThrowIfCancellationRequested()` used for cancellation diff --git a/skills/dotnet-api-design-cop/references/member-design-patterns.md b/skills/dotnet-api-design-cop/references/member-design-patterns.md new file mode 100644 index 0000000000..3595338be4 --- /dev/null +++ b/skills/dotnet-api-design-cop/references/member-design-patterns.md @@ -0,0 +1,266 @@ +# Member Design Patterns Reference + +Established C# member design conventions. + +## Properties vs Methods + +There is a clear line between properties (state access) and methods (operations). + +### Use Properties When: +- Access is cheap (field-like) +- Calling twice returns the same value +- No observable side effects +- Represents a logical attribute of the type + +```csharp +// System.IO.Stream — properties for state +public abstract long Length { get; } +public abstract long Position { get; set; } +public abstract bool CanRead { get; } +public abstract bool CanSeek { get; } + +// System.Collections.Generic.List +public int Count { get; } +public int Capacity { get; set; } +``` + +### Use Methods When: +- The operation is a conversion (`ToString()`, `ToArray()`) +- The call is expensive or has side effects +- Different results each time (`DateTime.Now` is a well-known exception) +- Returns a new object or array + +```csharp +// Conversions — always methods +public override string ToString(); +public T[] ToArray(); +public List ToList(); + +// Operations with side effects — always methods +public int Read(byte[] buffer, int offset, int count); +public void Write(byte[] buffer, int offset, int count); + +// Expensive operations — always methods +public DataTable GetSchemaTable(); +public byte[] ComputeHash(byte[] buffer); +``` + +## Method Overloading Patterns + +Established convention: overloads form a progressive series from simplest to most complete. + +### StringBuilder.Append Pattern +```csharp +// Simplest → most complete, all consistent +public StringBuilder Append(string value); +public StringBuilder Append(string value, int startIndex, int count); +public StringBuilder Append(char value); +public StringBuilder Append(char value, int repeatCount); +``` + +### Stream.Read Pattern +```csharp +// Modern .NET adds Span overloads alongside array overloads +public abstract int Read(byte[] buffer, int offset, int count); +public virtual int Read(Span buffer); +``` + +### Console.WriteLine Pattern +```csharp +// Many overloads, all following the same naming +public static void WriteLine(); +public static void WriteLine(string value); +public static void WriteLine(string format, object arg0); +public static void WriteLine(string format, object arg0, object arg1); +public static void WriteLine(string format, params object[] arg); +``` + +**Key patterns:** +1. Parameter order is consistent across all overloads +2. Simpler overloads delegate to the most complete one +3. Parameter names are identical across overloads +4. `CancellationToken` is always the last parameter +5. `params` array overload is the most flexible variant + +## Constructor Patterns + +### Simple Instantiation +The convention supports creating instances with minimal ceremony: + +```csharp +// Default constructor — ready to use +var sb = new StringBuilder(); +var list = new List(); + +// Parameterized — for required values +var uri = new Uri("https://example.com"); +var fs = new FileStream(path, FileMode.Open); + +// Common pattern: overloads from minimal to full +public StringBuilder(); +public StringBuilder(string value); +public StringBuilder(int capacity); +public StringBuilder(string value, int capacity); +``` + +### Argument Validation in Constructors +```csharp +public FileStream(string path, FileMode mode) +{ + ArgumentNullException.ThrowIfNull(path); + ArgumentException.ThrowIfNullOrEmpty(path); + // ... +} +``` + +## Event Patterns + +Established event design conventions (as seen in `FileSystemWatcher`, `ObservableCollection`, etc.): + +### Standard Pattern +```csharp +// 1. Define EventArgs if needed +public class FileChangedEventArgs : EventArgs +{ + public string FileName { get; } + public WatcherChangeTypes ChangeType { get; } + + public FileChangedEventArgs(string fileName, WatcherChangeTypes changeType) + { + FileName = fileName; + ChangeType = changeType; + } +} + +// 2. Declare event using EventHandler +public event EventHandler FileChanged; + +// 3. Raise through protected virtual method +protected virtual void OnFileChanged(FileChangedEventArgs e) +{ + FileChanged?.Invoke(this, e); +} +``` + +### Established Conventions: +- `EventHandler` is the standard delegate type +- Raising method is named `On` +- Raising method is `protected virtual` for extensibility +- `EventArgs.Empty` used when no data is needed +- EventArgs properties are typically read-only + +## Operator Overloading Patterns + +Operators are overloaded only on types with natural mathematical or comparison semantics: + +```csharp +// DateTime — subtraction produces TimeSpan +public static TimeSpan operator -(DateTime d1, DateTime d2); +public static DateTime operator +(DateTime d, TimeSpan t); + +// Decimal — full arithmetic operators +public static decimal operator +(decimal d1, decimal d2); +public static decimal operator -(decimal d1, decimal d2); +public static bool operator ==(decimal d1, decimal d2); +public static bool operator !=(decimal d1, decimal d2); +``` + +**Operator conventions:** +- Operators always come in pairs (`==`/`!=`, `<`/`>`, `<=`/`>=`) +- `IEquatable` is implemented alongside `==`/`!=` +- `GetHashCode()` is overridden whenever `Equals()` is +- Named method equivalents exist (`Add`, `Subtract`, `Equals`, `CompareTo`) + +## IEquatable Pattern + +As seen in `DateTime`, `Guid`, `Int32`, etc.: + +```csharp +public readonly struct Money : IEquatable +{ + public decimal Amount { get; } + public string Currency { get; } + + public bool Equals(Money other) + => Amount == other.Amount && Currency == other.Currency; + + public override bool Equals(object? obj) + => obj is Money other && Equals(other); + + public override int GetHashCode() + => HashCode.Combine(Amount, Currency); + + public static bool operator ==(Money left, Money right) => left.Equals(right); + public static bool operator !=(Money left, Money right) => !left.Equals(right); +} +``` + +## IComparable Pattern + +As seen in `String`, `DateTime`, `Int32`: + +```csharp +public readonly struct Version : IComparable, IEquatable +{ + public int Major { get; } + public int Minor { get; } + + public int CompareTo(Version other) + { + int result = Major.CompareTo(other.Major); + return result != 0 ? result : Minor.CompareTo(other.Minor); + } +} +``` + +## ToString Pattern + +Every type should override `ToString()` with a human-readable representation: + +```csharp +// DateTime +public override string ToString() => "2/12/2026 2:39:17 AM"; + +// Guid +public override string ToString() => "d85b1407-351d-4694-9392-03acc5870eb1"; + +// Custom types should follow the same pattern +public override string ToString() => $"{Name} ({Count} items)"; +``` + +## Virtual Member Patterns + +Virtual members should be used deliberately, not speculatively: + +```csharp +public class HttpMessageHandler +{ + // Virtual: designed as customization point + protected internal virtual HttpResponseMessage Send( + HttpRequestMessage request, CancellationToken cancellationToken); +} + +public abstract class Stream +{ + // Abstract: MUST be implemented + public abstract int Read(byte[] buffer, int offset, int count); + + // Virtual: CAN be overridden (has default implementation) + public virtual void CopyTo(Stream destination, int bufferSize) { /* default */ } + + // Non-virtual: fixed behavior + public void Dispose() { /* fixed cleanup workflow */ } +} +``` + +## Member Design Checklist + +- [ ] Properties are cheap and idempotent; methods are used for operations +- [ ] Overloads have consistent parameter order and naming +- [ ] Constructors support simple instantiation for common cases +- [ ] Events use `EventHandler` pattern +- [ ] No public fields (properties used instead) +- [ ] Operators come in pairs with named equivalents +- [ ] `IEquatable` implemented with `GetHashCode` and `==`/`!=` +- [ ] `ToString()` provides meaningful human-readable output +- [ ] Virtual members have clear extensibility purpose diff --git a/skills/dotnet-api-design-cop/references/naming-conventions.md b/skills/dotnet-api-design-cop/references/naming-conventions.md new file mode 100644 index 0000000000..ad9c93a38c --- /dev/null +++ b/skills/dotnet-api-design-cop/references/naming-conventions.md @@ -0,0 +1,177 @@ +# Naming Conventions Reference + +Established C# naming conventions for public API design. + +## Casing + +### PascalCase + +Used for all public identifiers except parameters. Every word starts with an uppercase letter. + +```csharp +// Types +public class StreamReader { } +public struct DateTime { } +public interface IEnumerable { } +public enum ConsoleColor { } + +// Members +public int Count { get; } +public void ReadLine() { } +public event EventHandler Click; +public const int MaxValue = int.MaxValue; +``` + +### camelCase + +Used for parameters and local variables (also for private fields with `_` prefix). + +```csharp +public void CopyTo(Stream destination, int bufferSize) { } +private int _count; +private static TimeSpan s_defaultTimeout; +``` + +## Acronym Casing + +Established convention: two-letter acronyms stay uppercase, three+ letters use PascalCase. + +| Acronym | Example | Pattern | +|---------|--------|---------| +| IO | `System.IO` | Two letters → uppercase | +| UI | `UIElement` | Two letters → uppercase | +| DB | `DbConnection` | Two letters → uppercase (note: newer APIs use `Db`) | +| Html | `HtmlWriter` | Three letters → PascalCase | +| Xml | `XmlReader` | Three letters → PascalCase | +| Json | `JsonSerializer` | Four letters → PascalCase | +| Url | `UrlEncoder` | Three letters → PascalCase | + +## Type Name Patterns + +### Classes and Structs — Nouns + +Use noun or noun phrase names: +- `FileStream`, `StringBuilder`, `HttpClient`, `MemoryCache` +- `DateTime`, `TimeSpan`, `Guid`, `Color` + +### Interfaces — `I` Prefix + Adjective/Noun + +Established convention: +- `IDisposable`, `IComparable`, `IFormattable` (adjectives) +- `IEnumerable`, `ICollection`, `IList` (nouns) +- `IServiceProvider`, `ICustomFormatter` (noun phrases) + +### Type Parameters — `T` Prefix + +```csharp +// Single type param: just T +public class List { } +public interface IComparer { } + +// Multiple or constrained: T + descriptive name +public class Dictionary { } +public interface ISessionChannel where TSession : ISession { } +``` + +### Suffixes + +| When type... | Suffix | Examples | +|-------------|--------|-------------| +| Derives from `Exception` | `Exception` | `ArgumentNullException`, `IOException` | +| Derives from `Attribute` | `Attribute` | `ObsoleteAttribute`, `SerializableAttribute` | +| Derives from `EventArgs` | `EventArgs` | `CancelEventArgs`, `PropertyChangedEventArgs` | +| Represents a collection | `Collection` | `ObservableCollection`, `KeyedCollection` | +| Represents a dictionary | `Dictionary` | `ConcurrentDictionary`, `SortedDictionary` | + +## Method Names — Verbs + +Methods use verbs or verb phrases: + +```csharp +// System.IO.Stream +public abstract int Read(byte[] buffer, int offset, int count); +public abstract void Write(byte[] buffer, int offset, int count); +public virtual void CopyTo(Stream destination); +public virtual void Close(); + +// System.String +public int CompareTo(string value); +public bool Contains(string value); +public string Replace(string oldValue, string newValue); +public string[] Split(char separator); +``` + +Async methods add `Async` suffix: +```csharp +public Task ReadAsync(byte[] buffer, int offset, int count); +public Task WriteAsync(byte[] buffer, int offset, int count); +``` + +## Property Names — Nouns/Adjectives + +```csharp +// Nouns +public int Count { get; } +public int Length { get; } +public string Name { get; set; } +public Stream BaseStream { get; } + +// Boolean with Is/Can/Has +public bool IsReadOnly { get; } +public bool CanRead { get; } +public bool CanSeek { get; } +public bool HasValue { get; } +public bool IsCompleted { get; } +``` + +## Event Names — Verb Tense + +Use present participle (gerund) for events that fire before/during, and past tense for events that fire after: + +| Pre-event | Post-event | +|-----------|-----------| +| `Closing` | `Closed` | +| `Validating` | `Validated` | +| `PropertyChanging` | `PropertyChanged` | +| `CollectionChanging` | N/A (some types omit pre-event) | + +## Enum Names + +Non-flag enums use singular nouns: +```csharp +public enum ConsoleColor { Black, Blue, Green, ... } +public enum DayOfWeek { Sunday, Monday, ... } +public enum FileMode { Create, Open, Append, ... } +``` + +Flag enums use plural nouns and `[Flags]`: +```csharp +[Flags] +public enum FileAttributes { ReadOnly = 1, Hidden = 2, System = 4, ... } + +[Flags] +public enum BindingFlags { Default = 0, Instance = 4, Static = 8, Public = 16, ... } +``` + +## Namespace Patterns + +Established convention: `.[.]` + +``` +System.Collections.Generic +System.IO.Compression +System.Net.Http +System.Text.Json +Microsoft.Extensions.Logging +Microsoft.Extensions.DependencyInjection +``` + +## What to Avoid + +These patterns should never appear in public APIs: + +- Hungarian notation (`strName`, `iCount`, `bEnabled`) +- Underscores in public names (`Get_Value`, `Max_Count`) +- Abbreviations (`Btn`, `Msg`, `Mgr` — except universally known ones like `IO`) +- Names differing only by case +- Language-specific type names in methods (`GetInt` vs `GetInt32`) diff --git a/skills/dotnet-api-design-cop/references/type-design-patterns.md b/skills/dotnet-api-design-cop/references/type-design-patterns.md new file mode 100644 index 0000000000..f0d186f460 --- /dev/null +++ b/skills/dotnet-api-design-cop/references/type-design-patterns.md @@ -0,0 +1,199 @@ +# Type Design Patterns Reference + +Established C# type design conventions. + +## When to Use Structs + +Use structs for small, immutable types that represent single values. + +| Struct | Size | Characteristics | +|--------|------|----------------| +| `Int32` | 4 bytes | Primitive value | +| `DateTime` | 8 bytes | Immutable, value semantics | +| `TimeSpan` | 8 bytes | Immutable, value semantics | +| `Guid` | 16 bytes | Immutable, value identity | +| `Decimal` | 16 bytes | Immutable, numeric value | +| `Point` | 8 bytes | Immutable, coordinate pair | +| `Color` | 4 bytes | Immutable, ARGB value | +| `CancellationToken` | 8 bytes | Lightweight, passed by value | +| `ReadOnlySpan` | 16 bytes | ref struct, zero-allocation view | + +**Consistent struct characteristics:** +- Small (≤ 16 bytes typically) +- Immutable (use `readonly struct`) +- Represent a single logical value +- Value equality semantics (`Equals`/`GetHashCode` based on content) +- No inheritance needed +- Rarely boxed in typical usage + +```csharp +// Established struct pattern +public readonly struct Point : IEquatable +{ + public double X { get; } + public double Y { get; } + + public Point(double x, double y) => (X, Y) = (x, y); + + public bool Equals(Point other) => X == other.X && Y == other.Y; + public override bool Equals(object? obj) => obj is Point other && Equals(other); + public override int GetHashCode() => HashCode.Combine(X, Y); +} +``` + +## When to Use Classes + +Use classes for types with identity, complex behavior, large size, or inheritance. + +| Class | Why Not Struct | +|-------|---------------| +| `String` | Variable size, reference semantics, sealed | +| `Stream` | Abstract base, many subclasses, manages resources | +| `HttpClient` | Complex state, disposable, large | +| `List` | Mutable, variable size, reference semantics | +| `Exception` | Inheritance hierarchy, reference identity | +| `Task` | Shared state, awaited from multiple locations | + +## When to Use Interfaces + +Interfaces are used for cross-hierarchy contracts that multiple unrelated types implement. + +```csharp +// IDisposable — implemented by classes (Stream, HttpClient) and some structs +public interface IDisposable +{ + void Dispose(); +} + +// IEnumerable — implemented by List, Array, Dictionary, etc. +public interface IEnumerable : IEnumerable +{ + IEnumerator GetEnumerator(); +} + +// IComparable — implemented by String, Int32, DateTime, etc. +public interface IComparable +{ + int CompareTo(T other); +} +``` + +**Interface conventions:** +- Every interface should have multiple implementations +- Interfaces are consumed by other APIs (`IEnumerable` consumed by LINQ) +- New interfaces are added cautiously (adding members breaks implementors) +- `I` prefix is universal and mandatory + +## Abstract Class Patterns + +Use abstract classes when shared implementation is needed alongside enforced customization: + +```csharp +// Stream — abstract base with shared logic + abstract customization points +public abstract class Stream : IDisposable, IAsyncDisposable +{ + // Abstract: derived types MUST implement + public abstract int Read(byte[] buffer, int offset, int count); + public abstract void Write(byte[] buffer, int offset, int count); + public abstract long Length { get; } + + // Virtual: shared default with optional override + public virtual void CopyTo(Stream destination) { /* default impl */ } + public virtual void Close() { Dispose(true); } + + // Concrete: shared behavior + public void Dispose() { Dispose(true); GC.SuppressFinalize(this); } +} +``` + +## Enum Patterns + +### Non-Flag Enums (Singular Noun) +```csharp +public enum ConsoleColor +{ + Black = 0, + DarkBlue = 1, + DarkGreen = 2, + // ... +} + +public enum FileMode +{ + CreateNew = 1, + Create = 2, + Open = 3, + OpenOrCreate = 4, + Truncate = 5, + Append = 6, +} +``` + +### Flag Enums (Plural Noun + `[Flags]` + Powers of Two) +```csharp +[Flags] +public enum FileAttributes +{ + ReadOnly = 0x0001, + Hidden = 0x0002, + System = 0x0004, + Directory = 0x0010, + Archive = 0x0020, + Normal = 0x0080, +} + +// Usage: var attrs = FileAttributes.ReadOnly | FileAttributes.Hidden; +``` + +## Sealed vs Unsealed + +Established convention: most types are unsealed. Sealing is used selectively. + +| Sealed | Why | +|--------|-----| +| `String` | Immutable invariants, security | +| `AesGcm` | Cryptographic safety, prevent insecure overrides | +| `Tuple` | No extensibility needed | + +| Unsealed | Why | +|----------|-----| +| `Stream` | Designed for subclassing | +| `HttpMessageHandler` | Extensibility point | +| `Collection` | Designed for customization | +| `Exception` | Custom exception types derive from it | + +## Static Class Patterns + +Use static classes as utility containers: + +```csharp +public static class Math +{ + public static double Sqrt(double d) { ... } + public static int Max(int val1, int val2) { ... } +} + +public static class Console +{ + public static void WriteLine(string value) { ... } + public static string ReadLine() { ... } +} + +public static class Path +{ + public static string Combine(string path1, string path2) { ... } + public static string GetExtension(string path) { ... } +} +``` + +## Type Design Checklist + +- [ ] Struct types are small (≤ 16 bytes), immutable, and represent single values +- [ ] No mutable structs +- [ ] Structs implement `IEquatable` with value-based equality +- [ ] Interfaces have multiple implementations and consumers +- [ ] Abstract classes provide shared implementation + customization points +- [ ] Flag enums have `[Flags]` attribute and power-of-two values +- [ ] Non-flag enums use singular names, flag enums use plural names +- [ ] Types are unsealed unless there's a specific reason to seal +- [ ] Static classes have a cohesive, single-purpose API surface From d663292811ce232a56b1a762665c95c4e1cacfc5 Mon Sep 17 00:00:00 2001 From: artl Date: Fri, 13 Feb 2026 16:07:59 -0800 Subject: [PATCH 2/5] Apply best practices to API design agent and skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent (126→43 lines, -66%): - Rename to reviewing-dotnet-api-design (gerund form) - Rewrite description: third person, no trigger phrases/examples - Remove redundant sections: Mission, Core Design Philosophy, Review Methodology (duplicated skill), When to Ask, Tone - Keep: key principles, output format, skill reference, escalation SKILL.md (360→109 lines, -70%): - Rename to reviewing-dotnet-api-design (gerund form) - Convert from inline conventions to router pattern - Steps 2-5 now point to reference files instead of duplicating content - Remove When to Use, When Not to Use, Failure Modes - Consolidate validation checklist and common pitfalls Reference files (1,003→689 lines, -31%): - naming-conventions.md: 177→71 (-60%) — removed prose, kept tables - type-design-patterns.md: 199→135 (-32%) — removed explanations Claude knows - member-design-patterns.md: 266→157 (-41%) — kept non-obvious patterns - error-handling-patterns.md: 195→176 (-10%) — light trim, unique value - api-review-checklist.md: 166→150 (-10%) — trimmed process prose Total: 1,489→841 lines (-44%) --- agents/dotnet-api-design-reviewer.agent.md | 126 ------ agents/reviewing-dotnet-api-design.agent.md | 43 +++ skills/dotnet-api-design-cop/SKILL.md | 360 ------------------ .../references/naming-conventions.md | 177 --------- skills/reviewing-dotnet-api-design/SKILL.md | 109 ++++++ .../references/api-review-checklist.md | 16 - .../references/error-handling-patterns.md | 39 +- .../references/member-design-patterns.md | 121 +----- .../references/naming-conventions.md | 71 ++++ .../references/type-design-patterns.md | 74 +--- 10 files changed, 244 insertions(+), 892 deletions(-) delete mode 100644 agents/dotnet-api-design-reviewer.agent.md create mode 100644 agents/reviewing-dotnet-api-design.agent.md delete mode 100644 skills/dotnet-api-design-cop/SKILL.md delete mode 100644 skills/dotnet-api-design-cop/references/naming-conventions.md create mode 100644 skills/reviewing-dotnet-api-design/SKILL.md rename skills/{dotnet-api-design-cop => reviewing-dotnet-api-design}/references/api-review-checklist.md (88%) rename skills/{dotnet-api-design-cop => reviewing-dotnet-api-design}/references/error-handling-patterns.md (79%) rename skills/{dotnet-api-design-cop => reviewing-dotnet-api-design}/references/member-design-patterns.md (53%) create mode 100644 skills/reviewing-dotnet-api-design/references/naming-conventions.md rename skills/{dotnet-api-design-cop => reviewing-dotnet-api-design}/references/type-design-patterns.md (65%) diff --git a/agents/dotnet-api-design-reviewer.agent.md b/agents/dotnet-api-design-reviewer.agent.md deleted file mode 100644 index 9c83ead569..0000000000 --- a/agents/dotnet-api-design-reviewer.agent.md +++ /dev/null @@ -1,126 +0,0 @@ ---- -description: "Use this agent when the user wants to review, design, or improve .NET API surfaces for consistency with established C# conventions.\n\nTrigger phrases include:\n- 'review my API design'\n- 'is this API consistent with .NET conventions?'\n- 'check my public API surface'\n- 'help me design this .NET API'\n- 'review my naming conventions'\n- 'should this be a class or struct?'\n- 'is this a breaking change?'\n- 'prepare an API proposal'\n- 'check my exception design'\n- 'review my library API'\n\nExamples:\n- User asks 'Does my API follow .NET naming conventions?' → invoke this agent to review naming against C# conventions\n- User shares a class and asks 'Is this API well-designed?' → invoke this agent to perform a full API design review\n- User asks 'Should I use a class or struct for this type?' → invoke this agent to analyze type design\n- User says 'I need to add a new overload without breaking existing callers' → invoke this agent to assess breaking change risk\n- User asks 'Review this API proposal before I submit it' → invoke this agent to apply the full API review checklist" -name: dotnet-api-design-reviewer -tools: ['shell', 'read', 'search', 'edit', 'task', 'skill', 'web_search', 'web_fetch', 'ask_user'] ---- - -# dotnet-api-design-reviewer instructions - -You are a senior .NET API design reviewer with deep expertise in established C# conventions and patterns. You care deeply about naming precision, type design choices, member design patterns, and the developer experience of consuming APIs. - -## Your Mission - -Help developers design and review .NET API surfaces that are consistent with established C# conventions by: -- Reviewing public API surfaces for naming, type design, member design, and pattern conformance -- Guiding type design decisions (class vs struct vs interface vs enum) -- Evaluating member design (properties vs methods, overloading patterns, parameter design) -- Checking error handling patterns -- Assessing extensibility mechanisms -- Validating collection usage in public APIs -- Identifying breaking changes and versioning risks -- Preparing and reviewing API proposals - -You do NOT cite or reference the Pearson-licensed "Framework Design Guidelines" book or the learn.microsoft.com/en-us/dotnet/standard/design-guidelines/ pages. - -## Core Design Philosophy - -1. **Caller-first design**: Always start with the code a developer will write. If the calling code is awkward, the API needs work — regardless of how clean the implementation is. - -2. **Consistency**: Follow established C# conventions so developers can transfer their existing knowledge to new APIs without surprises. - -3. **Progressive disclosure**: Simple things should be simple. Advanced things should be possible. The simplest overload handles 80% of use cases. - -4. **Hard to misuse**: The correct usage should be easier than the incorrect usage. The "pit of success" should be wide. - -5. **Additive evolution**: APIs can only be added, never removed without breaking consumers. Be conservative in what you expose — you can always add later. - -## Review Methodology - -### 1. Scenario Assessment -Write calling code for the top scenarios. If the code is clean in 3-5 lines, the design is on track. - -### 2. Naming Review -Check against established C# naming conventions: -- PascalCase types/methods/properties, camelCase parameters -- Verbs for methods, nouns for properties -- I-prefix interfaces, T-prefix type parameters -- Standard suffixes (Exception, Attribute, EventArgs, Collection) -- No abbreviations, no Hungarian notation - -### 3. Type Design Review -Apply established type choice conventions: -- Structs: small, immutable, value semantics (like DateTime, TimeSpan, Guid) -- Classes: identity, complex behavior, inheritance (like Stream, HttpClient) -- Interfaces: cross-hierarchy contracts (like IDisposable, IEnumerable) -- Enums: singular for non-flags, plural with [Flags] for flags - -### 4. Member Design Review -Check against established member patterns: -- Properties for state, methods for operations -- Consistent overloading patterns -- EventHandler for events -- No public fields -- CancellationToken always last - -### 5. Error Handling Review -Check error handling conventions: -- Standard exception types with paramName -- Try-Parse pattern for commonly-failing operations -- ThrowIf helpers (.NET 6+) -- Synchronous argument validation in async methods - -### 6. Collection Review -Check collection conventions: -- Collection/ReadOnlyCollection for public APIs (not List) -- IEnumerable for parameters -- Empty, not null, for empty collections - -### 7. Breaking Change Assessment -Identify anything that would break existing consumers. - -## Output Format - -When reviewing APIs: - -1. **Summary Assessment**: Brief overall evaluation -2. **Issues Found**: Categorized by severity - - **Critical**: Patterns that contradict established conventions (mutable structs, List in public API, bare Exception throwing) - - **Warning**: Deviations from common C# patterns - - **Suggestion**: Polish improvements -3. **For each issue**: What the convention is, what the code does, recommended fix with code example -4. **Strengths**: What's done well -5. **Scenario Test**: Sample calling code to validate usability - -When designing new APIs: - -1. **Scenario Code First**: Show calling code -2. **Proposed API Surface**: Type/member listing -3. **Design Rationale**: Why specific choices were made -4. **Breaking Change Risk**: If modifying existing APIs - -## Skills - -- **dotnet-api-design-cop**: Load for the comprehensive API design review workflow, checklists, and reference materials covering naming conventions, type design patterns, member design patterns, error handling patterns, and the API review checklist. - -## When to Ask for Clarification - -- If you don't know whether the API is for a library, framework, or application -- If the target .NET version is unclear -- If the review scope is ambiguous -- If multiple valid designs exist and you need to understand priorities -- If you need the existing API surface to assess breaking changes - -## Escalation - -Acknowledge when the question is better handled by another agent: -- Implementation performance → `dotnet-jit-expert` -- Async pattern correctness → `dotnet-async-patterns` / `dotnet-concurrency-expert` -- Synchronization primitives → `dotnet-sync-primitives` - -## Tone - -- **Precise**: Every recommendation references established conventions -- **Consistent**: Same standards applied uniformly -- **Pragmatic**: Conventions have context; explain when deviation is reasonable -- **Educational**: Explain the reasoning so developers internalize the patterns -- **Constructive**: Praise good choices alongside identifying issues diff --git a/agents/reviewing-dotnet-api-design.agent.md b/agents/reviewing-dotnet-api-design.agent.md new file mode 100644 index 0000000000..49cab2370d --- /dev/null +++ b/agents/reviewing-dotnet-api-design.agent.md @@ -0,0 +1,43 @@ +--- +description: "Reviews .NET API surfaces for consistency with established C# conventions covering naming, type design, member design, error handling, collections, extensibility, and breaking changes. Use when reviewing public API designs, preparing API proposals, or making type design decisions like class vs struct." +name: reviewing-dotnet-api-design +tools: ['shell', 'read', 'search', 'edit', 'task', 'skill', 'web_search', 'web_fetch', 'ask_user'] +--- + +# reviewing-dotnet-api-design + +You are a senior .NET API design reviewer. Help developers design and review .NET API surfaces that are consistent with established C# conventions. + +You do NOT cite or reference the Pearson-licensed "Framework Design Guidelines" book or the learn.microsoft.com/en-us/dotnet/standard/design-guidelines/ pages. + +## Key Principles + +1. **Caller-first design**: Start with the code a developer will write. If calling code is awkward, the API needs work. +2. **Additive evolution**: APIs can only be added, never removed. Be conservative in what you expose. + +## Output Format + +### When Reviewing APIs + +1. **Summary Assessment**: Brief overall evaluation +2. **Issues Found** (by severity): + - **Critical**: Contradicts established conventions (mutable structs, `List` in public API, bare `Exception`) + - **Warning**: Deviates from common C# patterns + - **Suggestion**: Polish improvements +3. **For each issue**: Convention → what code does → recommended fix +4. **Strengths**: What's done well +5. **Scenario Test**: Calling code for top 2-3 scenarios + +### When Designing New APIs + +1. **Scenario Code First**: Show calling code +2. **Proposed API Surface**: Type/member listing +3. **Breaking Change Risk**: If modifying existing APIs + +## Skills + +- **reviewing-dotnet-api-design**: Load for the full API design review workflow with checklists and reference materials covering naming, type design, member design, error handling, and the API review checklist. + +## Escalation + +Acknowledge when the issue is better handled by a performance, async, or concurrency specialist. Provide guidance on the right approach rather than forcing an API design angle. diff --git a/skills/dotnet-api-design-cop/SKILL.md b/skills/dotnet-api-design-cop/SKILL.md deleted file mode 100644 index 26730a5557..0000000000 --- a/skills/dotnet-api-design-cop/SKILL.md +++ /dev/null @@ -1,360 +0,0 @@ ---- -name: dotnet-api-design-cop -description: Reviews .NET API designs for consistency with established C# conventions. Use when reviewing public API surfaces, designing new library APIs, or checking naming, type choices, member design, error handling, and extensibility patterns against established C# conventions. ---- - -# .NET API Design Review - -This skill provides actionable guidance for reviewing .NET API designs against established C# conventions. It covers naming conventions, type design choices, member design, error handling, extensibility, collection usage, and resource management patterns. - -## When to Use - -- Reviewing a public API surface for consistency with C# conventions -- Designing new types, methods, properties, or events for a .NET library -- Preparing an API proposal for review -- Checking naming consistency across a namespace or assembly -- Evaluating type design choices (class vs struct vs interface) -- Reviewing error handling and exception usage -- Assessing extensibility mechanisms -- Validating collection usage in public APIs -- Checking IDisposable implementation patterns -- Evaluating breaking change risk in API modifications - -## When Not to Use - -- Performance optimization (use `dotnet-jit-optimization` skill) -- Async/await pattern correctness (use `dotnet-async-patterns` skill) -- Synchronization primitive selection (use `dotnet-sync-primitives` skill) -- Internal/private code review (these conventions target public API surfaces) -- REST/HTTP API endpoint design (different domain) - -## Inputs - -| Input | Required | Description | -|-------|----------|-------------| -| Code under review | Yes | The API surface to analyze (types, members, signatures) | -| Context | Recommended | Library, framework, or application? NuGet package or internal? | -| .NET version | Recommended | Target framework version | -| Scope | Recommended | Full review or focused area (naming, types, errors, etc.) | - -## Workflow - -### Step 1: Classify the API surface and write caller code - -First, classify what you are reviewing: - -| Surface type | Focus areas | Key risk | -|-------------|-------------|----------| -| New library API | Full review — naming, types, members, errors | Getting the shape wrong before anyone depends on it | -| Extension to existing API | Consistency with existing surface, overload patterns | Inconsistency with the established conventions of the library | -| Modification to existing API | Breaking change assessment first, then design | Breaking existing consumers | -| API proposal | Scenario code, then full review | Approving a surface that is hard to use | - -Then write sample code that uses the API for its top 2-3 scenarios. - -Ask: -1. Can a developer accomplish the main task in a few lines? -2. Is there one obvious type where usage starts? -3. Can users create an instance, set properties, and call methods without complex initialization? -4. Would IntelliSense lead developers to the right type? - -If the calling code is awkward, the API design needs work — regardless of how clean the implementation is. - -### Step 2: Review naming conventions - -These are established C# naming conventions. - -**Casing rules:** - -| Element | Convention | Examples | -|---------|-----------|-------------| -| Types | PascalCase | `StreamReader`, `StringBuilder`, `HttpClient` | -| Methods | PascalCase | `ReadLine()`, `GetHashCode()`, `ToString()` | -| Properties | PascalCase | `Length`, `Count`, `IsReadOnly` | -| Events | PascalCase | `Click`, `PropertyChanged`, `Closed` | -| Parameters | camelCase | `buffer`, `index`, `cancellationToken` | -| Constants | PascalCase | `MaxValue`, `Empty` | -| Interfaces | `I` + PascalCase | `IDisposable`, `IEnumerable`, `IComparable` | -| Type parameters | `T` + PascalCase | `TKey`, `TValue`, `TResult` (or just `T` for single param) | - -**Naming patterns:** - -| Pattern | Convention | Examples | -|---------|-----------|-------------| -| Methods | Verb or verb phrase | `Read`, `Write`, `Parse`, `CompareTo` | -| Properties | Noun, noun phrase, or adjective | `Name`, `Count`, `IsReadOnly`, `HasValue` | -| Boolean properties | Affirmative phrasing, often `Is`/`Can`/`Has` prefix | `IsEnabled`, `CanRead`, `HasValue` | -| Events | Verb tense (gerund for pre, past for post) | `Closing`/`Closed`, `Validating`/`Validated` | -| Exception types | End with `Exception` | `ArgumentNullException`, `IOException` | -| Attribute types | End with `Attribute` | `SerializableAttribute`, `ObsoleteAttribute` | -| EventArgs types | End with `EventArgs` | `CancelEventArgs`, `PropertyChangedEventArgs` | -| Enum (non-flag) | Singular noun | `ConsoleColor`, `DayOfWeek`, `FileMode` | -| Enum (flag) | Plural noun with `[Flags]` | `FileAttributes`, `BindingFlags` | - -**Common violations to flag:** -- Abbreviations or contractions in public names -- Hungarian notation (`strName`, `iCount`) -- Underscores in public member names -- Language-specific type names in method names (`GetInt` instead of `GetInt32`) -- Method names using nouns instead of verbs -- Property names using verbs instead of nouns - -**Before/after example — naming review correction:** -```csharp -// BEFORE: Multiple naming violations -public class data_processor // underscore, vague name -{ - public int GetInt(string s) { ... } // language-specific type name, cryptic parameter - public bool process() { ... } // lowercase, but also: does this return success? - public string strName { get; set; } // Hungarian notation -} - -// AFTER: Consistent naming -public class DataParser // PascalCase, specific noun -{ - public int GetInt32(string text) { ... } // framework type name, descriptive parameter - public DataResult Parse() { ... } // PascalCase verb, clear return type - public string Name { get; set; } // no prefix, noun -} -``` - -### Step 3: Review type design choices - -**When to use structs:** -`DateTime`, `TimeSpan`, `Guid`, `Point`, `Color`, `Decimal`, `Int32` — small, immutable types representing single values. - -Struct suitability checklist: -- Logically represents a single value -- Instance size ≤ 16 bytes -- Immutable (or `readonly struct`) -- No need for inheritance -- Frequently allocated (value type avoids heap allocation) - -**When to use classes:** -`String`, `Stream`, `HttpClient`, `List` — types with identity, complex behavior, inheritance, or large size. - -**When to use interfaces:** -`IEnumerable`, `IDisposable`, `IComparable` — cross-hierarchy contracts that both classes and structs implement. - -**Check for:** -- Mutable structs (these cause subtle bugs with value-copy semantics) -- Interfaces without any implementation in the library -- Enums missing `[Flags]` when values are combinable -- Overly sealed types (`String` is sealed, `Stream` is not — seal deliberately) - -**Before/after example — type design correction:** -```csharp -// BEFORE: Mutable struct with reference semantics -public struct Connection -{ - public string Host { get; set; } // mutable - public int Port { get; set; } // mutable - public List Tags { get; set; } // reference type in struct - public void Connect() { ... } // side-effecting method on value type -} - -// AFTER: Class (has identity, side effects, reference-type fields) -public class Connection : IDisposable -{ - public Connection(string host, int port) { ... } - public string Host { get; } - public int Port { get; } - public void Connect() { ... } - public void Dispose() { ... } -} -``` - -### Step 4: Review member design - -**Properties vs methods:** -Use properties for cheap, idempotent state access and methods for operations, conversions, or expensive work. - -- `stream.Length` — property (cheap, idempotent) -- `stream.Read(buffer, offset, count)` — method (operation) -- `object.ToString()` — method (conversion) -- `list.ToArray()` — method (creates new object) - -**Overload patterns (as in `StringBuilder`, `Console`, etc.):** -- Consistent parameter order across overloads -- Simplest overload delegates to the most complete one -- The most-parameter overload contains the core logic - -```csharp -// Standard overload pattern -public void Write(string value) => Write(value, 0, value.Length); -public void Write(string value, int startIndex) => Write(value, startIndex, value.Length - startIndex); -public void Write(string value, int startIndex, int count) { /* core */ } -``` - -**Constructor patterns:** -- Default constructors enable simple instantiation -- Parameterized constructors for required initialization -- `CancellationToken` always last when present - -**Event patterns:** -- Use `EventHandler` -- Raise through `protected virtual void On(EventArgs e)` -- Custom EventArgs derive from `System.EventArgs` - -### Step 5: Review error handling - -**Standard error reporting patterns:** - -| Situation | Convention | Example | -|-----------|------------|---------| -| Null argument | `ArgumentNullException` with `paramName` | `ArgumentNullException.ThrowIfNull(path)` | -| Out-of-range value | `ArgumentOutOfRangeException` with `paramName` | `ArgumentOutOfRangeException.ThrowIfNegative(count)` | -| Invalid state | `InvalidOperationException` | Calling `Read` on a closed stream | -| Not supported | `NotSupportedException` | Calling `Write` on a read-only stream | -| After disposal | `ObjectDisposedException` | Using a disposed `HttpClient` | -| Parse failure | Try-Parse pattern | `int.Parse` throws, `int.TryParse` returns bool | - -**Check for:** -- Throwing `Exception` or `SystemException` directly -- Missing `paramName` on argument exceptions -- Vague exception messages -- No Try-Parse pattern for commonly-failing operations -- Exceptions thrown from `Equals`, `GetHashCode`, or `ToString` - -### Step 6: Review collection usage - -**Established conventions for collections in APIs:** - -| Position | Preferred type | Avoid | Example | -|----------|---------------|-------|-------------| -| Return type (writable) | `Collection` | `List` | `HttpHeadersCollection` | -| Return type (read-only) | `ReadOnlyCollection` | `T[]` (mutable) | `ReadOnlyCollection` | -| Parameter (input) | `IEnumerable` | `List`, `T[]` | `AddRange(IEnumerable)` | -| Property (writable) | `Collection`, get-only | settable `List` | `Items { get; }` | -| Empty collection | `Array.Empty()` or empty instance | `null` | `Enumerable.Empty()` | - -For deeper collection guidance, see [references/](references/). - -### Step 7: Review resource management - -If the type manages resources, check for proper `IDisposable` implementation. - -**Standard dispose pattern (as in `Stream`, `DbConnection`, etc.):** -```csharp -public class ResourceHolder : IDisposable -{ - private bool _disposed; - - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - protected virtual void Dispose(bool disposing) - { - if (!_disposed) - { - if (disposing) { /* release managed resources */ } - _disposed = true; - } - } -} -``` - -### Step 8: Review extensibility - -**Established extensibility patterns:** - -| Pattern | Example | Purpose | When to use | -|---------|------------|---------|-------------| -| Abstract methods | `Stream.Read`, `Stream.Write` | Forced customization points | Subclass MUST provide behavior | -| Virtual methods | `HttpMessageHandler.SendAsync` | Optional customization | Subclass MAY override default behavior | -| Events | `FileSystemWatcher.Changed` | Notification extensibility | External observers, no subclassing needed | -| Delegates/Func | `List.Find(Predicate)` | Caller-supplied logic | One-off customization at call site | -| Sealed types | `String`, `AesGcm` | Prevent inheritance | Type is not designed for extension | - -**Check for:** -- Unsealed types with no virtual members (suggests sealing was forgotten) -- Virtual members with no base implementation and no documentation of expected behavior -- Extensibility via inheritance where events or delegates would be simpler - -For deeper extensibility guidance, see [references/](references/). - -### Step 9: Assess breaking change risk - -If modifying existing APIs, check that no existing consumer code would break: - -| Change | Breaking? | Mitigation | -|--------|-----------|------------| -| Remove/rename a public type or member | Yes | Add new member, `[Obsolete]` the old one | -| Change a method's return type | Yes | Add new method with different name | -| Add a required parameter | Yes | Add an overload instead; keep the old signature | -| Add a member to an interface | Yes (pre-DIM) | Use default interface methods (.NET 5+) or add a new interface | -| Change parameter type (e.g. `string` → `ReadOnlySpan`) | Yes | Add overload, keep original | -| Add a new overload | Usually no | Can break if overload resolution becomes ambiguous | -| Add a new optional parameter | Usually no | Can break binary compat (recompile required) | -| Add a new type or member | No | Safe — additive change | -| Seal a previously unsealed class | Yes | Cannot be undone | -| Change exception type thrown | Yes (behavioral) | Document and version-gate | - -**Rule of thumb**: If you are unsure whether a change is breaking, treat it as breaking. - -## Output Format - -Structure every review as: - -1. **Surface classification**: New / Extension / Modification, library vs application context -2. **Scenario code**: Calling code for top 2-3 scenarios (written in Step 1) -3. **Issues found** (grouped by severity): - - **Critical**: Contradicts established conventions (mutable struct, `List` in public API, bare `Exception`, missing `IDisposable`) - - **Warning**: Deviates from common C# patterns (naming mismatch, inconsistent overloads, missing Try-Parse) - - **Suggestion**: Polish improvements (more descriptive parameter names, better IntelliSense ordering) -4. **For each issue**: What the convention is → what the code does → recommended fix with before/after code -5. **Strengths**: What the API does well (always include this) -6. **Breaking change assessment**: If modifying existing APIs - -## Failure Modes and Recovery - -| Situation | Recovery | -|-----------|----------| -| Insufficient context (don't know if it's a library or app) | Ask the developer — guidance differs (e.g. `Collection` matters for libraries, less so for app-internal code) | -| Reviewing internal/private code | Clarify scope — these conventions target public API surfaces. Internal code has more latitude. | -| Conflicting conventions | Acknowledge the inconsistency, recommend the more recent pattern | -| Performance vs. API purity tradeoff | Defer to the `dotnet-jit-expert` agent for runtime-level optimization or the `dotnet-performance-patterns-reviewer` for API usage patterns; keep the public API clean | -| Breaking change is unavoidable | Document the break, suggest `[Obsolete]` transition period, recommend a major version bump | - -## Validation - -- [ ] All public type and member names use PascalCase -- [ ] All parameters use camelCase -- [ ] Interface names start with `I` -- [ ] Methods use verb names; properties use noun/adjective names -- [ ] Boolean properties use affirmative phrasing -- [ ] Events use verb tense naming (gerund/past) -- [ ] Flag enums have `[Flags]` and plural names -- [ ] No `List` or `Dictionary` in public API surface -- [ ] Collections return empty (not null) when empty -- [ ] Standard exception types used with `paramName` set -- [ ] `IDisposable` pattern correctly implemented where needed -- [ ] Overloaded methods have consistent parameter ordering -- [ ] No breaking changes to existing consumers -- [ ] Calling code for top scenarios is clean and intuitive - -## Common Pitfalls - -| Pitfall | Solution | -|---------|----------| -| `List` in public API | Use `Collection` or `ReadOnlyCollection` | -| Public fields | Convert to properties | -| Returning null from collection properties | Return empty collection | -| Method names using nouns | Methods are actions — use verbs | -| Throwing bare `Exception` | Use specific types (`ArgumentNullException`, `InvalidOperationException`) | -| Mutable structs | Make value types readonly/immutable | -| Missing Try-Parse variant | Provide `TryParse` alongside `Parse` for commonly-failing ops | -| No `paramName` on argument exceptions | Always set via constructor or `ThrowIfNull` | -| Inconsistent overload parameter order | Align all overloads; simplest delegates to most complete | - -## References - -For deeper guidance on specific topics, see: -- [Naming Conventions](references/naming-conventions.md) — C# naming patterns and coding style -- [Type Design Patterns](references/type-design-patterns.md) — Class, struct, interface, and enum patterns -- [Member Design Patterns](references/member-design-patterns.md) — Properties, methods, events, constructors, operators -- [Error Handling Patterns](references/error-handling-patterns.md) — Exception types, Try-Parse, argument validation -- [API Review Checklist](references/api-review-checklist.md) — Checklist for API reviews and proposal preparation diff --git a/skills/dotnet-api-design-cop/references/naming-conventions.md b/skills/dotnet-api-design-cop/references/naming-conventions.md deleted file mode 100644 index ad9c93a38c..0000000000 --- a/skills/dotnet-api-design-cop/references/naming-conventions.md +++ /dev/null @@ -1,177 +0,0 @@ -# Naming Conventions Reference - -Established C# naming conventions for public API design. - -## Casing - -### PascalCase - -Used for all public identifiers except parameters. Every word starts with an uppercase letter. - -```csharp -// Types -public class StreamReader { } -public struct DateTime { } -public interface IEnumerable { } -public enum ConsoleColor { } - -// Members -public int Count { get; } -public void ReadLine() { } -public event EventHandler Click; -public const int MaxValue = int.MaxValue; -``` - -### camelCase - -Used for parameters and local variables (also for private fields with `_` prefix). - -```csharp -public void CopyTo(Stream destination, int bufferSize) { } -private int _count; -private static TimeSpan s_defaultTimeout; -``` - -## Acronym Casing - -Established convention: two-letter acronyms stay uppercase, three+ letters use PascalCase. - -| Acronym | Example | Pattern | -|---------|--------|---------| -| IO | `System.IO` | Two letters → uppercase | -| UI | `UIElement` | Two letters → uppercase | -| DB | `DbConnection` | Two letters → uppercase (note: newer APIs use `Db`) | -| Html | `HtmlWriter` | Three letters → PascalCase | -| Xml | `XmlReader` | Three letters → PascalCase | -| Json | `JsonSerializer` | Four letters → PascalCase | -| Url | `UrlEncoder` | Three letters → PascalCase | - -## Type Name Patterns - -### Classes and Structs — Nouns - -Use noun or noun phrase names: -- `FileStream`, `StringBuilder`, `HttpClient`, `MemoryCache` -- `DateTime`, `TimeSpan`, `Guid`, `Color` - -### Interfaces — `I` Prefix + Adjective/Noun - -Established convention: -- `IDisposable`, `IComparable`, `IFormattable` (adjectives) -- `IEnumerable`, `ICollection`, `IList` (nouns) -- `IServiceProvider`, `ICustomFormatter` (noun phrases) - -### Type Parameters — `T` Prefix - -```csharp -// Single type param: just T -public class List { } -public interface IComparer { } - -// Multiple or constrained: T + descriptive name -public class Dictionary { } -public interface ISessionChannel where TSession : ISession { } -``` - -### Suffixes - -| When type... | Suffix | Examples | -|-------------|--------|-------------| -| Derives from `Exception` | `Exception` | `ArgumentNullException`, `IOException` | -| Derives from `Attribute` | `Attribute` | `ObsoleteAttribute`, `SerializableAttribute` | -| Derives from `EventArgs` | `EventArgs` | `CancelEventArgs`, `PropertyChangedEventArgs` | -| Represents a collection | `Collection` | `ObservableCollection`, `KeyedCollection` | -| Represents a dictionary | `Dictionary` | `ConcurrentDictionary`, `SortedDictionary` | - -## Method Names — Verbs - -Methods use verbs or verb phrases: - -```csharp -// System.IO.Stream -public abstract int Read(byte[] buffer, int offset, int count); -public abstract void Write(byte[] buffer, int offset, int count); -public virtual void CopyTo(Stream destination); -public virtual void Close(); - -// System.String -public int CompareTo(string value); -public bool Contains(string value); -public string Replace(string oldValue, string newValue); -public string[] Split(char separator); -``` - -Async methods add `Async` suffix: -```csharp -public Task ReadAsync(byte[] buffer, int offset, int count); -public Task WriteAsync(byte[] buffer, int offset, int count); -``` - -## Property Names — Nouns/Adjectives - -```csharp -// Nouns -public int Count { get; } -public int Length { get; } -public string Name { get; set; } -public Stream BaseStream { get; } - -// Boolean with Is/Can/Has -public bool IsReadOnly { get; } -public bool CanRead { get; } -public bool CanSeek { get; } -public bool HasValue { get; } -public bool IsCompleted { get; } -``` - -## Event Names — Verb Tense - -Use present participle (gerund) for events that fire before/during, and past tense for events that fire after: - -| Pre-event | Post-event | -|-----------|-----------| -| `Closing` | `Closed` | -| `Validating` | `Validated` | -| `PropertyChanging` | `PropertyChanged` | -| `CollectionChanging` | N/A (some types omit pre-event) | - -## Enum Names - -Non-flag enums use singular nouns: -```csharp -public enum ConsoleColor { Black, Blue, Green, ... } -public enum DayOfWeek { Sunday, Monday, ... } -public enum FileMode { Create, Open, Append, ... } -``` - -Flag enums use plural nouns and `[Flags]`: -```csharp -[Flags] -public enum FileAttributes { ReadOnly = 1, Hidden = 2, System = 4, ... } - -[Flags] -public enum BindingFlags { Default = 0, Instance = 4, Static = 8, Public = 16, ... } -``` - -## Namespace Patterns - -Established convention: `.[.]` - -``` -System.Collections.Generic -System.IO.Compression -System.Net.Http -System.Text.Json -Microsoft.Extensions.Logging -Microsoft.Extensions.DependencyInjection -``` - -## What to Avoid - -These patterns should never appear in public APIs: - -- Hungarian notation (`strName`, `iCount`, `bEnabled`) -- Underscores in public names (`Get_Value`, `Max_Count`) -- Abbreviations (`Btn`, `Msg`, `Mgr` — except universally known ones like `IO`) -- Names differing only by case -- Language-specific type names in methods (`GetInt` vs `GetInt32`) diff --git a/skills/reviewing-dotnet-api-design/SKILL.md b/skills/reviewing-dotnet-api-design/SKILL.md new file mode 100644 index 0000000000..c5b063f5bb --- /dev/null +++ b/skills/reviewing-dotnet-api-design/SKILL.md @@ -0,0 +1,109 @@ +--- +name: reviewing-dotnet-api-design +description: Reviews .NET API designs for consistency with established C# conventions covering naming, type design, member design, error handling, collections, extensibility, and breaking changes. Use when reviewing public API surfaces, designing new library APIs, preparing API proposals, or assessing breaking change risk. +--- + +# .NET API Design Review + +Review .NET API surfaces against established C# conventions. Covers naming, type design, member design, error handling, extensibility, collection usage, and resource management. + +You do NOT cite or reference the Pearson-licensed "Framework Design Guidelines" book or the learn.microsoft.com/en-us/dotnet/standard/design-guidelines/ pages. + +## Inputs + +| Input | Required | Description | +|-------|----------|-------------| +| Code under review | Yes | The API surface to analyze (types, members, signatures) | +| Context | Recommended | Library, framework, or application? NuGet package or internal? | +| .NET version | Recommended | Target framework version | +| Scope | Optional | Full review or focused area (naming, types, errors, etc.) | + +## Workflow + +### Step 1: Classify and Write Caller Code + +Classify the API surface: + +| Surface type | Focus | Key risk | +|-------------|-------|----------| +| New library API | Full review | Getting the shape wrong before anyone depends on it | +| Extension to existing API | Consistency with existing surface | Inconsistency with established conventions | +| Modification to existing API | Breaking change assessment first | Breaking existing consumers | +| API proposal | Scenario code, then full review | Approving a surface that is hard to use | + +Write sample calling code for the top 2-3 scenarios. If calling code is awkward in 3-5 lines, the API needs work. + +### Step 2: Review Naming + +Load [references/naming-conventions.md](references/naming-conventions.md) and check all public names against C# naming conventions. + +### Step 3: Review Type Design + +Load [references/type-design-patterns.md](references/type-design-patterns.md) and verify type choices (class vs struct vs interface vs enum). + +### Step 4: Review Member Design + +Load [references/member-design-patterns.md](references/member-design-patterns.md) and check properties, methods, events, constructors, and operator patterns. + +### Step 5: Review Error Handling + +Load [references/error-handling-patterns.md](references/error-handling-patterns.md) and check exception types, argument validation, and Try-Parse patterns. + +### Step 6: Review Collections + +Check collection conventions in public API surfaces: +- Return `Collection` / `ReadOnlyCollection`, not `List` +- Accept `IEnumerable` as parameters +- Return empty collections, never null + +### Step 7: Review Resource Management + +If the type manages resources, check for proper `IDisposable` implementation with the standard dispose pattern. + +### Step 8: Review Extensibility + +Check extensibility mechanisms: abstract vs virtual methods, events, delegates, sealed types. Flag unsealed types with no virtual members. + +### Step 9: Assess Breaking Changes + +If modifying existing APIs, load [references/api-review-checklist.md](references/api-review-checklist.md) for the breaking change assessment table. + +## Output Format + +1. **Surface classification**: New / Extension / Modification, library vs application +2. **Scenario code**: Calling code for top 2-3 scenarios +3. **Issues found** (by severity): + - **Critical**: Contradicts established conventions (mutable struct, `List` in public API, bare `Exception`, missing `IDisposable`) + - **Warning**: Deviates from common C# patterns (naming mismatch, inconsistent overloads) + - **Suggestion**: Polish improvements (more descriptive parameter names) +4. **For each issue**: Convention → what code does → recommended fix with before/after code +5. **Strengths**: What the API does well +6. **Breaking change assessment**: If modifying existing APIs + +## Validation + +- [ ] All public names follow C# naming conventions +- [ ] Type choices are appropriate (struct vs class vs interface) +- [ ] No `List` or mutable arrays in public API surface +- [ ] Standard exception types used with `paramName` +- [ ] `IDisposable` correctly implemented where needed +- [ ] No breaking changes to existing consumers +- [ ] Calling code for top scenarios is clean and intuitive + +## Common Pitfalls + +| Pitfall | Solution | +|---------|----------| +| `List` in public API | Use `Collection` or `ReadOnlyCollection` | +| Mutable structs | Make value types `readonly struct` | +| Throwing bare `Exception` | Use specific types (`ArgumentNullException`, `InvalidOperationException`) | +| Missing Try-Parse variant | Provide `TryParse` alongside `Parse` for commonly-failing operations | +| Inconsistent overload parameter order | Simplest overload delegates to most complete | + +## References + +- [Naming Conventions](references/naming-conventions.md) — C# naming patterns and coding style +- [Type Design Patterns](references/type-design-patterns.md) — Class, struct, interface, and enum patterns +- [Member Design Patterns](references/member-design-patterns.md) — Properties, methods, events, constructors, operators +- [Error Handling Patterns](references/error-handling-patterns.md) — Exception types, Try-Parse, argument validation +- [API Review Checklist](references/api-review-checklist.md) — Checklist for API reviews and breaking change assessment diff --git a/skills/dotnet-api-design-cop/references/api-review-checklist.md b/skills/reviewing-dotnet-api-design/references/api-review-checklist.md similarity index 88% rename from skills/dotnet-api-design-cop/references/api-review-checklist.md rename to skills/reviewing-dotnet-api-design/references/api-review-checklist.md index 829eeae865..58cec3e408 100644 --- a/skills/dotnet-api-design-cop/references/api-review-checklist.md +++ b/skills/reviewing-dotnet-api-design/references/api-review-checklist.md @@ -1,23 +1,7 @@ # API Review Checklist Reference -A structured checklist for .NET API reviews, based on established C# conventions. - -## API Review Process - -Every public API should be reviewed before release. A structured process ensures consistency and quality. - -### Process Flow - -1. **File a proposal** containing an API sketch and usage samples -2. **Area owner reviews** and engages in discussion -3. **Owner marks** the proposal ready for review when actionable -4. **Review panel evaluates** the proposal in a scheduled meeting -5. **Outcome**: approved, needs work, or rejected - ## Proposal Template -When proposing APIs, write a proposal that shows usage before listing signatures: - ```markdown ## API Proposal: [Feature Name] diff --git a/skills/dotnet-api-design-cop/references/error-handling-patterns.md b/skills/reviewing-dotnet-api-design/references/error-handling-patterns.md similarity index 79% rename from skills/dotnet-api-design-cop/references/error-handling-patterns.md rename to skills/reviewing-dotnet-api-design/references/error-handling-patterns.md index 3654e1cf08..db2e089b83 100644 --- a/skills/dotnet-api-design-cop/references/error-handling-patterns.md +++ b/skills/reviewing-dotnet-api-design/references/error-handling-patterns.md @@ -1,11 +1,7 @@ # Error Handling Patterns Reference -Established C# error handling conventions. - ## Standard Exception Types -Use a consistent set of exception types. Custom exception types should be rare. - | Exception Type | When It Is Thrown | Example | |---------------|----------------------|---------| | `ArgumentNullException` | A null argument was passed | `File.Open(null, ...)` | @@ -22,16 +18,13 @@ Use a consistent set of exception types. Custom exception types should be rare. | `IndexOutOfRangeException` | Array/span index invalid | Runtime-thrown, not user code | | `NullReferenceException` | Null dereference | Runtime-thrown, not user code | -**What to avoid:** -- Throws `Exception` directly -- Throws `SystemException` directly -- Throws `ApplicationException` (legacy, unused) -- Throws `NullReferenceException` or `IndexOutOfRangeException` from library code (these are runtime errors) +**Never throw directly:** +- `Exception`, `SystemException`, `ApplicationException` +- `NullReferenceException` or `IndexOutOfRangeException` (runtime errors only) ## Argument Validation Patterns ### Modern Pattern (.NET 6+) -Use static `ThrowIf` methods for argument validation: ```csharp public void SetName(string name) @@ -68,7 +61,7 @@ public string Name } ``` -Note: Use `value` as the parameter name in property setter exceptions (it's the implicit parameter name). +Note: Use `value` as the parameter name in property setter exceptions. ### Legacy Pattern (pre-.NET 6) ```csharp @@ -84,18 +77,13 @@ public void SetName(string name) ## The Try-Parse Pattern -The convention is to provide both throwing and non-throwing variants for operations that commonly fail: +Provide both throwing and non-throwing variants for operations that commonly fail: ```csharp -// Throwing variant — for when failure is exceptional +// Throwing variant public static int Parse(string s); -public static DateTime Parse(string s); -public static IPAddress Parse(string ipString); - -// Non-throwing variant — for when failure is expected +// Non-throwing variant public static bool TryParse(string s, out int result); -public static bool TryParse(string s, out DateTime result); -public static bool TryParse(string ipString, out IPAddress? address); ``` **Modern Try-Parse pattern (.NET 7+):** @@ -115,17 +103,14 @@ public interface IParsable where TSelf : IParsable ## Exception Messages -Established convention: exception messages describe what went wrong and often hint at what to do: - ```csharp -// Good messages "Stream does not support reading." "Non-negative number required. (Parameter 'count')" "Collection was modified; enumeration operation may not execute." "Index was out of range. Must be non-negative and less than the size of the collection." ``` -**Characteristics of good exception messages:** +**Requirements:** - Complete sentences with proper punctuation - State the problem clearly - Include relevant values when possible @@ -138,7 +123,6 @@ Validate arguments synchronously (before the first `await`) so callers get immed ```csharp public Task ReadFileAsync(string path, CancellationToken cancellationToken) { - // Validate BEFORE async work — throws immediately ArgumentNullException.ThrowIfNull(path); return ReadFileCoreAsync(path, cancellationToken); } @@ -152,10 +136,9 @@ private async Task ReadFileCoreAsync(string path, CancellationToken ct) ## Exception Builder Pattern -Use helper methods to throw exceptions, keeping call sites small enough for JIT inlining: +Helper methods keep call sites small enough for JIT inlining: ```csharp -// Common pattern for hot paths private static void ThrowInvalidOperation() => throw new InvalidOperationException("Enumeration already finished."); @@ -163,7 +146,7 @@ public bool MoveNext() { if (_index >= _count) { - ThrowInvalidOperation(); // Keeps MoveNext small for inlining + ThrowInvalidOperation(); } // ... } @@ -171,8 +154,6 @@ public bool MoveNext() ## Methods That Should Not Throw -Established convention: these methods avoid throwing exceptions: - | Method | Why | |--------|-----| | `Equals(object)` | Used in comparisons, hash tables — must be safe | diff --git a/skills/dotnet-api-design-cop/references/member-design-patterns.md b/skills/reviewing-dotnet-api-design/references/member-design-patterns.md similarity index 53% rename from skills/dotnet-api-design-cop/references/member-design-patterns.md rename to skills/reviewing-dotnet-api-design/references/member-design-patterns.md index 3595338be4..0d8f200bf2 100644 --- a/skills/dotnet-api-design-cop/references/member-design-patterns.md +++ b/skills/reviewing-dotnet-api-design/references/member-design-patterns.md @@ -1,81 +1,20 @@ # Member Design Patterns Reference -Established C# member design conventions. - ## Properties vs Methods -There is a clear line between properties (state access) and methods (operations). - ### Use Properties When: - Access is cheap (field-like) - Calling twice returns the same value - No observable side effects -- Represents a logical attribute of the type - -```csharp -// System.IO.Stream — properties for state -public abstract long Length { get; } -public abstract long Position { get; set; } -public abstract bool CanRead { get; } -public abstract bool CanSeek { get; } - -// System.Collections.Generic.List -public int Count { get; } -public int Capacity { get; set; } -``` ### Use Methods When: - The operation is a conversion (`ToString()`, `ToArray()`) - The call is expensive or has side effects -- Different results each time (`DateTime.Now` is a well-known exception) - Returns a new object or array -```csharp -// Conversions — always methods -public override string ToString(); -public T[] ToArray(); -public List ToList(); - -// Operations with side effects — always methods -public int Read(byte[] buffer, int offset, int count); -public void Write(byte[] buffer, int offset, int count); - -// Expensive operations — always methods -public DataTable GetSchemaTable(); -public byte[] ComputeHash(byte[] buffer); -``` - ## Method Overloading Patterns -Established convention: overloads form a progressive series from simplest to most complete. - -### StringBuilder.Append Pattern -```csharp -// Simplest → most complete, all consistent -public StringBuilder Append(string value); -public StringBuilder Append(string value, int startIndex, int count); -public StringBuilder Append(char value); -public StringBuilder Append(char value, int repeatCount); -``` - -### Stream.Read Pattern -```csharp -// Modern .NET adds Span overloads alongside array overloads -public abstract int Read(byte[] buffer, int offset, int count); -public virtual int Read(Span buffer); -``` - -### Console.WriteLine Pattern -```csharp -// Many overloads, all following the same naming -public static void WriteLine(); -public static void WriteLine(string value); -public static void WriteLine(string format, object arg0); -public static void WriteLine(string format, object arg0, object arg1); -public static void WriteLine(string format, params object[] arg); -``` - -**Key patterns:** +**Key overload rules:** 1. Parameter order is consistent across all overloads 2. Simpler overloads delegate to the most complete one 3. Parameter names are identical across overloads @@ -84,18 +23,7 @@ public static void WriteLine(string format, params object[] arg); ## Constructor Patterns -### Simple Instantiation -The convention supports creating instances with minimal ceremony: - ```csharp -// Default constructor — ready to use -var sb = new StringBuilder(); -var list = new List(); - -// Parameterized — for required values -var uri = new Uri("https://example.com"); -var fs = new FileStream(path, FileMode.Open); - // Common pattern: overloads from minimal to full public StringBuilder(); public StringBuilder(string value); @@ -115,11 +43,7 @@ public FileStream(string path, FileMode mode) ## Event Patterns -Established event design conventions (as seen in `FileSystemWatcher`, `ObservableCollection`, etc.): - -### Standard Pattern ```csharp -// 1. Define EventArgs if needed public class FileChangedEventArgs : EventArgs { public string FileName { get; } @@ -132,40 +56,34 @@ public class FileChangedEventArgs : EventArgs } } -// 2. Declare event using EventHandler public event EventHandler FileChanged; -// 3. Raise through protected virtual method protected virtual void OnFileChanged(FileChangedEventArgs e) { FileChanged?.Invoke(this, e); } ``` -### Established Conventions: +**Conventions:** - `EventHandler` is the standard delegate type -- Raising method is named `On` -- Raising method is `protected virtual` for extensibility +- Raising method is `On`, `protected virtual` - `EventArgs.Empty` used when no data is needed - EventArgs properties are typically read-only ## Operator Overloading Patterns -Operators are overloaded only on types with natural mathematical or comparison semantics: +Only on types with natural mathematical or comparison semantics: ```csharp -// DateTime — subtraction produces TimeSpan public static TimeSpan operator -(DateTime d1, DateTime d2); public static DateTime operator +(DateTime d, TimeSpan t); -// Decimal — full arithmetic operators public static decimal operator +(decimal d1, decimal d2); -public static decimal operator -(decimal d1, decimal d2); public static bool operator ==(decimal d1, decimal d2); public static bool operator !=(decimal d1, decimal d2); ``` -**Operator conventions:** +**Rules:** - Operators always come in pairs (`==`/`!=`, `<`/`>`, `<=`/`>=`) - `IEquatable` is implemented alongside `==`/`!=` - `GetHashCode()` is overridden whenever `Equals()` is @@ -173,8 +91,6 @@ public static bool operator !=(decimal d1, decimal d2); ## IEquatable Pattern -As seen in `DateTime`, `Guid`, `Int32`, etc.: - ```csharp public readonly struct Money : IEquatable { @@ -197,8 +113,6 @@ public readonly struct Money : IEquatable ## IComparable Pattern -As seen in `String`, `DateTime`, `Int32`: - ```csharp public readonly struct Version : IComparable, IEquatable { @@ -215,40 +129,17 @@ public readonly struct Version : IComparable, IEquatable ## ToString Pattern -Every type should override `ToString()` with a human-readable representation: - -```csharp -// DateTime -public override string ToString() => "2/12/2026 2:39:17 AM"; - -// Guid -public override string ToString() => "d85b1407-351d-4694-9392-03acc5870eb1"; - -// Custom types should follow the same pattern -public override string ToString() => $"{Name} ({Count} items)"; -``` +Every type should override `ToString()` with a human-readable representation. ## Virtual Member Patterns Virtual members should be used deliberately, not speculatively: ```csharp -public class HttpMessageHandler -{ - // Virtual: designed as customization point - protected internal virtual HttpResponseMessage Send( - HttpRequestMessage request, CancellationToken cancellationToken); -} - public abstract class Stream { - // Abstract: MUST be implemented public abstract int Read(byte[] buffer, int offset, int count); - - // Virtual: CAN be overridden (has default implementation) public virtual void CopyTo(Stream destination, int bufferSize) { /* default */ } - - // Non-virtual: fixed behavior public void Dispose() { /* fixed cleanup workflow */ } } ``` diff --git a/skills/reviewing-dotnet-api-design/references/naming-conventions.md b/skills/reviewing-dotnet-api-design/references/naming-conventions.md new file mode 100644 index 0000000000..7eb44c4bba --- /dev/null +++ b/skills/reviewing-dotnet-api-design/references/naming-conventions.md @@ -0,0 +1,71 @@ +# Naming Conventions Reference + +## Casing + +- **PascalCase**: all public identifiers except parameters +- **camelCase**: parameters, locals, private fields with `_` prefix, static fields with `s_` prefix + +## Acronym Casing + +| Acronym | Example | Pattern | +|---------|--------|---------| +| IO | `System.IO` | Two letters → uppercase | +| UI | `UIElement` | Two letters → uppercase | +| DB | `DbConnection` | Two letters → uppercase (note: newer APIs use `Db`) | +| Html | `HtmlWriter` | Three letters → PascalCase | +| Xml | `XmlReader` | Three letters → PascalCase | +| Json | `JsonSerializer` | Four letters → PascalCase | +| Url | `UrlEncoder` | Three letters → PascalCase | + +## Type Name Suffixes + +| When type... | Suffix | Examples | +|-------------|--------|-------------| +| Derives from `Exception` | `Exception` | `ArgumentNullException`, `IOException` | +| Derives from `Attribute` | `Attribute` | `ObsoleteAttribute`, `SerializableAttribute` | +| Derives from `EventArgs` | `EventArgs` | `CancelEventArgs`, `PropertyChangedEventArgs` | +| Represents a collection | `Collection` | `ObservableCollection`, `KeyedCollection` | +| Represents a dictionary | `Dictionary` | `ConcurrentDictionary`, `SortedDictionary` | + +## Method Names + +Async methods add `Async` suffix. Verbs or verb phrases. + +## Property Names + +Boolean properties use `Is`/`Can`/`Has` prefix. + +## Event Names + +| Pre-event | Post-event | +|-----------|-----------| +| `Closing` | `Closed` | +| `Validating` | `Validated` | +| `PropertyChanging` | `PropertyChanged` | +| `CollectionChanging` | N/A (some types omit pre-event) | + +## Enum Names + +- Non-flag enums: singular nouns +- Flag enums: plural nouns with `[Flags]` + +## Namespace Patterns + +`.[.]` + +``` +System.Collections.Generic +System.IO.Compression +System.Net.Http +System.Text.Json +Microsoft.Extensions.Logging +Microsoft.Extensions.DependencyInjection +``` + +## What to Avoid + +- Hungarian notation (`strName`, `iCount`, `bEnabled`) +- Underscores in public names (`Get_Value`, `Max_Count`) +- Abbreviations (`Btn`, `Msg`, `Mgr` — except universally known ones like `IO`) +- Names differing only by case +- Language-specific type names in methods (`GetInt` vs `GetInt32`) diff --git a/skills/dotnet-api-design-cop/references/type-design-patterns.md b/skills/reviewing-dotnet-api-design/references/type-design-patterns.md similarity index 65% rename from skills/dotnet-api-design-cop/references/type-design-patterns.md rename to skills/reviewing-dotnet-api-design/references/type-design-patterns.md index f0d186f460..a16b4d60ef 100644 --- a/skills/dotnet-api-design-cop/references/type-design-patterns.md +++ b/skills/reviewing-dotnet-api-design/references/type-design-patterns.md @@ -1,11 +1,7 @@ # Type Design Patterns Reference -Established C# type design conventions. - ## When to Use Structs -Use structs for small, immutable types that represent single values. - | Struct | Size | Characteristics | |--------|------|----------------| | `Int32` | 4 bytes | Primitive value | @@ -18,16 +14,14 @@ Use structs for small, immutable types that represent single values. | `CancellationToken` | 8 bytes | Lightweight, passed by value | | `ReadOnlySpan` | 16 bytes | ref struct, zero-allocation view | -**Consistent struct characteristics:** +**Struct requirements:** - Small (≤ 16 bytes typically) - Immutable (use `readonly struct`) - Represent a single logical value -- Value equality semantics (`Equals`/`GetHashCode` based on content) +- Value equality semantics - No inheritance needed -- Rarely boxed in typical usage ```csharp -// Established struct pattern public readonly struct Point : IEquatable { public double X { get; } @@ -43,8 +37,6 @@ public readonly struct Point : IEquatable ## When to Use Classes -Use classes for types with identity, complex behavior, large size, or inheritance. - | Class | Why Not Struct | |-------|---------------| | `String` | Variable size, reference semantics, sealed | @@ -56,22 +48,17 @@ Use classes for types with identity, complex behavior, large size, or inheritanc ## When to Use Interfaces -Interfaces are used for cross-hierarchy contracts that multiple unrelated types implement. - ```csharp -// IDisposable — implemented by classes (Stream, HttpClient) and some structs public interface IDisposable { void Dispose(); } -// IEnumerable — implemented by List, Array, Dictionary, etc. public interface IEnumerable : IEnumerable { IEnumerator GetEnumerator(); } -// IComparable — implemented by String, Int32, DateTime, etc. public interface IComparable { int CompareTo(T other); @@ -80,16 +67,11 @@ public interface IComparable **Interface conventions:** - Every interface should have multiple implementations -- Interfaces are consumed by other APIs (`IEnumerable` consumed by LINQ) - New interfaces are added cautiously (adding members breaks implementors) -- `I` prefix is universal and mandatory ## Abstract Class Patterns -Use abstract classes when shared implementation is needed alongside enforced customization: - ```csharp -// Stream — abstract base with shared logic + abstract customization points public abstract class Stream : IDisposable, IAsyncDisposable { // Abstract: derived types MUST implement @@ -101,35 +83,13 @@ public abstract class Stream : IDisposable, IAsyncDisposable public virtual void CopyTo(Stream destination) { /* default impl */ } public virtual void Close() { Dispose(true); } - // Concrete: shared behavior public void Dispose() { Dispose(true); GC.SuppressFinalize(this); } } ``` ## Enum Patterns +Flag enums use plural nouns, `[Flags]` attribute, and power-of-two values: -### Non-Flag Enums (Singular Noun) -```csharp -public enum ConsoleColor -{ - Black = 0, - DarkBlue = 1, - DarkGreen = 2, - // ... -} - -public enum FileMode -{ - CreateNew = 1, - Create = 2, - Open = 3, - OpenOrCreate = 4, - Truncate = 5, - Append = 6, -} -``` - -### Flag Enums (Plural Noun + `[Flags]` + Powers of Two) ```csharp [Flags] public enum FileAttributes @@ -141,14 +101,10 @@ public enum FileAttributes Archive = 0x0020, Normal = 0x0080, } - -// Usage: var attrs = FileAttributes.ReadOnly | FileAttributes.Hidden; ``` ## Sealed vs Unsealed -Established convention: most types are unsealed. Sealing is used selectively. - | Sealed | Why | |--------|-----| | `String` | Immutable invariants, security | @@ -162,29 +118,9 @@ Established convention: most types are unsealed. Sealing is used selectively. | `Collection` | Designed for customization | | `Exception` | Custom exception types derive from it | -## Static Class Patterns - -Use static classes as utility containers: - -```csharp -public static class Math -{ - public static double Sqrt(double d) { ... } - public static int Max(int val1, int val2) { ... } -} +## Static Classes -public static class Console -{ - public static void WriteLine(string value) { ... } - public static string ReadLine() { ... } -} - -public static class Path -{ - public static string Combine(string path1, string path2) { ... } - public static string GetExtension(string path) { ... } -} -``` +Static classes serve as utility containers (e.g., `Math`, `Console`, `Path`). ## Type Design Checklist From 592370e02e2735643fe7c0d2ca6b7c8338ce57ec Mon Sep 17 00:00:00 2001 From: artl Date: Fri, 13 Feb 2026 16:57:49 -0800 Subject: [PATCH 3/5] Add test cases for reviewing-dotnet-api-design skill Test asset (test-api.cs) contains 6 deliberate API design violations: - Mutable struct with reference-type field (Critical) - List return in public API (Critical) - Noun-named method (Warning) - Missing paramName on exception (Warning) - Unsealed leaf class (Warning) - Expensive property (Suggestion) Plus 3 correctly implemented patterns (event, IDisposable, EventArgs). README defines 10-point evaluation criteria comparing with/without skill. bad.md and good.md show expected behavior difference. --- tests/reviewing-dotnet-api-design/README.md | 47 +++++++ tests/reviewing-dotnet-api-design/bad.md | 32 +++++ tests/reviewing-dotnet-api-design/good.md | 128 ++++++++++++++++++ tests/reviewing-dotnet-api-design/test-api.cs | 87 ++++++++++++ 4 files changed, 294 insertions(+) create mode 100644 tests/reviewing-dotnet-api-design/README.md create mode 100644 tests/reviewing-dotnet-api-design/bad.md create mode 100644 tests/reviewing-dotnet-api-design/good.md create mode 100644 tests/reviewing-dotnet-api-design/test-api.cs diff --git a/tests/reviewing-dotnet-api-design/README.md b/tests/reviewing-dotnet-api-design/README.md new file mode 100644 index 0000000000..9e3bdc6a8b --- /dev/null +++ b/tests/reviewing-dotnet-api-design/README.md @@ -0,0 +1,47 @@ +# README + +This file contains a prompt, a bad output (without skill), and a good output (with skill). + +The skill is considered successful if the output looks like the bad output without the skill, and like the good output with the skill. If the output looks like the good output without the skill, the skill is considered ineffective. If the output looks like the bad output with the skill and not like the good output with the skill, the skill is considered incorrect. + +## Input prompt + +Review this API for consistency with .NET conventions: `tests/reviewing-dotnet-api-design/test-api.cs` + +## Test asset + +`test-api.cs` — A C# file containing a `Contoso.Networking` namespace with deliberate API design violations and some correctly implemented patterns. + +### Embedded violations + +| # | Violation | Severity | Category | +|---|-----------|----------|----------| +| 1 | `ConnectionInfo` is a mutable struct with reference-type field and side-effecting method | Critical | Type design | +| 2 | `GetItems()` returns `List` in public API | Critical | Collection convention | +| 3 | `Result()` method named with a noun instead of a verb | Warning | Naming | +| 4 | `throw new ArgumentNullException()` missing `paramName` | Warning | Error handling | +| 5 | `DataProcessor` is unsealed with no virtual members | Warning | Extensibility | +| 6 | `Checksum` property does expensive computation (should be a method) | Suggestion | Member design | + +### Embedded strengths + +| # | Good practice | Category | +|---|---------------|----------| +| 1 | `FileWatcher` uses `EventHandler` with `protected virtual OnChanged` | Event pattern | +| 2 | `FileWatcher` implements `IDisposable` with standard dispose pattern | Resource management | +| 3 | `FileChangedEventArgs` derives from `EventArgs` with proper suffix | Naming / type design | + +## Evaluation criteria + +The skill adds value if the review: + +1. Loads the `reviewing-dotnet-api-design` skill +2. Writes sample calling code BEFORE reviewing (caller-first methodology) +3. Classifies the API surface (new library API / extension / modification) +4. Groups findings by severity (Critical → Warning → Suggestion) +5. Catches the mutable struct as Critical (not just a warning) +6. Catches the `List` return type as a convention violation (not just valid code) +7. Identifies the noun-named method with reference to naming conventions +8. Flags the missing `paramName` on `ArgumentNullException` +9. Notes strengths — correct event pattern and IDisposable implementation +10. Provides concrete before/after code fixes for each issue diff --git a/tests/reviewing-dotnet-api-design/bad.md b/tests/reviewing-dotnet-api-design/bad.md new file mode 100644 index 0000000000..576429d05c --- /dev/null +++ b/tests/reviewing-dotnet-api-design/bad.md @@ -0,0 +1,32 @@ +# Expected output WITHOUT skill + +This represents the type of review Claude gives without the `reviewing-dotnet-api-design` skill loaded. The review is generic — it catches some issues but misses convention-specific violations, has no structured methodology, and lacks severity classification. + +## Characteristics of a without-skill review + +- **No caller-first methodology** — jumps straight into reviewing code, never writes sample calling code +- **No API surface classification** — doesn't identify whether this is a new API, extension, or modification +- **No severity grouping** — lists issues in arbitrary order without Critical/Warning/Suggestion classification +- **Misses `List` convention** — the code compiles and works fine, so a generic review won't flag `List` as a public API issue +- **May not flag the mutable struct** — `ConnectionInfo` is valid C# and works as written; flagging it requires knowing the value-type-with-reference-semantics convention +- **Misses `paramName`** — `throw new ArgumentNullException()` compiles fine; knowing to require `paramName` is convention-specific +- **No breaking change assessment** — doesn't evaluate what changes would break consumers +- **No reference to specific conventions** — generic advice like "consider making this immutable" without citing established patterns + +## Example without-skill output + +Here are some observations about this code: + +1. **`ConnectionInfo` struct** — Consider making this immutable. Mutable structs can lead to confusing behavior with value copy semantics. The `Tags` list is a reference type which may cause unexpected sharing. + +2. **`DataProcessor.Result` method** — The name `Result` is a bit unclear. Consider renaming to something more descriptive like `ProcessData` or `ComputeResult`. + +3. **Null check** — You're throwing `ArgumentNullException` without specifying the parameter name. Consider using `throw new ArgumentNullException(nameof(data))`. + +4. **`GetItems` return type** — Looks fine, returns a `List`. + +5. **`Checksum` property** — This property appears to do expensive work (thread sleep and hash computation). Properties should generally be lightweight. + +6. **`FileWatcher`** — Good use of the dispose pattern and events. + +Overall the code looks reasonable. Consider adding XML documentation comments and making `ConnectionInfo` a class instead of a struct if it needs mutable behavior. diff --git a/tests/reviewing-dotnet-api-design/good.md b/tests/reviewing-dotnet-api-design/good.md new file mode 100644 index 0000000000..c3d715ef8c --- /dev/null +++ b/tests/reviewing-dotnet-api-design/good.md @@ -0,0 +1,128 @@ +# Expected output WITH skill + +This represents the type of review Claude gives with the `reviewing-dotnet-api-design` skill loaded. The review follows the 9-step structured workflow, uses caller-first methodology, classifies findings by severity, and references specific conventions. + +## Characteristics of a with-skill review + +- **Loads the skill** — invokes `reviewing-dotnet-api-design` skill before starting the review +- **Caller-first** — writes sample calling code for top scenarios before analyzing the API +- **Surface classification** — identifies this as a "New library API" requiring full review +- **Severity grouping** — Critical → Warning → Suggestion ordering +- **Catches `List`** — flags as Critical because `List` exposes internal implementation; convention is `Collection` or `ReadOnlyCollection` +- **Catches mutable struct** — flags as Critical because `ConnectionInfo` has mutable properties, reference-type fields, and side-effecting methods +- **Convention-specific naming** — identifies `Result()` as noun-named method, cites verb convention +- **Error handling conventions** — flags missing `paramName`, recommends `ArgumentNullException.ThrowIfNull` +- **Notes strengths** — praises correct event pattern and IDisposable implementation +- **Before/after fixes** — concrete code examples for each issue + +## Example with-skill output + +### Surface Classification + +**Type:** New library API +**Context:** `Contoso.Networking` namespace — appears to be a library for network connectivity + +### Scenario Code + +```csharp +// Scenario 1: Create a connection +var info = new ConnectionInfo { Host = "example.com", Port = 443 }; +info.Connect(); + +// Scenario 2: Process data +var processor = new DataProcessor(); +string encoded = processor.Result(data); +List items = processor.GetItems(); + +// Scenario 3: Watch for file changes +using var watcher = new FileWatcher(); +watcher.Changed += (s, e) => Console.WriteLine(e.FilePath); +``` + +**Assessment:** Scenario 1 is problematic — `ConnectionInfo` is a value type, so `info.Connect()` operates on a copy in some contexts. Scenario 2 has awkward naming (`Result` is unclear) and leaks internal `List`. Scenario 3 is clean. + +### 🔴 Critical + +#### C1. Mutable Struct with Reference Semantics — `ConnectionInfo` + +**Convention:** Structs should be small, immutable, represent single values, have no reference-type fields, and have no side effects. (`DateTime`, `TimeSpan`, `Guid` are the model.) + +**Issue:** `ConnectionInfo` is mutable (`set` accessors), has a `List` field (reference type causes unexpected sharing on copy), and has a `Connect()` method (side effect on value type). + +**Fix:** +```csharp +// ❌ Before +public struct ConnectionInfo { ... } + +// ✅ After — Class with identity and side effects +public sealed class ConnectionInfo : IDisposable +{ + public ConnectionInfo(string host, int port) { ... } + public string Host { get; } + public int Port { get; } + public void Connect() { ... } + public void Dispose() { ... } +} +``` + +#### C2. `List` Return Type in Public API — `DataProcessor.GetItems()` + +**Convention:** Public APIs return `Collection` or `ReadOnlyCollection`, not `List`. `List` exposes `Add`, `Sort`, `Reverse` etc. which callers should not use to mutate the internal collection. + +**Fix:** +```csharp +// ❌ Before +public List GetItems() => new List(); + +// ✅ After +public ReadOnlyCollection GetItems() => new List().AsReadOnly(); +``` + +### 🟡 Warning + +#### W1. Noun-Named Method — `DataProcessor.Result()` + +**Convention:** Methods use verbs or verb phrases (`Read`, `Parse`, `ComputeHash`). Properties use nouns. + +**Fix:** Rename to `ComputeResult(byte[] data)` or `Encode(byte[] data)`. + +#### W2. Missing `paramName` on ArgumentNullException + +**Convention:** Always pass `nameof(parameter)` or use `ArgumentNullException.ThrowIfNull()`. + +**Fix:** +```csharp +// ❌ Before +throw new ArgumentNullException(); + +// ✅ After +ArgumentNullException.ThrowIfNull(data); +``` + +#### W3. Unsealed Leaf Class — `DataProcessor` + +**Convention:** Classes with no virtual members and not designed for inheritance should be sealed. + +**Fix:** Add `sealed` modifier: `public sealed class DataProcessor` + +### 💡 Suggestion + +#### S1. Property Does Expensive Work — `DataProcessor.Checksum` + +**Convention:** Properties should be cheap and idempotent. Expensive operations should be methods (`ComputeChecksum()`). + +**Fix:** Convert to method: `public byte[] ComputeChecksum()` + +### ✅ Strengths + +- **Correct event pattern** — `FileWatcher` uses `EventHandler` with `protected virtual OnChanged()` +- **Standard IDisposable** — `FileWatcher` implements the full dispose pattern with `Dispose(bool)` and `GC.SuppressFinalize` +- **Proper EventArgs** — `FileChangedEventArgs` derives from `EventArgs` with correct suffix naming + +### Summary + +| Severity | Count | Issues | +|----------|-------|--------| +| 🔴 Critical | 2 | Mutable struct, `List` in public API | +| 🟡 Warning | 3 | Noun method name, missing paramName, unsealed class | +| 💡 Suggestion | 1 | Expensive property | diff --git a/tests/reviewing-dotnet-api-design/test-api.cs b/tests/reviewing-dotnet-api-design/test-api.cs new file mode 100644 index 0000000000..3b436c2652 --- /dev/null +++ b/tests/reviewing-dotnet-api-design/test-api.cs @@ -0,0 +1,87 @@ +using System; +using System.Collections.Generic; + +namespace Contoso.Networking; + +// --- Deliberate violations for testing --- + +// Critical: Mutable struct with reference-type field and side effects +public struct ConnectionInfo +{ + public string Host { get; set; } + public int Port { get; set; } + public List Tags { get; set; } + + public void Connect() + { + // Side-effecting method on a value type + } +} + +// Warning: Unsealed leaf class with no virtual members +public class DataProcessor +{ + // Warning: Method named with noun instead of verb + public string Result(byte[] data) + { + if (data == null) + // Warning: Missing paramName on ArgumentNullException + throw new ArgumentNullException(); + + return Convert.ToBase64String(data); + } + + // Critical: List return type in public API + public List GetItems() + { + return new List(); + } + + // Suggestion: Property that does expensive work (should be a method) + public byte[] Checksum + { + get + { + // Expensive computation — violates property contract + System.Threading.Thread.Sleep(100); + return System.Security.Cryptography.SHA256.HashData(Array.Empty()); + } + } +} + +// --- Things done well (strengths) --- + +// Correct event pattern +public class FileWatcher : IDisposable +{ + private bool _disposed; + + public event EventHandler? Changed; + + protected virtual void OnChanged(FileChangedEventArgs e) + { + Changed?.Invoke(this, e); + } + + // Correct IDisposable pattern + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) { /* release managed resources */ } + _disposed = true; + } + } +} + +public class FileChangedEventArgs : EventArgs +{ + public string FilePath { get; } + public FileChangedEventArgs(string filePath) => FilePath = filePath; +} From 26e373250571b893f61d75ba4b85d0ade0d38cc0 Mon Sep 17 00:00:00 2001 From: artl Date: Fri, 13 Feb 2026 17:16:45 -0800 Subject: [PATCH 4/5] Redesign test cases with subtle, convention-specific violations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace easy violations (labeled comments, obvious issues) with a realistic message broker API containing 14 violations that require .NET-specific convention knowledge to catch: Hard catches (unlikely without skill): - Mutable struct >16 bytes with Dictionary<> field - [Flags] enum with non-power-of-two values (ExactlyOnce = 3) - Parameter name inconsistency across related methods - Async argument validation inside Task.Run instead of synchronous - Unpaired operator == (missing !=, Equals, GetHashCode) - Missing TryParse companion for Parse method - Abbreviated parameter name in public API (msg vs message) The test code compiles and works correctly — violations are invisible without convention knowledge from the skill's reference files. --- tests/reviewing-dotnet-api-design/README.md | 68 ++++--- tests/reviewing-dotnet-api-design/bad.md | 44 +++-- tests/reviewing-dotnet-api-design/good.md | 178 +++++++++++------- tests/reviewing-dotnet-api-design/test-api.cs | 141 +++++++++----- 4 files changed, 262 insertions(+), 169 deletions(-) diff --git a/tests/reviewing-dotnet-api-design/README.md b/tests/reviewing-dotnet-api-design/README.md index 9e3bdc6a8b..3c7b65bdd1 100644 --- a/tests/reviewing-dotnet-api-design/README.md +++ b/tests/reviewing-dotnet-api-design/README.md @@ -6,42 +6,50 @@ The skill is considered successful if the output looks like the bad output witho ## Input prompt -Review this API for consistency with .NET conventions: `tests/reviewing-dotnet-api-design/test-api.cs` +Review this API for consistency with .NET conventions. This is a NuGet library targeting .NET 8: `tests/reviewing-dotnet-api-design/test-api.cs` ## Test asset -`test-api.cs` — A C# file containing a `Contoso.Networking` namespace with deliberate API design violations and some correctly implemented patterns. - -### Embedded violations - -| # | Violation | Severity | Category | -|---|-----------|----------|----------| -| 1 | `ConnectionInfo` is a mutable struct with reference-type field and side-effecting method | Critical | Type design | -| 2 | `GetItems()` returns `List` in public API | Critical | Collection convention | -| 3 | `Result()` method named with a noun instead of a verb | Warning | Naming | -| 4 | `throw new ArgumentNullException()` missing `paramName` | Warning | Error handling | -| 5 | `DataProcessor` is unsealed with no virtual members | Warning | Extensibility | -| 6 | `Checksum` property does expensive computation (should be a method) | Suggestion | Member design | - -### Embedded strengths +`test-api.cs` — A C# file containing a `Contoso.Messaging` namespace implementing a message broker library. The code compiles and works correctly, but contains subtle API design convention violations that require .NET-specific knowledge to catch. + +### Embedded violations (hard — most require convention knowledge) + +| # | Violation | Severity | Category | Claude catches without skill? | +|---|-----------|----------|----------|-------------------------------| +| 1 | `MessageEnvelope` is a mutable struct >16 bytes with `Dictionary<>` field | Critical | Type design | Unlikely — compiles fine, struct size rule is .NET-specific | +| 2 | `[Flags] DeliveryMode` values aren't powers of two — `ExactlyOnce = 3` aliases `AtMostOnce | AtLeastOnce` silently | Critical | Type design | Unlikely — compiles fine, requires knowing flags must be powers of two | +| 3 | `ActiveTopics` returns `List` in public API | Critical | Collections | Sometimes — code works, convention is `ReadOnlyCollection` | +| 4 | `Subscribe()` uses different parameter names (`topicName`/`callback`) than `Publish()`/`Unsubscribe()` (`topic`/`handler`) — inconsistent across overloads of the same concept | Warning | Member design | Unlikely — each method compiles independently, requires cross-method convention check | +| 5 | `PublishAsync` validates arguments inside `Task.Run` instead of synchronously before first await | Warning | Error handling | Unlikely — both paths throw, requires knowing .NET async validation convention | +| 6 | `ArgumentNullException()` thrown without `paramName` in `Subscribe()` — two instances | Warning | Error handling | Sometimes — compiles fine | +| 7 | `ArgumentException("Topic is required")` instead of `ArgumentNullException(nameof(envelope.Topic))` for null topic | Warning | Error handling | Unlikely — both throw, requires knowing exception type hierarchy convention | +| 8 | `Retrieval()` method named with noun instead of verb | Warning | Naming | Sometimes — requires naming convention knowledge | +| 9 | `operator ==` defined without matching `operator !=`, `Equals`, or `GetHashCode` | Warning | Member design | Unlikely — compiles with warning only, requires knowing operator pair convention | +| 10 | `BrokerException` constructor uses `msg` parameter instead of `message` — abbreviation in public API | Warning | Naming | Unlikely — compiles fine, requires knowing abbreviation convention | +| 11 | `MessageResult.Parse()` throws bare `Exception` instead of `FormatException`; no `TryParse` variant | Warning | Error handling | Sometimes catches `Exception`; unlikely to flag missing `TryParse` | +| 12 | `IMessageBroker` interface has no async methods despite I/O-bound operations | Suggestion | Member design | Sometimes — requires domain judgment | +| 13 | `MessageBroker` unsealed with no virtual members | Suggestion | Extensibility | Sometimes | +| 14 | `ContainsKey` + indexer double-lookup in `Subscribe()` | Suggestion | Member design | Sometimes — functional but inefficient | + +### Things done well (strengths) | # | Good practice | Category | |---|---------------|----------| -| 1 | `FileWatcher` uses `EventHandler` with `protected virtual OnChanged` | Event pattern | -| 2 | `FileWatcher` implements `IDisposable` with standard dispose pattern | Resource management | -| 3 | `FileChangedEventArgs` derives from `EventArgs` with proper suffix | Naming / type design | +| 1 | `CancellationToken` is last parameter in `PublishAsync` | Member design | +| 2 | `BrokerException` derives from `Exception` with proper suffix | Naming | +| 3 | `ReadOnlyMemory` used for `Payload` (not `byte[]`) | Type design | ## Evaluation criteria -The skill adds value if the review: - -1. Loads the `reviewing-dotnet-api-design` skill -2. Writes sample calling code BEFORE reviewing (caller-first methodology) -3. Classifies the API surface (new library API / extension / modification) -4. Groups findings by severity (Critical → Warning → Suggestion) -5. Catches the mutable struct as Critical (not just a warning) -6. Catches the `List` return type as a convention violation (not just valid code) -7. Identifies the noun-named method with reference to naming conventions -8. Flags the missing `paramName` on `ArgumentNullException` -9. Notes strengths — correct event pattern and IDisposable implementation -10. Provides concrete before/after code fixes for each issue +The skill demonstrates unique value if the review: + +1. Writes sample calling code BEFORE reviewing (caller-first — the calling code should reveal that `MessageEnvelope` is awkward as a struct) +2. Classifies the API surface type (new library API) +3. Groups findings by severity (Critical → Warning → Suggestion) +4. Catches the struct size / mutability / reference-type field triple violation on `MessageEnvelope` +5. Catches the `[Flags]` enum with non-power-of-two values (`ExactlyOnce = 3`) +6. Catches the parameter name inconsistency across `Subscribe` vs `Publish`/`Unsubscribe` +7. Catches the async argument validation timing issue in `PublishAsync` +8. Catches the unpaired `operator ==` (missing `!=`, `Equals`, `GetHashCode`) +9. Flags the missing `TryParse` variant on `MessageResult.Parse` +10. Notes strengths — `CancellationToken` placement, `ReadOnlyMemory` usage diff --git a/tests/reviewing-dotnet-api-design/bad.md b/tests/reviewing-dotnet-api-design/bad.md index 576429d05c..053b5a3d1f 100644 --- a/tests/reviewing-dotnet-api-design/bad.md +++ b/tests/reviewing-dotnet-api-design/bad.md @@ -1,32 +1,42 @@ # Expected output WITHOUT skill -This represents the type of review Claude gives without the `reviewing-dotnet-api-design` skill loaded. The review is generic — it catches some issues but misses convention-specific violations, has no structured methodology, and lacks severity classification. +This represents the type of review Claude gives without the `reviewing-dotnet-api-design` skill loaded. Claude catches general code quality issues but misses .NET-specific API design conventions. ## Characteristics of a without-skill review -- **No caller-first methodology** — jumps straight into reviewing code, never writes sample calling code -- **No API surface classification** — doesn't identify whether this is a new API, extension, or modification -- **No severity grouping** — lists issues in arbitrary order without Critical/Warning/Suggestion classification -- **Misses `List` convention** — the code compiles and works fine, so a generic review won't flag `List` as a public API issue -- **May not flag the mutable struct** — `ConnectionInfo` is valid C# and works as written; flagging it requires knowing the value-type-with-reference-semantics convention -- **Misses `paramName`** — `throw new ArgumentNullException()` compiles fine; knowing to require `paramName` is convention-specific -- **No breaking change assessment** — doesn't evaluate what changes would break consumers -- **No reference to specific conventions** — generic advice like "consider making this immutable" without citing established patterns +- **No caller-first methodology** — reviews code top-to-bottom, never writes calling code to test usability +- **No API surface classification** — doesn't identify this as a library API with different conventions than application code +- **No severity grouping** — flat list of observations +- **Misses struct size convention** — `MessageEnvelope` compiles fine; Claude won't know the ≤16 byte soft limit +- **Misses `[Flags]` power-of-two rule** — `ExactlyOnce = 3` compiles and works; requires knowing bitwise combination semantics +- **Misses parameter name consistency** — `topicName` vs `topic` across methods is invisible without cross-method convention checking +- **Misses async validation timing** — both paths throw the same exception; requires knowing the .NET convention for immediate vs deferred validation +- **Misses operator pair convention** — `operator ==` without `!=`/`Equals`/`GetHashCode` generates a warning but Claude likely won't explain the convention depth +- **Misses `TryParse` convention** — `Parse` that throws is valid; knowing to also provide `TryParse` is .NET-specific +- **Catches some obvious issues** — bare `Exception`, `List` (maybe), noun-named method (maybe) ## Example without-skill output -Here are some observations about this code: +Here are some observations about the `Contoso.Messaging` API: -1. **`ConnectionInfo` struct** — Consider making this immutable. Mutable structs can lead to confusing behavior with value copy semantics. The `Tags` list is a reference type which may cause unexpected sharing. +1. **`MessageEnvelope` struct** — This struct has mutable properties and a `Dictionary` field. Consider whether a class might be more appropriate since it contains reference types. -2. **`DataProcessor.Result` method** — The name `Result` is a bit unclear. Consider renaming to something more descriptive like `ProcessData` or `ComputeResult`. +2. **`DeliveryMode` enum** — Has `[Flags]` attribute. Note that `ExactlyOnce = 3` which is the combination of the other two values — this might be intentional for bitwise combination. -3. **Null check** — You're throwing `ArgumentNullException` without specifying the parameter name. Consider using `throw new ArgumentNullException(nameof(data))`. +3. **`Subscribe` method** — The null checks throw `ArgumentNullException` without parameter names. Consider adding `nameof()`. -4. **`GetItems` return type** — Looks fine, returns a `List`. +4. **`Publish` method** — Throws `ArgumentException` for a null topic. The error message is okay but you might want to be more specific. -5. **`Checksum` property** — This property appears to do expensive work (thread sleep and hash computation). Properties should generally be lightweight. +5. **`PublishAsync`** — Uses `Task.Run` which isn't ideal for a library. Consider using truly async I/O instead. -6. **`FileWatcher`** — Good use of the dispose pattern and events. +6. **`Retrieval` method** — The name is a bit unusual. Consider `Retrieve` or `GetMessage` instead. -Overall the code looks reasonable. Consider adding XML documentation comments and making `ConnectionInfo` a class instead of a struct if it needs mutable behavior. +7. **`ActiveTopics`** — Returns a `List`. Consider whether an `IReadOnlyList` would be more appropriate. + +8. **`operator ==`** — You've defined `==` but the compiler will warn about missing `!=` and `GetHashCode`. Consider implementing those too. + +9. **`MessageResult.Parse`** — Throws generic `Exception`. Use a more specific exception type. + +10. **`BrokerException`** — Good use of a custom exception type. + +Overall the code structure is reasonable. The main areas to improve are exception handling specificity and the struct vs class decision for `MessageEnvelope`. diff --git a/tests/reviewing-dotnet-api-design/good.md b/tests/reviewing-dotnet-api-design/good.md index c3d715ef8c..68a97e7932 100644 --- a/tests/reviewing-dotnet-api-design/good.md +++ b/tests/reviewing-dotnet-api-design/good.md @@ -1,128 +1,164 @@ # Expected output WITH skill -This represents the type of review Claude gives with the `reviewing-dotnet-api-design` skill loaded. The review follows the 9-step structured workflow, uses caller-first methodology, classifies findings by severity, and references specific conventions. +This represents the type of review Claude gives with the `reviewing-dotnet-api-design` skill loaded. The review follows the 9-step structured workflow with convention-specific catches that require the reference files. ## Characteristics of a with-skill review -- **Loads the skill** — invokes `reviewing-dotnet-api-design` skill before starting the review -- **Caller-first** — writes sample calling code for top scenarios before analyzing the API -- **Surface classification** — identifies this as a "New library API" requiring full review -- **Severity grouping** — Critical → Warning → Suggestion ordering -- **Catches `List`** — flags as Critical because `List` exposes internal implementation; convention is `Collection` or `ReadOnlyCollection` -- **Catches mutable struct** — flags as Critical because `ConnectionInfo` has mutable properties, reference-type fields, and side-effecting methods -- **Convention-specific naming** — identifies `Result()` as noun-named method, cites verb convention -- **Error handling conventions** — flags missing `paramName`, recommends `ArgumentNullException.ThrowIfNull` -- **Notes strengths** — praises correct event pattern and IDisposable implementation -- **Before/after fixes** — concrete code examples for each issue +- **Loads the skill** — invokes `reviewing-dotnet-api-design` skill +- **Caller-first** — writes calling code that reveals `MessageEnvelope` is awkward as a struct (value-copy semantics on a large, mutable type) +- **Surface classification** — identifies as "New library API" for NuGet distribution +- **Severity grouping** — Critical → Warning → Suggestion with specific criteria +- **Catches struct triple violation** — mutability + >16 bytes + reference-type field, with size calculation +- **Catches `[Flags]` non-power-of-two** — `ExactlyOnce = 3` silently aliases the combination; flags values must be powers of two +- **Catches parameter name inconsistency** — `topicName`/`callback` vs `topic`/`handler` across methods +- **Catches async validation timing** — argument validation must happen synchronously before first `await` +- **Catches operator pair** — `==` without `!=`, `Equals`, `GetHashCode` violates operator convention +- **Catches missing `TryParse`** — `Parse` method that throws should have a `TryParse` companion +- **Notes strengths** — correct `CancellationToken` placement, `ReadOnlyMemory` for payload ## Example with-skill output ### Surface Classification -**Type:** New library API -**Context:** `Contoso.Networking` namespace — appears to be a library for network connectivity +**Type:** New library API (NuGet package) +**Namespace:** `Contoso.Messaging` — pub/sub message broker ### Scenario Code ```csharp -// Scenario 1: Create a connection -var info = new ConnectionInfo { Host = "example.com", Port = 443 }; -info.Connect(); - -// Scenario 2: Process data -var processor = new DataProcessor(); -string encoded = processor.Result(data); -List items = processor.GetItems(); - -// Scenario 3: Watch for file changes -using var watcher = new FileWatcher(); -watcher.Changed += (s, e) => Console.WriteLine(e.FilePath); +// Scenario 1: Publish a message +var envelope = new MessageEnvelope +{ + Topic = "orders.created", + Payload = payloadBytes, + Headers = new Dictionary { ["content-type"] = "application/json" }, + SentAt = DateTimeOffset.UtcNow +}; +var broker = new MessageBroker(); +broker.Publish(envelope); + +// Scenario 2: Subscribe to a topic +broker.Subscribe("orders.created", msg => Console.WriteLine(msg.Topic)); + +// Scenario 3: Parse a result +var result = MessageResult.Parse("OK|"); ``` -**Assessment:** Scenario 1 is problematic — `ConnectionInfo` is a value type, so `info.Connect()` operates on a copy in some contexts. Scenario 2 has awkward naming (`Result` is unclear) and leaks internal `List`. Scenario 3 is clean. +**Assessment:** Scenario 1 is awkward — `MessageEnvelope` is a struct but requires initializing 5 properties including a `Dictionary`. Assigning this to another variable copies all fields, but the `Dictionary` is shared by reference. This is a class, not a struct. Scenario 2 is clean. Scenario 3 has no safe alternative if the format is wrong. ### 🔴 Critical -#### C1. Mutable Struct with Reference Semantics — `ConnectionInfo` +#### C1. `MessageEnvelope` — Mutable struct with reference-type field, exceeds 16-byte guideline + +**Convention:** Structs should be ≤16 bytes, immutable (`readonly struct`), with no reference-type fields. `MessageEnvelope` has 5 properties including `Dictionary` and `string` fields — far exceeding 16 bytes and creating value-copy confusion. -**Convention:** Structs should be small, immutable, represent single values, have no reference-type fields, and have no side effects. (`DateTime`, `TimeSpan`, `Guid` are the model.) +**Fix:** Convert to `sealed class` or `readonly record struct` (if immutable and small). -**Issue:** `ConnectionInfo` is mutable (`set` accessors), has a `List` field (reference type causes unexpected sharing on copy), and has a `Connect()` method (side effect on value type). +#### C2. `[Flags] DeliveryMode` — Values are not powers of two + +**Convention:** `[Flags]` enum values must be powers of two (1, 2, 4, 8...) so bitwise combination works correctly. `ExactlyOnce = 3` silently equals `AtMostOnce | AtLeastOnce`, making it impossible to distinguish. **Fix:** ```csharp -// ❌ Before -public struct ConnectionInfo { ... } - -// ✅ After — Class with identity and side effects -public sealed class ConnectionInfo : IDisposable +[Flags] +public enum DeliveryMode { - public ConnectionInfo(string host, int port) { ... } - public string Host { get; } - public int Port { get; } - public void Connect() { ... } - public void Dispose() { ... } + AtMostOnce = 1, + AtLeastOnce = 2, + ExactlyOnce = 4 } ``` -#### C2. `List` Return Type in Public API — `DataProcessor.GetItems()` +Or remove `[Flags]` if these are mutually exclusive modes (use singular `DeliveryMode` without `[Flags]`). -**Convention:** Public APIs return `Collection` or `ReadOnlyCollection`, not `List`. `List` exposes `Add`, `Sort`, `Reverse` etc. which callers should not use to mutate the internal collection. +#### C3. `ActiveTopics` returns `List` in public API -**Fix:** -```csharp -// ❌ Before -public List GetItems() => new List(); +**Convention:** Public APIs return `ReadOnlyCollection` or `IReadOnlyList`, not `List`. -// ✅ After -public ReadOnlyCollection GetItems() => new List().AsReadOnly(); -``` +**Fix:** Return `ReadOnlyCollection` or `IReadOnlyList`. ### 🟡 Warning -#### W1. Noun-Named Method — `DataProcessor.Result()` +#### W1. Parameter names inconsistent across related methods -**Convention:** Methods use verbs or verb phrases (`Read`, `Parse`, `ComputeHash`). Properties use nouns. +**Convention:** Parameters representing the same concept must use identical names across all methods. -**Fix:** Rename to `ComputeResult(byte[] data)` or `Encode(byte[] data)`. +| Method | Topic param | Handler param | +|--------|------------|---------------| +| `Publish` | `envelope` (contains `.Topic`) | — | +| `Subscribe` | `topicName` ← inconsistent | `callback` ← inconsistent | +| `Unsubscribe` | `topic` | — | -#### W2. Missing `paramName` on ArgumentNullException +**Fix:** Standardize to `topic` and `handler` across all methods. -**Convention:** Always pass `nameof(parameter)` or use `ArgumentNullException.ThrowIfNull()`. +#### W2. `PublishAsync` validates arguments inside `Task.Run` + +**Convention:** Async methods must validate arguments synchronously before the first `await`. Deferring validation into the task means the caller gets a faulted `Task` instead of an immediate exception at the call site. **Fix:** ```csharp -// ❌ Before -throw new ArgumentNullException(); +public Task PublishAsync(MessageEnvelope envelope, CancellationToken token) +{ + ArgumentNullException.ThrowIfNull(envelope.Topic); + return PublishAsyncCore(envelope, token); +} -// ✅ After -ArgumentNullException.ThrowIfNull(data); +private async Task PublishAsyncCore(MessageEnvelope envelope, CancellationToken token) +{ + await Task.Run(() => Publish(envelope), token); +} ``` -#### W3. Unsealed Leaf Class — `DataProcessor` +#### W3. `operator ==` without `!=`, `Equals`, or `GetHashCode` + +**Convention:** Operators must be implemented in pairs. `==` requires `!=`, and both require consistent `Equals(object)` and `GetHashCode()` overrides. -**Convention:** Classes with no virtual members and not designed for inheritance should be sealed. +**Fix:** Implement `!=`, override `Equals(object)`, override `GetHashCode()`, and implement `IEquatable`. -**Fix:** Add `sealed` modifier: `public sealed class DataProcessor` +#### W4. `ArgumentNullException` without `paramName` in `Subscribe()` + +**Convention:** Always pass `nameof(parameter)`. Use `ArgumentNullException.ThrowIfNull()` (.NET 6+). + +#### W5. `ArgumentException` for null topic in `Publish()` — wrong exception type + +**Convention:** Null arguments get `ArgumentNullException`, not `ArgumentException`. + +#### W6. `BrokerException(string msg)` — abbreviated parameter name + +**Convention:** No abbreviations in public APIs. Use `message` not `msg`. + +#### W7. `MessageResult.Parse` throws bare `Exception`; no `TryParse` variant + +**Convention:** Use `FormatException` for parse failures. Provide a `TryParse(string, out MessageResult)` companion for callers who expect failures. + +#### W8. `Retrieval()` — noun-named method + +**Convention:** Methods use verbs. Rename to `Retrieve()` or `GetNextMessage()`. ### 💡 Suggestion -#### S1. Property Does Expensive Work — `DataProcessor.Checksum` +#### S1. `MessageBroker` unsealed with no virtual members + +**Convention:** Seal classes not designed for inheritance. No virtual members = not designed for extension. + +#### S2. `IMessageBroker` has no async methods + +For an I/O-bound broker, consider adding `PublishAsync`/`SubscribeAsync` to the interface. -**Convention:** Properties should be cheap and idempotent. Expensive operations should be methods (`ComputeChecksum()`). +#### S3. `ContainsKey` + indexer double-lookup in `Subscribe()` -**Fix:** Convert to method: `public byte[] ComputeChecksum()` +**Convention:** Use `TryGetValue` for single-lookup pattern. ### ✅ Strengths -- **Correct event pattern** — `FileWatcher` uses `EventHandler` with `protected virtual OnChanged()` -- **Standard IDisposable** — `FileWatcher` implements the full dispose pattern with `Dispose(bool)` and `GC.SuppressFinalize` -- **Proper EventArgs** — `FileChangedEventArgs` derives from `EventArgs` with correct suffix naming +- **`CancellationToken` last** — `PublishAsync(envelope, token)` follows correct parameter ordering +- **`ReadOnlyMemory` for Payload** — Avoids mutable `byte[]` in the public surface +- **`BrokerException`** — Correct `Exception` suffix naming ### Summary -| Severity | Count | Issues | -|----------|-------|--------| -| 🔴 Critical | 2 | Mutable struct, `List` in public API | -| 🟡 Warning | 3 | Noun method name, missing paramName, unsealed class | -| 💡 Suggestion | 1 | Expensive property | +| Severity | Count | Key Issues | +|----------|-------|------------| +| 🔴 Critical | 3 | Mutable oversized struct, `[Flags]` non-power-of-two, `List` return | +| 🟡 Warning | 8 | Param name inconsistency, async validation timing, unpaired operator, missing TryParse, wrong exception types, abbreviation | +| 💡 Suggestion | 3 | Unsealed class, no async interface, double-lookup | diff --git a/tests/reviewing-dotnet-api-design/test-api.cs b/tests/reviewing-dotnet-api-design/test-api.cs index 3b436c2652..f35ca44bbe 100644 --- a/tests/reviewing-dotnet-api-design/test-api.cs +++ b/tests/reviewing-dotnet-api-design/test-api.cs @@ -1,87 +1,126 @@ using System; using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; -namespace Contoso.Networking; +namespace Contoso.Messaging; -// --- Deliberate violations for testing --- +// A message broker library for pub/sub messaging. +// Target: .NET 8, NuGet package. -// Critical: Mutable struct with reference-type field and side effects -public struct ConnectionInfo +public struct MessageEnvelope { - public string Host { get; set; } - public int Port { get; set; } - public List Tags { get; set; } + public string Topic { get; set; } + public string CorrelationId { get; set; } + public Dictionary Headers { get; set; } + public ReadOnlyMemory Payload { get; set; } + public DateTimeOffset SentAt { get; set; } +} - public void Connect() - { - // Side-effecting method on a value type - } +[Flags] +public enum DeliveryMode +{ + AtMostOnce = 1, + AtLeastOnce = 2, + ExactlyOnce = 3 } -// Warning: Unsealed leaf class with no virtual members -public class DataProcessor +public interface IMessageBroker { - // Warning: Method named with noun instead of verb - public string Result(byte[] data) - { - if (data == null) - // Warning: Missing paramName on ArgumentNullException - throw new ArgumentNullException(); + void Publish(MessageEnvelope envelope); + void Subscribe(string topic, Action handler); + void Unsubscribe(string topic); +} - return Convert.ToBase64String(data); - } +public class MessageBroker : IMessageBroker +{ + private readonly Dictionary>> _handlers = new(); - // Critical: List return type in public API - public List GetItems() - { - return new List(); - } + public string BrokerEndpoint { get; set; } - // Suggestion: Property that does expensive work (should be a method) - public byte[] Checksum + public List ActiveTopics { get { - // Expensive computation — violates property contract - System.Threading.Thread.Sleep(100); - return System.Security.Cryptography.SHA256.HashData(Array.Empty()); + var topics = new List(); + foreach (var kvp in _handlers) + if (kvp.Value.Count > 0) + topics.Add(kvp.Key); + return topics; } } -} -// --- Things done well (strengths) --- + public void Publish(MessageEnvelope envelope) + { + if (envelope.Topic == null) + throw new ArgumentException("Topic is required"); -// Correct event pattern -public class FileWatcher : IDisposable -{ - private bool _disposed; + if (_handlers.TryGetValue(envelope.Topic, out var handlers)) + foreach (var handler in handlers) + handler(envelope); + } + + public void Subscribe(string topicName, Action callback) + { + if (topicName == null) throw new ArgumentNullException(); + if (callback == null) throw new ArgumentNullException(); + + if (!_handlers.ContainsKey(topicName)) + _handlers[topicName] = new List>(); + _handlers[topicName].Add(callback); + } - public event EventHandler? Changed; + public void Unsubscribe(string topic) + { + _handlers.Remove(topic); + } - protected virtual void OnChanged(FileChangedEventArgs e) + public async Task PublishAsync(MessageEnvelope envelope, CancellationToken token) { - Changed?.Invoke(this, e); + await Task.Run(() => + { + if (envelope.Topic == null) + throw new ArgumentException("Topic is required"); + + Publish(envelope); + }, token); } - // Correct IDisposable pattern - public void Dispose() + public MessageEnvelope? Retrieval(string topic) { - Dispose(true); - GC.SuppressFinalize(this); + return null; } - protected virtual void Dispose(bool disposing) + public int MessageCount { - if (!_disposed) + get { - if (disposing) { /* release managed resources */ } - _disposed = true; + int count = 0; + foreach (var kvp in _handlers) + count += kvp.Value.Count; + return count; } } + + public static bool operator ==(MessageBroker left, MessageBroker right) + => left?.BrokerEndpoint == right?.BrokerEndpoint; +} + +public class BrokerException : Exception +{ + public BrokerException(string msg) : base(msg) { } } -public class FileChangedEventArgs : EventArgs +public class MessageResult { - public string FilePath { get; } - public FileChangedEventArgs(string filePath) => FilePath = filePath; + public bool Success { get; set; } + public string Error { get; set; } + + public static MessageResult Parse(string raw) + { + if (raw == null) throw new Exception("Input required"); + var parts = raw.Split('|'); + if (parts.Length != 2) throw new Exception("Bad format"); + return new MessageResult { Success = parts[0] == "OK", Error = parts[1] }; + } } From 826ffc9633313cc94838bdb6409db3dc71d4359c Mon Sep 17 00:00:00 2001 From: Art Leonard Date: Mon, 16 Feb 2026 16:37:24 -0800 Subject: [PATCH 5/5] Clarify guidelines for .NET API design review Removed instruction to avoid citing specific resources. --- agents/reviewing-dotnet-api-design.agent.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/agents/reviewing-dotnet-api-design.agent.md b/agents/reviewing-dotnet-api-design.agent.md index 49cab2370d..07d821d5bd 100644 --- a/agents/reviewing-dotnet-api-design.agent.md +++ b/agents/reviewing-dotnet-api-design.agent.md @@ -8,8 +8,6 @@ tools: ['shell', 'read', 'search', 'edit', 'task', 'skill', 'web_search', 'web_f You are a senior .NET API design reviewer. Help developers design and review .NET API surfaces that are consistent with established C# conventions. -You do NOT cite or reference the Pearson-licensed "Framework Design Guidelines" book or the learn.microsoft.com/en-us/dotnet/standard/design-guidelines/ pages. - ## Key Principles 1. **Caller-first design**: Start with the code a developer will write. If calling code is awkward, the API needs work.