What to Look for as a Reviewer: Reading Order and a Practical Checklist for .NET + Angular PRs

Version note: The code in this post was built and run with .NET 10 (SDK 10.0.402), EF Core 10.0.15, SQL Server 2025, xUnit.net v3 4.0.3, Angular 22.2 (TypeScript 6.0) and Vitest 5.1.

Guess First

The coupon feature of Bookstore, the online bookstore used as the shared example across this site's series, was opened as a three-layer stack in the previous post. Ayşe is now reviewing the top layer, the pull request that applies coupons to the cart and the order: 15 files, 332 lines, green tests, a decent description. Following the reading suggestion in the description, she starts with CouponService:

public sealed class CouponService(BookstoreContext db)
{
    public async Task<Coupon?> FindValidAsync(string code, decimal subtotal, CancellationToken ct)
    {
        string normalized = code.Trim().ToUpperInvariant();
        Coupon? coupon = await db.Coupons.AsNoTracking().FirstOrDefaultAsync(c => c.Code == normalized, ct);
        if (coupon is null || coupon.ExpiresAtUtc < DateTime.Now || subtotal < coupon.MinSubtotal)
        {
            return null;
        }
        return coupon;
    }

    public async Task<decimal> CalculateDiscountAsync(Coupon coupon, CartPrice price, CancellationToken ct)
    {
        decimal eligible = 0m;
        foreach (PricedLine line in price.Lines)
        {
            if (coupon.CategoryId is null)
            {
                eligible += line.LineTotal;
                continue;
            }
            int categoryId = await db.Books.AsNoTracking()
                .Where(b => b.Id == line.Book.Id)
                .Select(b => b.CategoryId)
                .FirstAsync(ct);
            if (categoryId == coupon.CategoryId)
            {
                eligible += line.LineTotal;
            }
        }
        return CouponRules.CalculateDiscount(coupon, eligible);
    }
}

After CLASSIC15 is applied to a cart with five different books, how many SQL commands does a single GET /api/carts/{id} send to the database? And in these 40 lines, how many problems do you see that none of the tests caught?

Reading Order: Purpose First, Then Design, Details Last

Reading a pull request from the first file to the last is the most common and least effective way. Google's Engineering Practices guide suggests three steps; Bookstore does the same.

The first step is the big picture. Read the description and decide whether the change should happen at all. How coupons are calculated was discussed with Selin, there is a story, and what was left out is written down. If "this pull request should not exist" needs saying, you say it now and with a reason; you do not get into line comments.

The second step is the main parts. Find the files with the most logic changes and judge the design there. If you see a design problem, send that comment immediately; do not hold it back until you have read the rest. The author is most likely building new work on top of this pull request; the later a design objection comes, the more code is thrown away. Reading the tests at this step helps too: tests are the shortest specification of what the change should do.

The third step is the remaining files. If the design holds up, you read the other files in a sensible order without skipping any. If the pull request is too large to follow that order, asking for it to be split is, as we discussed in the previous post, the reviewer's right.

This order has a trap: looking only at the diff. A diff shows the lines that changed; it does not show code that did not change but is affected by the change. One of the problems Ayşe found is exactly in such a place.

Design and Performance: Module Boundary and N+1

The answer to the guess is eight. Running the API with EF Core's command log on and fetching the cart once gives eight Executed DbCommand lines in the log: the cart, the books, the coupon and a separate category query per line. The last five are identical:

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (1ms) [Parameters=[@line_Book_Id='?' (DbType = Int32)], CommandType='Text', CommandTimeout='30']
      SELECT TOP(1) [b].[CategoryId]
      FROM [catalog].[Books] AS [b]
      WHERE [b].[Id] = @line_Book_Id
info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (1ms) [Parameters=[@line_Book_Id='?' (DbType = Int32)], CommandType='Text', CommandTimeout='30']
      SELECT TOP(1) [b].[CategoryId]
      FROM [catalog].[Books] AS [b]
      WHERE [b].[Id] = @line_Book_Id
...

This is a classic N+1: N separate queries for N lines. Each takes a millisecond, so nobody notices locally; but the same method runs both in the cart view and when placing an order, that is, inside the order transaction too. As the number of books in a cart and the number of carts open at the same time grow, the number of queries multiplies. The tests do not catch this, because tests check the result, not the query count.

Behind the N+1 there is a more important design problem. A comment sits at the top of CatalogService: this service is the only door other modules use to read the catalog. CouponService, however, is in the Ordering module and goes straight to the Catalog's table. Today there is one DbContext and it works; but the day Catalog moves to its own database or service, this line breaks. And the information needed is already at hand: when the cart is priced, the books are read in one query through ICatalogService.GetByIdsAsync. Ayşe's comment states both points together and suggests a fix: add the category to BookInfo and calculate the discount from the lines you already have. Emre's fix:

public sealed record BookInfo(int Id, string Isbn, string Title, decimal Price, int CategoryId);

// The priced lines already carry each book's category; no extra query per line.
public static decimal CalculateDiscount(Coupon coupon, CartPrice price)
{
    decimal eligible = price.Lines
        .Where(l => coupon.CategoryId is null || l.Book.CategoryId == coupon.CategoryId)
        .Sum(l => l.LineTotal);
    return CouponRules.CalculateDiscount(coupon, eligible);
}

The same request for the same cart now sends three commands: the cart, the books (with their category) and the coupon. The method is no longer async either; a calculation that does not touch the database can be tested as a pure function. Other forms of N+1 in EF Core and the cost of tracking are covered in the EF Core series; the sign to look for in review is simple: a query that goes to the database with await inside a loop.

Behaviour: Code That Did Not Change but Is Affected

When Ayşe sees the call to CouponRules.CalculateDiscount, she opens a file that is not in the diff. This function came in the second layer of the stack and was approved; this pull request does not change it, but it is the first to send it real data:

public static decimal CalculateDiscount(Coupon coupon, decimal eligibleSubtotal) => coupon.Kind switch
{
    CouponKind.Percent => Math.Round(eligibleSubtotal * coupon.Value / 100m, 2, MidpointRounding.AwayFromZero),
    CouponKind.Amount => coupon.Value,
    _ => throw new UnreachableException()
};

A fixed-amount coupon returns its value whatever the eligible amount is. WELCOME25 has no minimum cart total; on a 19.90 lira cart the discount is 25 lira and the total goes negative. With a category coupon, if the cart has no books from that category, the eligible amount is zero and the discount is still the coupon's value. The second layer's tests only tried the happy path. Instead of arguing, Ayşe writes two tests and runs them:

[Fact]
public void Amount_discount_never_exceeds_the_eligible_subtotal()
{
    Coupon welcome25 = new() { Code = "WELCOME25", Kind = CouponKind.Amount, Value = 25m };

    Assert.Equal(19.90m, CouponRules.CalculateDiscount(welcome25, 19.90m));
}

[Fact]
public void Nothing_eligible_means_no_discount()
{
    Coupon welcome25 = new() { Code = "WELCOME25", Kind = CouponKind.Amount, Value = 25m };

    Assert.Equal(0m, CouponRules.CalculateDiscount(welcome25, 0m));
}

failed Bookstore.Api.Tests.CouponRulesTests.Amount_discount_never_exceeds_the_eligible_subtotal (1ms)
  Assert.Equal() Failure: Values differ
  Expected: 19,90
  Actual:   25
failed Bookstore.Api.Tests.CouponRulesTests.Nothing_eligible_means_no_discount (0ms)
  Assert.Equal() Failure: Values differ
  Expected: 0
  Actual:   25
Test run summary: Failed!
  total: 20
  failed: 2
  succeeded: 18

A red test is more convincing than "I think there is a problem here". The fix is one line: CouponKind.Amount => Math.Min(coupon.Value, eligibleSubtotal).

A second behaviour problem in the same file is the DateTime.Now in the expiry check. Coupon times are stored in the database as UTC, while DateTime.Now returns the machine's local time, and DateTime comparison ignores Kind. A small experiment on a machine in the Istanbul time zone:

DateTime expiresAtUtc = new(2026, 12, 31, 20, 59, 59, DateTimeKind.Utc);
DateTime now = new DateTimeOffset(2026, 12, 31, 22, 30, 0, TimeSpan.FromHours(3)).LocalDateTime;

Console.WriteLine($"now (local) = {now:yyyy-MM-dd HH:mm} {now.Kind}, as UTC = {now.ToUniversalTime():HH:mm}");
Console.WriteLine($"expired?    = {expiresAtUtc < now}");
Console.WriteLine($"expired?    = {expiresAtUtc < now.ToUniversalTime()} (compared in UTC)");

$ dotnet run expiry.cs
now (local) = 2026-12-31 22:30 Local, as UTC = 19:30
expired?    = True
expired?    = False (compared in UTC)

The New Year campaign should run until midnight Turkish time; with this code the coupon looks invalid after 9 pm. If the production server runs on UTC the problem does not show; it shows on the developer's machine, and it cannot be tested. The application already registers TimeProvider in the dependency injection container; the fix is for CouponService to take it too and compare with clock.GetUtcNow().UtcDateTime.

Do the Tests Check Behaviour or Implementation?

The first coupon test in the pull request looked like this:

CartView? cart = await response.Content.ReadFromJsonAsync<CartView>(ct);
Assert.Equal(Math.Round(cart!.Subtotal * 10 / 100m, 2), cart.Discount);
Assert.Equal(cart.Subtotal - cart.Discount, cart.Total);

The test calculates the expected value by repeating the formula from the implementation. If the formula is wrong, the test calculates the same wrong value and stays green. There is a hidden difference in this test as well: the code uses MidpointRounding.AwayFromZero, the test the default rounding. Dune's price shows no difference; Emma's does:

decimal subtotal = 249.90m;
Console.WriteLine(Math.Round(subtotal * 15 / 100m, 2));
Console.WriteLine(Math.Round(subtotal * 15 / 100m, 2, MidpointRounding.AwayFromZero));

$ dotnet run round.cs
37,48
37,49

Ayşe suggests writing the expected values as concrete numbers calculated by hand and testing rejected coupons as well. What a test should protect and the Arrange-Act-Assert layout we discussed in the Unit Test series, in "Anatomy of a Good Unit Test: AAA, Naming, One Behavior and Readable Assertions". One of Emre's new tests:

[Fact]
public async Task CLASSIC15_discounts_only_classics()
{
    Guid cartId = await CreateCartWithAsync(Dune, Emma);

    CartView cart = await ApplyAsync(cartId, "classic15");

    // 15% of 249.90 = 37.485, rounded away from zero
    Assert.Equal("CLASSIC15", cart.CouponCode);
    Assert.Equal(37.49m, cart.Discount);
    Assert.Equal(701.41m, cart.Total);
}

This one test protects three things: case-insensitive code matching, the category filter and the rounding rule. Next to it came tests that check SAVE50 is rejected below the minimum total and that an unknown code returns 400.

Migration: Existing Data and the Old Version

The migration's designer file is not read, but its Up method always is. Ayşe's easiest path is to look at the SQL the migration will produce:

$ dotnet ef migrations script AddCoupons ApplyCoupons --project src/Bookstore.Api
BEGIN TRANSACTION;
ALTER TABLE [ordering].[Orders] ADD [CouponCode] varchar(32) NULL;

ALTER TABLE [ordering].[Orders] ADD [Discount] decimal(12,2) NOT NULL DEFAULT 0.0;

ALTER TABLE [ordering].[Orders] ADD [Subtotal] decimal(12,2) NOT NULL DEFAULT 0.0;

ALTER TABLE [ordering].[Carts] ADD [CouponCode] varchar(32) NULL;
...

Subtotal is a new required column with a default of zero. Applying the migration to a database that has an order placed before coupons and looking at that order:

Id Subtotal Discount Total CouponCode
-- -------- -------- ----- ----------
1 .00 .00 489.00 NULL

Old orders show a subtotal of zero. A report or an order history screen that reads this field shows wrong information. The fix is to add a backfill to the migration:

// Orders placed before coupons had no discount: their subtotal is their total.
migrationBuilder.Sql("UPDATE [ordering].[Orders] SET [Subtotal] = [Total] WHERE [Subtotal] = 0;");

Id Subtotal Discount Total
-- -------- -------- -----
1 489.00 .00 489.00

The backfill fixes the existing rows, but one question remains: what happens if the old version keeps running for a while during the deploy? The old code does not know Subtotal; orders it places after the migration are still saved with zero. Ayşe does not treat this as blocking, because in Bookstore's deploy two versions never take orders at the same time; but she asks for it to be written into the "Risk and rollback" section of the description. Where the old and new versions run side by side, the right path is expand-contract: first a nullable column, then code that both versions write, and making it required last. The details are in the Deployment Strategies series, in "Where Migrations Fit in a Deploy: Expand-Contract, EF Core Migration Bundles and Backward-Compatible Schemas".

Security and Personal Data: Logs, XSS and Authorization

The log line written when an order is placed looks like this in the API's console:

info: Bookstore.Ordering.OrderService[0]
      Order 2 placed by ada@example.com with coupon CLASSIC15, discount 136.32

An e-mail address is personal data. Logs are often open to a wider audience than the application database, kept longer and hard to cover in deletion requests. The log line's job is to make the order findable; the customer number is enough for that. The same line after the fix:

info: Bookstore.Ordering.OrderService[0]
      Order 2 placed by customer 2 with coupon BOOK10, discount 24.99

On the Angular side, the coupon description contains HTML that the marketing team writes in the back office (<strong>15%</strong> off classics). To show it, Emre switched off Angular's sanitizer:

private readonly sanitizer = inject(DomSanitizer);
protected readonly couponDescription = computed(() =>
  this.sanitizer.bypassSecurityTrustHtml(this.store.cart()?.couponDescription ?? ''));

bypassSecurityTrustHtml is a name that should always make you stop in a review. The description comes from the database; if a back office account is compromised, or one day another system writes the description, a stored XSS that runs script on every customer's cart page opens up. Ayşe shows it with a test again:

it('keeps the formatting but drops script-capable markup', async () => {
  // ...
  TestBed.inject(HttpTestingController).expectOne('/api/carts/cart-1').flush(cartWith(
    '<strong>15%</strong> off classics<img src="x" onerror="alert(document.cookie)">'));
  // ...
  expect(description.querySelector('strong')?.textContent).toBe('15%');
  expect(description.querySelector('img[onerror]')).toBeNull();
});

 FAIL   bookstore-web  src/app/cart/cart-page.spec.ts > CartPage coupon description > keeps the formatting but drops script-capable markup
AssertionError: expected <img src="x" …(1)></img> to be null

- Expected:
null

+ Received:
<img
  onerror="alert(document.cookie)"
  src="x"
/>

The fix is deleting code: DomSanitizer and computed go away, and the template uses [innerHTML]="store.cart()!.couponDescription" directly. Angular then passes the value through its own sanitizer; <strong> stays, onerror is dropped and the test passes. The full list of DOM XSS sinks in Angular and what to check in review are in the Frontend Security series, in "A DOM XSS Sink Inventory for Angular: ElementRef, Renderer2, Dynamic URLs, HTML-Producing Libraries and a Code Review Checklist".

On authorization Ayşe has a question, not a finding: the coupon endpoints require no authentication. In Bookstore the cart is anonymous and identified by an unguessable GUID; the existing cart endpoints work with the same model. Since the coupon does not change that model, the question is answered and closed in the pull request. Input validation, on the other hand, is done well: ApplyCouponRequest has [Required, StringLength(32)], the same length as the database column. Ayşe writes that as a comment too; saying what is good makes it more likely the same care is repeated in the next pull request.

Bookstore's Review Checklist

None of what Ayşe found is luck; all of it comes from the team's short checklist. The list is not a form but questions to keep in mind while reading:

• Behaviour: does the code do what the description claims? What happens with edge cases such as zero, negative values, an empty list, a boundary value, an expired record? Is there unchanged code that is now called with new data?
• Tests: are the expected values concrete numbers or a repetition of the implementation? Are the error and rejection paths tested?
• EF Core: queries inside a loop (N+1), unnecessary tracking, direct access to another module's table.
• Migrations: what happens to existing rows, can the old version keep running after the migration, does Down really undo it?
• Security: authorization checks, input validation, personal data in logs, UTC and rounding in time and money calculations.
• Angular: use of bypassSecurityTrust*, innerHTML and ElementRef.nativeElement; subscribe calls in components without takeUntilDestroyed; a computed signal instead of calling a method in the template.

What is not on the list matters too: indentation, whitespace, use of var, naming format, unused using directives. Those are a tool's job, not a person's. In Bookstore, TreatWarningsAsErrors in Directory.Build.props turns analyzer warnings into build errors; how formatters and analyzers are wired into a CI gate is in the Coding Conventions and Team Standards series, in "Automating the Rules: Pre-Commit Hooks, CI Gates, Gradual Adoption in Legacy Codebases and .git-blame-ignore-revs". The deeper side of analyzer rules is the subject of the Static Analysis series. Update the checklist the second time you write the same comment: a rule that was repeated as a comment on two pull requests should move either to the list or to a tool.

At the end of the review Emre adds five commits: the discount limit, the category and TimeProvider, the migration backfill, concrete tests and the Angular sanitizer. The final state:

$ dotnet test
Test run summary: Passed!
  total: 23
  failed: 0
  succeeded: 23
  skipped: 0
  duration: 2s 211ms
$ cd web/bookstore-web && npm test -- --watch=false
 Test Files  2 passed (2)
      Tests  2 passed (2)

In Production

Every problem caught in review brings a question with it: what would catch this before review next time? For N+1, a check in the integration tests that counts queries, or watching the number of commands per request in the EF Core logs; for DateTime.Now, an analyzer that keeps a list of banned APIs; for personal data in logs, a rule that looks at field names in structured logging. Review's job is not to find the same mistake every time but to hand the recurring mistake over to a tool.

The reviewer's time is a resource too. This pull request's review took close to an hour, because Ayşe wrote and ran the tests herself and tried the migration on a real database. That is not done for every pull request; but it should be for pull requests touching migrations, money calculations and security. Which pull requests need that much attention can be signalled by labels and CODEOWNERS; we will look at those in the fifth post.

Common Pitfalls

Looking only at the diff. CouponRules did not change in this pull request, but its bug became visible with it. Opening the code that a changed line calls, or that calls it, is the most skipped step of review.

Applying the checklist mechanically. Ticking "Authorization: present?" is easy; the real question is whether this endpoint's authorization model is consistent with the rest of the system. The list is there to trigger thinking, not to replace it.

Trusting the tests without reading them. Assuming the tests check the right thing because CI is green lets a test that repeats the implementation hide every mistake.

Writing everything with the same weight. If an N+1, a stored XSS and a naming suggestion are written in the same tone, the author cannot tell which one blocks the merge. We will talk about ways to say clearly which comments are blocking in the next post.

Saving the design comment for the end. Ayşe sent the module boundary comment on her first read; Emre could start fixing it while the other comments were still coming.

A Small Experiment

★☆☆ Turn on the EF Core command log in your own project (set the Microsoft.EntityFrameworkCore.Database.Command category to Information) and call your most used endpoint once. How many SQL commands do you see, and which of them come from inside a loop?

★★☆ In Bookstore, FindValidAsync now takes a TimeProvider. Write an integration test that checks an expired coupon is rejected, using a TimeProvider whose clock can be moved forward. Which service do you need to replace in WebApplicationFactory?

★★★ Sort the review comments on your team's last ten pull requests under the headings of this post's checklist. Is there a heading that got no comments? Find the places where the same comment was written twice, and for each suggest an analyzer, a test or a CI check that would catch it.

Ayşe knew what to look for and found it. But how she writes what she found matters as much as what she found: the same N+1 comment can either make Emre think or put him on the defensive. In the next post we move to the communication side of review: the tone of comments, Conventional Comments, how to handle disagreement and review fatigue.