Opening: Why Code Reviews Matter in .NET Teams
I've reviewed thousands of pull requests across e-commerce platforms, trading systems, and ride-hailing services over the past 5+ years. The difference between code that works and code that survives production is often invisible—buried in async patterns, query inefficiency, or error handling details. My .NET pull request checklist has evolved from painful incidents and late-night debugging sessions into a systematic way to catch issues before they become problems.
When I sit down to review someone's PR, I'm not looking for style nitpicks or naming preferences. I'm checking for patterns that will either scale smoothly or explode under load. This checklist covers five areas I've learned to never skip: async flow, blocking calls, database queries, error mapping, and test philosophy. Let me walk you through each one.
CancellationToken Passed Through Async Calls
CancellationToken is one of the most underused features in .NET, and I catch its absence in nearly every junior's PR. It's not just about politeness—it's about giving callers the ability to cancel work when it no longer matters. In a ride-hailing system, if a passenger cancels their request mid-flight, we need to propagate that cancellation through the entire chain: API → business logic → database queries → external API calls. Without CancellationToken, you're forcing the system to waste resources on work nobody wants.
Here's what I look for:
- Every async method accepts a
CancellationToken ct = defaultparameter - The token is passed to all downstream async calls
- Long-running loops check
ct.ThrowIfCancellationRequested() - Task.Delay, Task.Run, and external API calls receive the token
A common example from a CRM I worked on:
// ❌ Missing CancellationToken
public async Task> GetCustomersBySegmentAsync(string segment)
{
var customers = await _db.Customers
.Where(c => c.Segment == segment)
.ToListAsync();
foreach (var customer in customers)
{
await _emailService.SendAsync(customer.Email, "New offer");
}
return customers;
}
// ✅ With CancellationToken
public async Task> GetCustomersBySegmentAsync(
string segment,
CancellationToken ct = default)
{
var customers = await _db.Customers
.Where(c => c.Segment == segment)
.ToListAsync(ct);
foreach (var customer in customers)
{
ct.ThrowIfCancellationRequested();
await _emailService.SendAsync(customer.Email, "New offer", ct);
}
return customers;
}
The difference is subtle but critical. If your API client times out or the user navigates away, that outer CancellationToken gets triggered, and now your email loop stops instead of burning through your API rate limit.
No Blocking Calls: .Result, .Wait()
The moment I see .Result or .Wait() on an async method, I know we have a problem. These block the current thread and can cause deadlocks, thread pool starvation, and cascading timeouts. I've debugged production issues at 2 AM that came down to a single .Wait() in the wrong place.
The rule is absolute: in async code, never block. Use await all the way up. If you're in a synchronous context (like a console app entry point or legacy code), you have deeper problems than a checklist can solve.
Real scenario from an e-commerce platform:
// ❌ Blocking call in async context
public async Task ProcessOrderAsync(Order order)
{
// This blocks the thread waiting for payment
var paymentResult = _paymentService.ChargeAsync(order.Amount).Result;
if (!paymentResult.Success)
return OrderResult.Failed("Payment failed");
await _db.SaveChangesAsync();
return OrderResult.Success();
}
// ✅ Proper async all the way
public async Task ProcessOrderAsync(
Order order,
CancellationToken ct = default)
{
var paymentResult = await _paymentService.ChargeAsync(
order.Amount,
ct);
if (!paymentResult.Success)
return OrderResult.Failed("Payment failed");
await _db.SaveChangesAsync(ct);
return OrderResult.Success();
}
When you have many concurrent orders and all of them hit .Wait(), your thread pool gets exhausted. New requests queue up. Response times balloon. Users see timeouts. Then customer support gets flooded. All from four characters: `.Wait()`.
Queries Projected, No N+1, No Unbounded Lists
Database performance is where most .NET applications leak resources. I check every LINQ query for three things: projection, N+1 patterns, and unbounded result sets. This is the most common reason a PR gets sent back.
Projection means only selecting the columns you need:
// ❌ Fetches entire customer object into memory
var customers = await _db.Customers
.Where(c => c.City == "Colombo")
.ToListAsync();
var names = customers.Select(c => c.Name).ToList();
// ✅ Selects only the column needed
var names = await _db.Customers
.Where(c => c.City == "Colombo")
.Select(c => c.Name)
.ToListAsync();
N+1 happens when you query for a parent, then loop through and query for each child:
// ❌ N+1: 1 query for orders + N queries for items
var orders = await _db.Orders.Where(o => o.UserId == userId).ToListAsync();
foreach (var order in orders)
{
var items = await _db.OrderItems
.Where(oi => oi.OrderId == order.Id)
.ToListAsync();
order.Items = items;
}
// ✅ Single query with Include
var orders = await _db.Orders
.Where(o => o.UserId == userId)
.Include(o => o.Items)
.ToListAsync();
Unbounded lists kill your API and database:
// ❌ Someone requests all 10 million customers at once
public async Task> GetAllCustomersAsync()
{
return await _db.Customers.ToListAsync();
}
// ✅ Pagination built in from the start
public async Task> GetCustomersAsync(
int pageNumber = 1,
int pageSize = 50,
CancellationToken ct = default)
{
const int maxPageSize = 200;
pageSize = Math.Min(pageSize, maxPageSize);
var skip = (pageNumber - 1) * pageSize;
var total = await _db.Customers.CountAsync(ct);
var items = await _db.Customers
.Skip(skip)
.Take(pageSize)
.ToListAsync(ct);
return new PagedResult(items, total, pageNumber, pageSize);
}
These three patterns compound. A single unbounded query with N+1 loading can turn a 50ms operation into a 5-second disaster.
Errors Mapped to ProblemDetails, Nothing Leaked
One of my most important rules: exceptions should never flow directly to the client. Stack traces, SQL errors, and internal state are information security doesn't want floating around. In ASP.NET Core, we have ProblemDetails. Use it.
I check that:
- A global exception handler exists and catches unhandled exceptions
- Domain exceptions map to meaningful HTTP status codes (409, 400, 404, not 500)
- No raw exception details reach the API response
- Logging happens server-side for debugging
Here's a middleware pattern I use in every API:
public class GlobalExceptionHandlerMiddleware
{
private readonly RequestDelegate _next;
private readonly ILogger _logger;
public GlobalExceptionHandlerMiddleware(
RequestDelegate next,
ILogger logger)
{
_next = next;
_logger = logger;
}
public async Task InvokeAsync(HttpContext context)
{
try
{
await _next(context);
}
catch (Exception ex)
{
_logger.LogError(ex, "Unhandled exception occurred");
await HandleExceptionAsync(context, ex);
}
}
private static Task HandleExceptionAsync(HttpContext context, Exception exception)
{
context.Response.ContentType = "application/problem+json";
var response = new ProblemDetails
{
Instance = context.Request.Path,
};
switch (exception)
{
case ValidationException ve:
context.Response.StatusCode = StatusCodes.Status400BadRequest;
response.Title = "Validation failed";
response.Status = StatusCodes.Status400BadRequest;
response.Detail = ve.Message;
break;
case NotFoundException nfe:
context.Response.StatusCode = StatusCodes.Status404NotFound;
response.Title = "Resource not found";
response.Status = StatusCodes.Status404NotFound;
break;
default:
context.Response.StatusCode = StatusCodes.Status500InternalServerError;
response.Title = "An error occurred";
response.Status = StatusCodes.Status500InternalServerError;
response.Detail = "Please contact support";
break;
}
return context.Response.WriteAsJsonAsync(response);
}
}
Register it early in your Startup:
app.UseMiddleware();
The key is: don't leak internals. A database connection string in an exception message is a security incident waiting to happen.
Tests Cover the Behavior, Not the Implementation
I've seen tests that break every time someone refactors, even when behavior doesn't change. Those tests are liabilities. Good tests verify outcomes, not internal implementation details.
Red flags I catch:
- Tests that verify
_mockService.Verify(x => x.Method())too many times - Tests that require private methods to be public or internal
- Tests that break when you reorganize code structure
- Tests that depend on specific class instantiation order
Example from a trading platform feature:
// ❌ Testing implementation, not behavior
[Fact]
public async Task ProcessTrade_CallsValidatorThenRepository()
{
// Strictly verifies method call order
var mockValidator = new Mock();
var mockRepo = new Mock();
var service = new TradeService(mockValidator.Object, mockRepo.Object);
await service.ProcessTradeAsync(trade);
mockValidator.Verify(v => v.ValidateAsync(trade), Times.Once);
mockRepo.Verify(r => r.SaveAsync(trade), Times.Once);
// This test fails if you refactor to validate in parallel later
}
// ✅ Testing behavior/outcome
[Fact]
public async Task ProcessTrade_ValidTrade_SavesSuccessfully()
{
var service = new TradeService(
new RealTradeValidator(),
new InMemoryTradeRepository());
var trade = new Trade { Price = 100, Quantity = 10 };
var result = await service.ProcessTradeAsync(trade);
Assert.True(result.Success);
Assert.Single(await service.GetTradesAsync());
}
[Fact]
public async Task ProcessTrade_InvalidPrice_ReturnsValidationError()
{
var service = new TradeService(
new RealTradeValidator(),
new InMemoryTradeRepository());
var trade = new Trade { Price = -10, Quantity = 10 };
var result = await service.ProcessTradeAsync(trade);
Assert.False(result.Success);
Assert.Contains("price", result.Error, StringComparison.OrdinalIgnoreCase);
}
The second approach is resilient. You can refactor the internals—add caching, use a different validator library, parallelize calls—and the test still passes. That's a good test.
Common Mistakes
Assuming CancellationToken is optional: I've had developers say "our endpoints never get cancelled." That's not how the real world works. Client network drops. Load balancers timeout. Azure functions are interrupted. Always pass the token through.
Using .AsEnumerable() instead of .ToListAsync(): This shifts the query to LINQ-to-Objects, pulling the entire table into memory first. Always materialize in the database layer.
Mapping exceptions too late: If you only catch exceptions at the controller level, you miss logging opportunities and context. Map early, log early, then re-throw or return a Result type.
Over-mocking in tests: Every mock you add makes your test more brittle and less useful. Mock external dependencies (APIs, databases). Don't mock the system under test.
Forgetting to paginate GET endpoints: Every GET endpoint that returns a collection should have a default page size. If you're not enforcing it, someone will request 50 million records in production.
Key Takeaways
- CancellationToken is not optional. Pass it through every async call so cancellations propagate correctly.
- Never block with .Result or .Wait() in async code. The entire point of async is to not block threads.
- Optimize queries at the source: project only needed columns, use Include() for relationships, and always paginate collections.
- Map errors centrally using middleware or filters. Never let raw exceptions reach the client.
- Test behavior, not implementation. Good tests survive refactoring because they verify outcomes, not internal details.
Comments
Loading comments…