Conducting Effective Code Reviews

1 Step 1: Know the Expectations

A reviewer who approves a pull request shares ownership of it. If it breaks production, "I only skimmed it" is not an excuse. At the same time, review should never become a bottleneck. These are the studio norms:

• Respond within one business day, even if only to say when you'll do the full review.
• Keep pull requests small. Around 400 changed lines is the practical limit for a careful review. Larger changes should be split into a series of pull requests.
• Authors review their own diff first, before asking anyone else to.
• Automate what can be automated. Formatting, linting, and tests run in CI. Humans focus on what tools can't judge.
• Junior developers review too. Reading other people's code is one of the fastest ways to learn a codebase, and questions from a fresh pair of eyes often expose unclear code.

2 Step 2: As the Author, Make the PR Reviewable

The quality of a review depends heavily on the pull request. Before requesting review, check each of these:

1. The branch is rebased on the latest Dev and CI is green (see the Daily Git Terminal Workflow).
2. You've read your own diff on GitHub, line by line, and removed debug logging, commented-out code, and unrelated changes.
3. The description explains what changed, why, and exactly how to test it, and links the issue.
4. UI or gameplay changes include screenshots or a short screen recording.
5. Work that isn't ready is opened as a draft pull request, so nobody reviews it early.

## Summary
<!-- What does this change, in one or two sentences? -->

Resolves #<issue-number>

## Why
<!-- The problem or requirement behind it. -->

## How to test
1.
2.

## Acceptance criteria
- [ ] <criterion copied from the issue>
- [ ] <criterion copied from the issue>

## Screenshots / notes for reviewers
<!-- Anything risky, anything you're unsure about, anything out of scope. -->

Save this as .github/pull_request_template.md in a repository and GitHub pre-fills it on every new pull request.

3 Step 3: Review in Passes, Big Picture First

Don't start at the first line of the first file. Read in passes, from the outside in, so you don't spend twenty minutes on variable names in code that should not exist at all.

1. Context: read the linked issue and the PR description. What problem is this solving?
2. Shape: skim the list of changed files. Is the change in the right places? Is anything missing, such as tests, migrations, or docs?
3. Run it: for anything non-trivial, check the branch out and try it.
4. Details: now read the code line by line, using the checklist in Step 4.

gh pr list --search "review-requested:@me"   # PRs waiting on you
gh pr view 123 --web                        # open PR #123 in the browser
gh pr checkout 123                          # check out its branch locally to run it
gh pr diff 123                              # read the diff in the terminal
gh pr checks 123                            # CI status

These use the GitHub CLI. See Claude Code & GitHub CLI Setup to install it. Run git switch Dev afterwards to leave the PR's branch.

4 Step 4: Know What to Look For

Work through these areas roughly in order of importance. A pull request with a design problem needs that conversation before anyone discusses style.

Correctness

• Does the code do what the issue asks, including the edge cases: empty lists, nulls, zero, negative numbers, very large inputs, and duplicate requests?
• Are errors handled, or silently swallowed? Will a failure leave data half-written?
• Is there shared state that two threads, requests, or players could change at the same time?
• Do the tests cover the new behavior, and would they fail if the code were wrong?

Architecture and design

• Does the change follow the repository's documented architecture (ARCHITECTURE.md) and existing patterns, or invent a new one without a good reason?
• Is the logic in the right layer? Business rules belong in the domain or service layer, not in controllers, UI components, or MonoBehaviours.
• Does it duplicate something that already exists?
• Multi-tenant SaaS: is every query, cache key, file path, and background job scoped to the tenant? A missing tenant filter is a data breach, not a bug, so treat it as blocking.
• Are public API contracts, database schemas, or save-file formats changed? If so, is the change backwards compatible, and is the migration reversible?

Security

• Is all external input validated? Are queries parameterized, never built by string concatenation?
• Does every new endpoint or action check that the caller is allowed to perform it?
• Are there secrets, tokens, or connection strings in the diff? Is personal data written to logs?

Performance

• Services: database calls inside loops (the "N+1" problem), unbounded queries without pagination, missing indexes on new filter columns, and synchronous calls that should be asynchronous.
• Games: work done every frame that could be cached. Watch for GetComponent, Find, LINQ, string building, or new allocations inside Update(); they cause garbage-collection stutters. Pool frequently spawned objects.
• Only raise performance concerns that matter at realistic scale. "This could be faster" isn't useful without "and it runs 10,000 times per request".

Readability and style

• Could a new team member understand this code in six months? Are names specific (expiredInvoices, not list2)?
• Do comments explain why, rather than restating what the code does?
• Does it match the conventions of the surrounding code? If the linter or formatter accepts it, don't argue about formatting by hand; propose a linter rule instead.

5 Step 5: Leave Constructive, Actionable Feedback

A good review comment is about the code, not the person; explains why; and makes clear what the author should do next. Start each comment with a label so the author knows how much weight to give it. The studio uses Conventional Comments (opens in a new tab):

This is wrong.
Why would you do it this way??
Use a dictionary.

issue: This query isn't filtered by tenantId, so an admin in one
tenant could see invoices from every tenant. Can we go through
InvoiceRepository.ForTenant() like the other endpoints do?

suggestion (non-blocking): FindItem() scans the whole list on each
call, and it's called once per inventory slot. A Dictionary keyed by
itemId would make this O(1). Fine to leave for a follow-up issue.

question: Why does the retry stop after 3 attempts? If it's a
platform limit, a comment saying so would help the next reader.

• Suggest the fix. For small changes, use GitHub's suggestion block (the ± button in the comment box) so the author can apply it in one click.
• Ask, don't assume. "What happens if items is empty?" invites a conversation; "this crashes" when it doesn't, ends one.
• Say "we" and "this code", not "you".
• Batch your comments. Use Start a review instead of Add single comment, so the author gets one notification with everything, plus your overall verdict.

Finish with one of three verdicts. Approve: ready to merge, possibly with non-blocking comments. Request changes: at least one blocking issue. Comment: questions only, no verdict yet. From the terminal:

gh pr review 123 --approve --body "Looks good. Two non-blocking suggestions inline."
gh pr review 123 --request-changes --body "Tenant filter missing on the invoice query; see inline."
gh pr review 123 --comment --body "A couple of questions before I approve."

6 Step 6: As the Author, Respond to Feedback

1. Reply to every comment, even if only "Done" or "Fixed in a1b2c3d". Silence leaves reviewers guessing.
2. Push fixes as new commits during review, so reviewers can use "Changes since last review". Avoid rebasing mid-review unless you need to; if you do, say so in a comment.
3. Disagreeing is fine. Explain your reasoning once. If a thread goes back and forth more than twice, talk it through on a call, then write the outcome in the thread.
4. Turn out-of-scope suggestions into new issues rather than growing the pull request.
5. Re-request review when you're ready, using the circular-arrow icon next to the reviewer's name.

7 Step 7: For Business Analysts: Verify Business Logic and Acceptance Criteria

A developer reviewer asks "is this code correct?"; a BA reviewer asks "is this the right behavior?". You don't need to read every line of code to do that well. Your job is to confirm that each acceptance criterion in the issue is met, that the business rules are implemented as agreed, and that nothing was added that nobody asked for.

1. Start from the issue, not the code. Open the linked issue and list every acceptance criterion. If a criterion is ambiguous, resolve it in the issue first; a pull request isn't the place to change requirements.
2. Map each criterion to evidence. For each one, find proof in the PR: a test whose name describes the rule, a screenshot, or a step you can run yourself in the preview or QA environment.
3. Read the tests, not the implementation. Well-named tests read like business rules, for example Invoice_IsOverdue_WhenUnpaid30DaysAfterDueDate. Check the numbers in them match the agreed rule: 30 days, not 31.
4. Scan for rule changes. In the Files changed tab, look for constants, configuration, feature flags, prices, rates, limits, and status names. Each change should trace back to a requirement.
5. Probe the edge cases you know from the domain. A tenant on a trial plan; a customer in a different time zone; a refund larger than the original charge; a month-end or leap-day date; a player with a full inventory. Ask whether each one is handled.
6. Check for scope creep. Behavior that wasn't in the issue may be welcome, but it needs to be agreed and documented rather than slipped in.
7. Record your verification as a review comment, so the evidence lives with the change.

## BA verification for #<issue-number>

- [x] AC1: Unpaid invoices show "Overdue" 30 days after the due date.
      Verified in QA with invoice INV-TEST-0042; test InvoiceTests.cs line 88.
- [x] AC2: Overdue invoices are excluded from the "Due soon" email.
      Verified with the email preview in QA.
- [ ] AC3: Tenant admins can change the overdue threshold.
      Not found in this PR. Is it planned for a follow-up issue?

Edge cases checked: due date on Feb 29; tenant in UTC+13.
Scope: no unrequested behavior found.

Use real test data from the QA environment, never production customer data, when recording evidence.