# Code Review

> Code review checklist, comment guidelines, and PR review best practices. Covers PR review workflows, code suggestions, nitpick vs blocking review classification, LGTM criteria, review checklist (correctness, security, performance, readability), and constructive feedback patterns. Use when reviewing pull requests, providing code suggestions, or establishing a team code review process and review comment conventions.

- Skill: `iceflower/code-review` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx skillmds@latest add iceflower/code-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/iceflower/code-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- License: MIT
- Author: iceflower (https://skillmd.com/u/iceflower)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/iceflower/code-review

---

# Code Review Rules

## 1. Review Checklist

### Correctness

- Does the code do what it claims to do?
- Are edge cases handled (null, empty, boundary values)?
- Are error paths handled properly?
- Is the logic correct for concurrent/async scenarios?

### Security

- Is user input validated and sanitized?
- Are sensitive data (passwords, tokens) excluded from logs and responses?
- Are authorization checks in place?
- Are SQL queries parameterized (no string concatenation)?

### Performance

- Are there N+1 query problems?
- Are large collections paginated?
- Are expensive operations cached where appropriate?
- Are database indexes considered for new query patterns?

### Readability

- Are names descriptive and consistent with existing conventions?
- Is the code self-explanatory without excessive comments?
- Is nesting depth reasonable (3 levels max)?
- Are magic numbers replaced with named constants?

### Testability

- Is the change covered by tests?
- Are tests testing behavior, not implementation?
- Are new edge cases included in tests?
- Can dependencies be easily replaced in tests?

---

## 2. Review Priorities

### Must Fix (block merge)

- Bugs or incorrect logic
- Security vulnerabilities
- Data loss risk
- Breaking API contract changes without versioning
- Missing error handling for critical paths

### Should Fix (strongly recommend)

- N+1 queries or obvious performance issues
- Missing input validation
- Duplicate code that should be extracted
- Missing tests for complex logic

### Nice to Have (optional, suggest)

- Minor naming improvements
- Code style preferences
- Additional documentation
- Alternative implementation approaches

---

## 3. Review Comment Style

### Good Comments

```text
// Specific, actionable, with context
"This query will cause N+1 when users have orders.
Consider using @EntityGraph or JOIN FETCH."

// Suggest, don't demand
"Consider using `sealed class` here — it would make
the when-expression exhaustive and catch missing cases at compile time."

// Ask questions to understand intent
"Is this intentional? If the token is expired, this returns null
instead of throwing — which means the caller needs to handle null."
```

### Bad Comments

```text
// Vague
"This doesn't look right."

// Nitpicking without value
"Add a blank line here."

// Prescriptive without rationale
"Use a different pattern."
```

---

## 4. Review Scope

### What to Review

- Business logic changes
- API contract changes (request/response, status codes)
- Database schema changes (migrations)
- Security-related code (auth, validation, data handling)
- Configuration changes (application.yml, build files)

### What to Skip

- Auto-generated code (unless the generator config changed)
- Dependency lock files (verify dependency changes only)
- IDE configuration files
- Formatting-only changes (should be handled by linter)

---

## 5. Self-Review Before Requesting Review

### Pre-PR Checklist

- [ ] Diff reviewed — no debug code, no commented-out code
- [ ] Tests pass locally
- [ ] No unintended file changes
- [ ] Commit messages follow convention
- [ ] PR description explains what and why
- [ ] Breaking changes documented

---

## 6. Anti-Patterns

- **Rubber Stamping**: Approving code without thorough review. Always read and understand all changes before approving
- **Nitpicking**: Over-focusing on minor style issues while missing core logic flaws. Delegate linter-fixable issues to automation
- **Delayed Reviews**: Leaving PRs unreviewed causes context loss and merge conflicts. Review within 24 hours
- **Oversized PRs**: Too many changes in one PR degrades review quality. Keep PRs under 400 lines
- **Emotional Feedback**: Use constructive suggestions like "Changing X to Y would improve Z" instead of accusatory questions

## 7. Related Skills

- `code-quality`: Code quality principles and refactoring techniques
- `git-workflow`: Git commit conventions and branch strategies
- `testing`: Test code review perspectives
- `security`: Security vulnerability review checklist