Code Review Skill

Guidelines for performing effective code reviews that catch issues, improve code quality, and maintain a positive team dynamic.

Review Mindset

Bias Towards Shipping

The primary goal is to get working code merged, not to achieve perfection.

Before requesting changes, always ask yourself:

"Is this feedback worth an additional review cycle?"

Most feedback should be:

• •Approved with suggestions for the author to consider
• •Comments for future reference, not blocking
• •Follow-up issues for later improvement

Reserve "Request Changes" for actual problems:

• •Security vulnerabilities
• •Bugs that will cause runtime errors
• •Breaking changes to existing functionality
• •Missing critical requirements

Do NOT block for:

• •Style preferences (unless egregiously inconsistent)
• •"I would have done it differently"
• •Minor optimizations that don't matter at current scale
• •Missing tests for edge cases that are unlikely
• •Refactoring opportunities (create a follow-up issue instead)

Goals of Code Review

1. •Catch real bugs before they reach production
2. •Prevent security issues - the things that actually matter
3. •Share knowledge across the team
4. •Ship working software - don't let perfect be the enemy of good

The Right Attitude

• •You're reviewing the code, not the person
• •Assume good intent - the author tried their best
• •Be a collaborator, not a gatekeeper
• •Your job is to help ship good code, not to find fault
• •Velocity matters - every review cycle has a cost

Review Process

1. Understand the Context First

Before looking at code:

1. Read the PR description
2. Read the linked issue
3. Understand WHAT is being done and WHY
4. Consider: Is this the right approach?

2. First Pass: High-Level Review

Ask yourself:

• •Does this solve the problem described in the issue?
• •Is the approach reasonable?
• •Are there obvious architectural concerns?
• •Is anything missing?

3. Second Pass: Detailed Review

Look at each file for:

• •Correctness (does it work?)
• •Edge cases and error handling
• •Security implications
• •Performance concerns
• •Test coverage
• •Code style and readability

4. Summarize Your Review

End with an overall assessment:

**[Architect]** Overall this looks good. Clean implementation of the user registration flow.

A few suggestions:
1. Consider adding rate limiting (security)
2. The validation error messages could be more user-friendly
3. Minor: prefer `const` over `let` where possible

Approving with minor suggestions - none are blocking.

What to Look For

Correctness

• •Does the code do what it's supposed to do?
• •Are there logic errors?
• •Are edge cases handled?
• •What happens with null/undefined/empty inputs?

**Concern:** This will throw if `user` is null:
\`\`\`javascript
const name = user.profile.name;
\`\`\`

Consider:
\`\`\`javascript
const name = user?.profile?.name ?? 'Unknown';
\`\`\`

Error Handling

• •Are errors caught appropriately?
• •Are error messages helpful?
• •Does the error handling match the project patterns?

**Suggestion:** The error is swallowed here - the user won't know what went wrong:
\`\`\`javascript
try {
  await saveUser(data);
} catch (e) {
  console.log(e);
}
\`\`\`

Consider surfacing this to the user or rethrowing.

Security (OWASP Top 10)

• •SQL injection (parameterized queries?)
• •XSS (output encoding?)
• •Authentication/authorization checks
• •Sensitive data exposure
• •Input validation

**Security Issue:** User input is interpolated directly into the query:
\`\`\`javascript
const query = `SELECT * FROM users WHERE email = '${email}'`;
\`\`\`

Use parameterized queries to prevent SQL injection.

Performance

• •Unnecessary loops or iterations
• •N+1 query problems
• •Missing indexes
• •Large payloads
• •Memory leaks

**Performance:** This creates an N+1 query problem - one query per user:
\`\`\`javascript
for (const user of users) {
  user.posts = await getPosts(user.id);
}
\`\`\`

Consider using a batch query or join.

Maintainability

• •Is the code readable?
• •Are names descriptive?
• •Is the logic easy to follow?
• •Will this be easy to modify later?

**Readability:** This function does a lot - consider splitting:
- Validation logic → `validateUserInput()`
- Transformation → `transformUserData()`
- Persistence → `saveUser()`

This would make each piece easier to test and understand.

Tests

• •Are tests included?
• •Do tests cover the main paths?
• •Do tests cover edge cases?
• •Are tests readable and maintainable?

**Testing:** Good coverage of the happy path! Consider adding tests for:
- Invalid email format
- Duplicate email (already registered)
- Missing required fields

Consistency

• •Does it follow project conventions?
• •Does it match existing patterns?
• •Is it consistent with itself?

**Consistency:** The rest of the codebase uses `async/await`, but this uses `.then()`:
\`\`\`javascript
fetchUser(id).then(user => { ... });
\`\`\`

Consider using async/await for consistency.

How to Give Feedback

Be Specific and Actionable

Bad:

This code is confusing.

Good:

The nested callbacks make this hard to follow. Consider using async/await or extracting the inner logic into named functions.

Explain the Why

Bad:

Use const here.

Good:

Use const here since user is never reassigned. This signals intent to readers and catches accidental reassignment.

Offer Solutions

Bad:

This won't scale.

Good:

This loads all users into memory, which won't scale. Consider:

1. •Pagination with limit/offset
2. •Streaming with cursor-based pagination
3. •If the use case allows, a count query instead

Distinguish Severity

Use prefixes to indicate importance:

Important: Most comments should be Suggestions or Nits, not Blockers. If you find yourself writing many Blockers, reconsider whether they truly block shipping.

**Blocker:** This SQL query is vulnerable to injection. (Security - must fix)

**Suggestion:** Consider extracting this into a helper function for reuse. (Not blocking - can be done later)

**Nit:** Extra blank line here. (Definitely not blocking)

**Question:** Why did you choose this approach over X? (Curious, not blocking)

**Praise:** Really clean error handling here! (Always welcome)

Ask Questions Instead of Demanding

Demanding:

Change this to use a Map instead of an object.

Collaborative:

Have you considered using a Map here? It might give better performance for frequent lookups. What do you think?

Common Review Scenarios

When You'd Do It Differently

Don't block for preference. Ask yourself:

• •Is their way wrong, or just different?
• •Does it work correctly?
• •Is it maintainable?

If it's just different:

**Note:** I might have used X approach here, but this works well too. Not blocking.

When Something Is Missing

**Missing:** I don't see error handling for the case where the API returns 404. What should happen?

When You Don't Understand

**Question:** I'm not following the logic in this section. Could you add a comment explaining the business rule, or walk me through it?

When Praising Good Work

Don't just point out problems:

**Praise:** Nice job extracting this into a reusable hook! This will help with the other forms too.

**Praise:** The test coverage here is excellent.

Review Response Templates

Default to Approve. Most reviews should approve, possibly with suggestions.

Approve (Most Common)

**[Architect]** LGTM! Clean implementation, good test coverage.

Minor suggestions (not blocking):
- Line 42: prefer const
- Consider adding a comment explaining the retry logic

Merging as-is is fine. Address these if you agree, or not - your call.

Approve with Suggestions

**[Architect]** Approving - this is solid work.

A few things to consider (can address now or in follow-up):
1. The validation could be more specific about what's wrong
2. Consider adding logging for debugging

Not blocking merge. Ship it!

Request Changes (Rare - Use Sparingly)

Only use for genuine blockers: security issues, bugs, or missing critical functionality.

**[Architect]** Good progress! Found one issue that needs fixing before merge:

**Blocker:**
1. SQL injection vulnerability in the search query - this is a security risk

Everything else looks good. Happy to re-review once the security fix is in.

Ask yourself before requesting changes:

• •Will this cause a production incident if shipped?
• •Is this a security vulnerability?
• •Does it break existing functionality?

If the answer to all three is "no", consider approving with suggestions instead.

Decline (N/A)

**[UI/UX]** N/A - No UI changes in this PR.

Reviewed: Backend/infrastructure changes only, no user-facing impact.

Review Etiquette

Do

• •Review promptly (within 24 hours ideally)
• •Be thorough but not pedantic
• •Acknowledge good work
• •Offer to discuss complex issues in person/call
• •Re-review quickly after changes

Don't

• •Nitpick excessively
• •Bike-shed on minor style issues
• •Block for preferences
• •Be condescending
• •Leave reviews hanging

Self-Review Checklist

Before requesting review, authors should self-review:

• • I've re-read my own diff
• • Tests pass locally
• • No console.logs or debug code left
• • No commented-out code
• • Variable names are descriptive
• • Complex logic has comments
• • Error cases are handled
• • No obvious security issues