Let’s be honest: most of us have a love-hate relationship with code reviews. On a good day, they’re a brilliant collaboration that catches bugs and spreads knowledge. On a bad day, they feel like a bureaucratic bottleneck where personal opinions and style nits grind progress to a halt.
The difference isn’t magic. It’s about having a system. A good c# code review isn’t just about finding mistakes; it’s a force multiplier for your team’s quality, consistency, and velocity. It’s the highest-leverage activity you can do to improve your codebase, short of a full rewrite (and nobody wants that).
So, how do we get more of the good days?
It starts by agreeing on what we’re looking for and how we talk about it.
The Social Contract of a Good PR
Before we even get into the code, let’s talk about the human element. A pull request is a conversation, and every good conversation has some ground rules. Without them, you get chaos.
For the Author: Set Your Reviewer Up for Success
You wouldn’t ask someone to proofread a novel without telling them the plot. Don’t throw a 50-file PR at your team with a title like “bug fixes” and expect a high-quality review.
Write a great PR description.
What’s the “why” behind this change? What problem are you solving? Link to the ticket. Include screenshots of UI changes. If you made a specific architectural decision, explain your reasoning. Give your reviewer the context they need to understand your choices.
Self-review first.
Seriously. Read through your own changes file by file. You’ll be amazed at the number of typos, forgotten `console.log`s, and simple logic errors you catch. This shows respect for your reviewer’s time.
Keep it small.
The single biggest enemy of a good review is a massive PR. It’s impossible to properly review 2,000 lines of changes. The reviewer gets fatigue, skims, and just rubber-stamps it with an “LGTM.” Aim for atomic changes that solve one problem.
For the Reviewer: Be a Coach, Not a Cop
Your job isn’t to punish the author for mistakes. It’s to help them—and the team—ship better code. Your mindset is everything.
– Ask questions, don’t make demands. Instead of “Change this to a dictionary,” try “I’m curious about using a list here. Would a dictionary give us faster lookups if this collection gets large?” This opens a dialogue instead of shutting it down.
– Review the code, not the coder. Keep feedback focused on the technical implementation. “This method is getting a bit long” is great feedback. “Why do you always write long methods?” is not.
– Offer solutions, or at least point in the right direction. If you spot a problem, suggest a better way. “This could lead to an N+1 query” is good. “This could lead to an N+1 query; maybe we can use `Include()` here to eager-load the related data?” is even better.
– Automate the small stuff. Don’t waste time and goodwill on style nits like brace placement or spacing. That’s a job for a machine. Use a code formatter and a linter. More on this later.
What to Really Look for in a PR
Okay, the PR is small and the context is clear. We’re all being nice to each other. Now what? When you’re staring at the diff, where should you focus your attention? I break it down into a few key areas.
Clarity and Maintainability
This is the foundation. If you can’t understand the code, you can’t verify its correctness or performance. Future You will thank you for being ruthless here.
Is the intent clear? – Can you understand what a method does just by its name and signature? Are variable names descriptive (`customer` vs. `c`)? Modern C# gives us so many tools for expression—records, pattern matching, LINQ—are we using them to make code *simpler*?
Is it consistent? – Does the code follow the established patterns and conventions of the project? Consistency is a feature. It makes the codebase predictable and easier to navigate.
Are the comments explaining *why*, not *what*? – A comment like `// Increment i` is useless. A comment like `// We have to iterate backwards here because we’re removing items from the collection` is gold.
Architecture and Design
This is where we move from the line-by-line to the bigger picture. Are these changes setting us up for future success or painting us into a corner?
Is it testable? – This is the canary in the coal mine for good design. If the code is hard to test, it’s probably too tightly coupled. Are we using dependency injection to provide dependencies instead of `new`ing them up inside a class? Can we mock the external services this code talks to?
Does it follow SOLID principles (within reason)? – A class shouldn’t have ten different responsibilities (Single Responsibility). You should be able to extend behavior without modifying existing code (Open/Closed). We don’t need to be dogmatic, but these principles are fantastic guides for building flexible software.
Are we introducing new dependencies wisely? – Adding a new NuGet package is easy. But each one adds to our dependency tree, potential security vulnerabilities, and maintenance overhead. Is this new package necessary? Is it well-maintained?
Performance and Resource Management
In C#, it’s deceptively easy to write code that *works* but is secretly slow or leaky. These are often invisible until production traffic hits.
Watch out for `async/await` footguns – Is there any `async void`? It’s almost always a bad idea outside of event handlers. Are we using `ConfigureAwait(false)` in library code where appropriate? Are we accidentally creating a deadlock by mixing blocking calls (`.Result`, `.Wait()`) with async code?
Resource disposal is key – Anything that implements `IDisposable` (database connections, file streams, `HttpClient`) must be disposed of. The `using` statement (or `using` declaration in C# 8+) is your best friend. A missing `using` is a guaranteed resource leak.
Efficient data handling – Is there a hidden N+1 query in a loop that’s calling an Entity Framework Core navigation property? Are we calling `.ToList()` or `.ToArray()` prematurely, pulling way more data into memory than we need? Are we using the right data structure for the job (e.g., `HashSet` for fast lookups)?
C# code review focused on security and robustness
This is about building resilient code that can withstand weird inputs and unexpected failures.
Never trust input – Are we validating data coming from the outside world (API requests, user forms)? Are we using parameterized queries to prevent SQL injection, or are we concatenating strings to build SQL? (Please say it’s the former).
Handle nulls gracefully – With nullable reference types enabled (and you should enable them), the compiler helps a ton. But we still need to be vigilant. What happens if a key method returns `null`? Does the calling code handle it, or does it crash with a `NullReferenceException`?
Error handling and logging – Is the code just swallowing exceptions with an empty `catch` block? That’s a ticking time bomb. We should handle exceptions we can recover from and let the unhandled ones bubble up to a global handler. Are we logging enough information to diagnose a problem in production?
The Ultimate C# Code Review Checklist
Here’s a practical, no-fluff checklist. You don’t have to check every single box on every single PR, but use it as a guide to prompt your thinking.
✅ Readability & Style
- Is the code easy to understand at a glance?
- Are names for variables, methods, and classes clear and unambiguous?
- Does it follow the project’s existing coding conventions?
- Are comments explaining *why* something is done, not *what* is done?
✅ Architecture & Design
- Does this change belong in this class/project? (Single Responsibility)
- Is the code testable? Are dependencies injected?
- Is it over-engineered? Is there a simpler solution?
- Does it introduce new third-party dependencies? If so, are they justified?
✅ Performance & Resource Management
- Is `async/await` used correctly (no `async void`, no `.Result` on tasks)?
- Are `IDisposable` objects properly disposed of with `using` statements?
- Is there potential for an N+1 query or inefficient data retrieval (e.g., premature `.ToList()`)?
- Are appropriate data structures being used for the task (e.g., `List` vs. `Dictionary` vs. `HashSet`)?
✅ Security
- Is all external input validated and sanitized?
- Are parameterized queries used for all database access to prevent SQLi?
- Is sensitive data (passwords, API keys) handled securely (e.g., not logged in plain text)?
✅ Robustness & Error Handling
- Are `null` return values or arguments handled correctly? (Especially important if not using nullable reference types).
- Are exceptions handled appropriately? Are they caught too broadly or swallowed silently?
- Is there enough logging to diagnose issues in production?
- Are edge cases considered (e.g., empty collections, zero values, failed network calls)?
✅ Testing
- Are there tests for the new logic?
- Do the tests cover the “happy path” as well as edge cases and failure modes?
- Are the tests easy to read and understand?
Fostering a culture of high-quality code review is one of the best investments a development team can make. It’s not about adding friction; it’s about building a shared understanding of what “good” looks like and helping each other get there. It makes the code better, and it makes every developer on the team better, too.
