# Code Review — Giving and Receiving Feedback

<!-- hint:slides topic="Code review: review checklist (correctness, readability, security), giving constructive feedback, and PR lifecycle" slides="5" -->

## Why Code Reviews Matter

Code review is a process where someone other than the author examines code before it merges. It catches bugs, improves design, spreads knowledge, and upholds team standards. Reviews are about the code, not the person.

## What to Look For

### Correctness
Does the code do what it claims? Are edge cases handled? Could it fail in production?

### Readability
- **Naming:** Clear, consistent variable and function names
- **Comments:** Explain *why*, not *what*
- **Structure:** Logic flows naturally; no magic numbers or deep nesting

### Performance
- Unnecessary loops or allocations?
- O(n²) when O(n) is possible?
- Blocking calls in hot paths?

### Security
- Input validation and sanitization
- No hardcoded secrets or credentials
- Sensitive data handling

```javascript
// ❌ Security risk: SQL injection
const query = `SELECT * FROM users WHERE id = ${userId}`;

// ✅ Parameterized query
const query = 'SELECT * FROM users WHERE id = ?';
db.execute(query, [userId]);
```

## Writing Good PR Descriptions

A good description explains **what**, **why**, and **how**:

```markdown
## What
Add user profile export as JSON.

## Why
Product needs GDPR "data portability" compliance by Q2.

## How
- New `/api/me/export` endpoint
- Uses existing serialization layer
- Rate-limited to 1 req/min per user
```

Include testing notes, screenshots for UI changes, and migration steps when relevant.

## Giving Feedback

### Be Specific
❌ "This is wrong."  
✅ "This loop runs O(n²) when the input can be large; consider a Map for O(n) lookups."

### Explain Why
❌ "Use `const` here."  
✅ "Using `const` here prevents accidental reassignment and signals immutability to readers."

### Suggest Alternatives
❌ "Refactor this."  
✅ "This function is doing three things. Consider extracting validation and formatting into separate helpers."

### Separate Nitpicks
Mark minor preferences as "nit:" so authors can address them without blocking merge.

## Receiving Feedback

### Don't Take It Personally
Feedback targets the code, not you. Even senior engineers get thorough reviews.

### Ask Questions
If a suggestion is unclear, ask: "Can you elaborate on why X would be better here?"

### Disagree Thoughtfully
It's OK to push back. Explain your reasoning: "I considered X, but chose Y because..."

## Review Checklists

**Before submitting:**
- [ ] Tests pass locally
- [ ] PR description is complete
- [ ] No debug logs or commented code
- [ ] Changes are scoped to the stated goal

**When reviewing:**
- [ ] Understand the goal before reading code
- [ ] Check tests cover the new behavior
- [ ] Verify no regressions introduced
- [ ] Approve or request changes promptly

## Common Anti-Patterns

| Anti-Pattern | Problem |
|--------------|---------|
| "LGTM" without reading | Rubber-stamping; defeats the purpose |
| Nitpicking style only | Misses design and correctness |
| Blocking on opinions | Distinguish "must fix" vs "nice to have" |
| Delayed feedback | Blocks the author's flow |
| Dismissive tone | "Obviously wrong" → demoralizes |

## PR Lifecycle

```mermaid
flowchart TD
    A[Author writes code] --> B[Author creates PR]
    B --> C[Reviewer(s) assigned]
    C --> D{Review passes?}
    D -->|No| E[Author addresses feedback]
    E --> C
    D -->|Yes| F[Approve & merge]
    F --> G[Post-merge: monitor, document]
```

---

## Key Takeaways

1. **Correctness first** — then readability, performance, security
2. **Specific, kind feedback** — explain why, suggest alternatives
3. **Separate blocking vs nitpicks** — unblock quickly when possible
4. **PR descriptions matter** — context speeds reviews
5. **Reviews are for the code** — keep feedback professional and constructive