Skill v1.0.1
currentAutomated scan100/1002 files
version: "1.0.1" name: code-review-csharp description: Perform structured code reviews of C# source code covering naming conventions, performance, security, readability, and .NET best practices. Trigger phrases include "review this C# code", "check my C# for best practices", "analyze this C# class", "find issues in my C# code".
Review C# Code for Best Practices
Perform a structured code review of C# source code. Identify issues related to naming conventions, performance, security, readability, and .NET best practices. Produce a prioritized report with actionable suggestions.
Gather Context
Before reviewing, determine:
- Code to review — the C# file, class, or snippet. If the user points to a file, read it. If they paste code, use it directly.
- Review focus — what to prioritize:
all(default) — full review across all categoriesnaming— naming conventions onlyperformance— performance and memory onlysecurity— security vulnerabilities onlyreadability— clarity, structure, and maintainability only
- Target framework — the .NET version. Default to .NET 8 if not specified. Auto-detect from
<TargetFramework>in.csprojif accessible. - Severity threshold —
all(default),warnings-and-above, orcritical-only.
If context is missing: Default to all focus, .NET 8, and all severities. Don't block the review to ask — just state your assumptions at the top of the report. Only ask if you genuinely have no code to review.
Report Format
# Code Review: {FileName or ClassName}> Target: .NET {version} | Focus: {focus} | Reviewed: {date}## Summary{One-paragraph overview. Lead with the most important finding. State the total issue count by severity.}## Issues| # | Severity | Location | Issue | Why It Matters | Suggestion ||---|----------|----------|-------|----------------|------------|| 1 | Critical | `Class.Method:L12` | {issue} | {impact} | {fix} || 2 | Warning | `Class.Property` | {issue} | {impact} | {fix} || 3 | Info | `Class.Method:L25` | {issue} | {impact} | {fix} |## What's Done Well-{Positive pattern — be specific, not generic}## Recommendations1.**[Critical]** {Highest-impact change}2.**[Warning]** {Next priority}3....
Severity levels:
- Critical — bugs, security vulnerabilities, data loss risks. Fix immediately.
- Warning — convention violations, performance pitfalls, maintainability concerns. Fix soon.
- Info — style preferences, minor modernization opportunities. Fix when convenient.
Review Categories
1. Structure and Design
- Single Responsibility Principle — does the class do too many things?
- File length and class complexity
- Namespace organization
- Unnecessary or missing
usingdirectives - Dependency injection — is
newused where DI should be? - Constructor complexity — too many dependencies suggests SRP violation
- Proper use of
sealedon classes not designed for inheritance
2. Naming Conventions
Verify against C# Coding Conventions:
| Element | Convention | Example | |
|---|---|---|---|
| Classes, records, structs | PascalCase | UserManager | |
| Interfaces | I + PascalCase | IUserRepository | |
| Methods | PascalCase | GetUserById | |
| Properties | PascalCase | FirstName | |
| Public constants/statics | PascalCase | MaxRetryCount | |
| Private fields | _camelCase | _userCount | |
| Local variables, parameters | camelCase | userId | |
| Enum types and values | PascalCase | OrderStatus.Pending | |
| Type parameters | T + PascalCase | TEntity, TKey | |
| Async methods | PascalCase + Async suffix | GetUserAsync | |
| Booleans | Is/Has/Can/Should prefix | IsActive, HasPermission | |
| Event handlers | PascalCase + EventArgs | OnUserCreated |
3. Performance
Check for:
- String concatenation in loops → suggest
StringBuilderorstring.Join - Unnecessary allocations: boxing, repeated
ToList(),newin hot paths async void→ should beasync Task(except event handlers)Task.ResultorTask.Wait()→ deadlock risk, useawaitinstead- Missing
ConfigureAwait(false)in library code - Sync-over-async (
Task.Run(() => AsyncMethod().Result)) IEnumerablemultiple enumeration → materialize withToList()or useIReadOnlyList- LINQ misuse:
Count() > 0→Any(),.Count()onICollection→.Count - Missing
IDisposable/using/await usingfor disposable resources - Unnecessary
Task.Runon already-async code - Large
structtypes being copied repeatedly (considerreforclass)
4. Security
Check for:
- SQL injection — string concatenation or interpolation in SQL → use parameterized queries
- Hardcoded secrets — connection strings, API keys, passwords in source → use configuration/secrets management
- Missing input validation — public method parameters not validated → add guard clauses
- Exception information leakage — returning stack traces or internal messages to callers → use generic error responses
- Null safety — missing null checks, inconsistent nullable reference type annotations
- Insecure cryptography — MD5/SHA1 for security purposes, custom crypto → use standard libraries
- Unsafe deserialization —
BinaryFormatter,TypeNameHandling.Allin JSON → use safe serializers - Path traversal — user input in file paths without sanitization
- `HttpClient` misuse — creating new instances per request → use
IHttpClientFactory - LINQ injection — dynamic LINQ from user input without sanitization
5. Readability and Maintainability
Check for:
- Methods over ~30 lines → suggest extraction
- Deeply nested conditionals (3+ levels) → suggest guard clauses or early returns
- Magic numbers and strings → extract to named constants
- Outdated or misleading comments (worse than no comments)
- Missing XML doc comments on public API members
- Inconsistent formatting and whitespace
- Modern C# opportunities (based on target framework):
- Pattern matching (
is,switchexpressions) - File-scoped namespaces (.NET 6+)
- Primary constructors (.NET 8+)
- Records for immutable data types
varusage consistency- Collection expressions (.NET 8+)
- Raw string literals (.NET 7+)
requiredproperties (.NET 7+)
Rules
Do:
- Explain why each issue matters, not just what to change
- Be specific about locations — class name, method name, and line number where possible
- Acknowledge good patterns — at least 1-2 things done well. A review that only lists problems is demoralizing.
- Respect the target framework — don't suggest
.NET 8features for a.NET 6project - Sort issues by severity: Critical → Warning → Info
- If the code has no issues, say so. Don't invent problems to fill a report.
- If asked to fix issues (not just review), apply the changes directly
Don't:
- Modify code unless the user explicitly asks you to apply fixes
- Flag framework-specific features as issues when they match the target version
- Report false positives — every issue must be verifiable in the code
- Be vague — "consider improving performance" is not actionable. Say exactly what to change and why.
- Overwhelm with low-severity noise — if there are many Info items, summarize them rather than listing each one
Example
Input:
public class userManager{string connStr = "Server=prod;Password=admin123";public void deleteUser(int id){var sql = "DELETE FROM Users WHERE Id = " + id;// execute sql}public List<string> GetNames(List<User> users){List<string> result = new List<string>();for (int i = 0; i < users.Count; i++){result.Add(users[i].FirstName + " " + users[i].LastName);}return result;}}
Output:
Code Review: userManager
Target: .NET 8 (assumed) | Focus: all | Reviewed: 2026-02-18
Summary
This class has 2 critical security issues that need immediate attention: hardcoded credentials and a SQL injection vulnerability. There are also naming convention violations and missed opportunities for modern C# patterns. 6 issues total (2 Critical, 3 Warning, 1 Info).
Issues
| # | Severity | Location | Issue | Why It Matters | Suggestion | |
|---|---|---|---|---|---|---|
| 1 | Critical | userManager.connStr | Hardcoded connection string with plaintext password | Credentials in source code end up in version control and are trivially extractable | Move to appsettings.json + secrets management (e.g., Azure Key Vault, User Secrets for dev) | |
| 2 | Critical | userManager.deleteUser:L6 | SQL injection via string concatenation | Attacker-controlled input can execute arbitrary SQL | Use parameterized queries or an ORM (e.g., Entity Framework, Dapper with parameters) | |
| 3 | Warning | Class declaration | userManager violates PascalCase convention | Inconsistent naming makes the codebase harder to navigate | Rename to UserManager | |
| 4 | Warning | deleteUser method | Method name violates PascalCase convention | Same as above | Rename to DeleteUser | |
| 5 | Warning | connStr field | Private field missing _ prefix and access modifier | Implicit private is less readable; missing prefix breaks convention | Rename to _connectionString with explicit private | |
| 6 | Info | GetNames method | Manual loop with string concatenation where LINQ would be clearer | LINQ expresses intent more directly and reduces boilerplate | users.Select(u => $"{u.FirstName} {u.LastName}").ToList() |
What's Done Well
GetNamesreturns a clearly typedList<string>— good use of explicit return types- Method names are descriptive of their intent (
deleteUser,GetNames)
Recommendations
- [Critical] Remove hardcoded credentials — externalize all secrets to configuration
- [Critical] Replace concatenated SQL with parameterized queries throughout the codebase
- [Warning] Apply consistent PascalCase naming to all classes, methods, and properties
- [Info] Modernize
GetNameswith LINQ and string interpolation
Pre-Delivery Checklist
Before finalizing the report, verify:
- [ ] Every reported issue is real and verifiable in the code — no false positives
- [ ] Each issue has a severity, location, explanation, and actionable suggestion
- [ ] At least 1-2 positive patterns acknowledged
- [ ] Recommendations are ordered by impact
- [ ] Suggestions are compatible with the target .NET version
- [ ] Security issues (if any) are marked Critical
- [ ] Report follows the defined format