TUTORIAL 06

Code Review Workflow

@dev submitted PR#42 "feat(tasks): add task filtering by status". You're @lead reviewing it.

⏱ ~15 min · Hands-on

/agile-code-pr-review /agile-code-review /agile-code-merge

Scenario

Your teammate @dev has opened PR#42 with a new task filtering feature. The PR adds a FilterService, updates TaskController, and includes new tests. You're @lead — it's your job to review before this merges to develop.

Step 1: Read the PR Diff

Before running any commands, read the diff and understand what changed. Don't jump to nitpicking — understand intent first.

PR#42 — feat(tasks): add task filtering by status

Files changed: 4 files, +187 lines, -12 lines

src/services/FilterService.java — NEW (92 lines)
src/controllers/TaskController.java — modified (+14 lines)
test/services/FilterServiceTest.java — NEW (68 lines)
test/controllers/TaskControllerTest.java — modified (+13 lines)

Step 2: Run Structured Review

Use /agile-code-pr-review for a systematic, severity-classified review.

/agile-code-pr-review

PR Review — #42 feat(tasks): add task filtering by status
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

🔴 S0 — BLOCKS MERGE
────────────────────
[1] SQL Injection in FilterService.java:34
    String query = "SELECT * FROM tasks WHERE status = '" + status + "'";
    → User input concatenated directly into SQL. Must use parameterized query.

🟠 S1 — MUST FIX
────────────────────
[2] SRP Violation in FilterService.java
    FilterService handles both filtering AND sorting (methods: filterByStatus,
    filterByPriority, sortByDate, sortByPriority). These are two responsibilities.
    → Split into FilterService and SortService.

🟡 S2 — SHOULD FIX
────────────────────
[3] Poor naming in TaskController.java:67
    List<Task> data = filterService.filterByStatus(status);
    → 'data' is generic. Use 'filteredTasks' for clarity.

🔵 S3 — NIT
────────────────────
[4] Import order in FilterService.java:1-8
    Imports are not grouped (java.util mixed with org.springframework).
    → Follow project convention: java.* → javax.* → org.* → com.*

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Verdict: ❌ CHANGES REQUESTED (1 blocker, 1 must-fix)

Step 3: Understand the Severity System

Severity	Label	Action	Blocks Merge?
🔴 S0	Critical	Security vulnerability, data loss, crash	YES
🟠 S1	Major	Design flaw, SRP violation, missing error handling	YES
🟡 S2	Minor	Naming, readability, minor improvements	No (should fix)
🔵 S3	Nit	Style, formatting, personal preference	No (optional)

Rule: Any 🔴 S0 or 🟠 S1 finding blocks the merge. 🟡 S2 should be fixed but doesn't block. 🔵 S3 is at the author's discretion.

Step 4: Author Addresses Findings

@dev pushes fixes for each finding:

🔴 S0 Fix: Parameterized Query

❌ Before (SQL Injection)

String query = "SELECT * FROM tasks WHERE status = '" + status + "'";

✅ After (Parameterized)

String query = "SELECT * FROM tasks WHERE status = ?";
PreparedStatement stmt = conn.prepareStatement(query);
stmt.setString(1, status);

🟠 S1 Fix: Split Responsibilities

@dev extracts sorting logic into a new SortService:

FilterService — filterByStatus(), filterByPriority()
SortService (new) — sortByDate(), sortByPriority()

🟡 S2 Fix: Rename Variable

// Before
List<Task> data = filterService.filterByStatus(status);

// After
List<Task> filteredTasks = filterService.filterByStatus(status);

🔵 S3: Acknowledged

@dev reorders imports to follow project convention. Quick fix, no debate needed.

Step 5: Re-Review

@dev pushes the fixes and requests re-review. Run /agile-code-pr-review again.

/agile-code-pr-review

PR Re-Review — #42 feat(tasks): add task filtering by status
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Previous Findings:
  🔴 S0 [1] SQL Injection .............. ✅ RESOLVED (parameterized query)
  🟠 S1 [2] SRP Violation .............. ✅ RESOLVED (SortService extracted)
  🟡 S2 [3] Poor naming ................ ✅ RESOLVED (renamed to filteredTasks)
  🔵 S3 [4] Import order ............... ✅ RESOLVED (reordered)

New Findings: (none)

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Verdict: ✅ APPROVED — merge to develop

Step 6: Merge with /agile-code-merge

With the PR approved, merge the feature branch using the agile command:

/agile-code-merge

🔀 Merge — PR #42
═══════════════════

Pre-merge checks:
  ✅ PR approved by @lead
  ✅ CI passing on branch
  ✅ No merge conflicts
  ✅ Branch up to date with develop

Merging: feature/task-filtering → develop
Strategy: merge --no-ff (project convention)

Merge complete.
Branch feature/task-filtering deleted (was 7b2e4d1).

✅ PR #42 merged to develop.

✅ PR#42 Merged

The task filtering feature lands on develop with a critical SQL injection caught before it reached users. /agile-code-merge verified all pre-merge conditions, applied the project's merge strategy, and cleaned up the branch automatically. This is why code review exists.

Best Practices: As a Reviewer

📖 Understand first, critique second. Read the PR description and related ticket before looking at code.
❓ Ask "why" before saying "change this". The author may have context you don't. Ask: "What was the reasoning for X?"
💬 Suggest, don't command. "Consider using X" is better than "Use X." Reviews are collaborative, not authoritarian.
⏰ Review within 24 hours. Stale PRs block the team. If you can't review in time, say so and hand off.
🎯 Use severity levels. Not everything is a blocker. Classify your feedback so the author knows what's critical vs. nice-to-have.
👍 Call out good code too. Positive feedback reinforces good patterns across the team.

Best Practices: As an Author

🔍 Self-review first. Read your own diff before requesting review. You'll catch 30% of issues yourself.
📦 Keep PRs small. Under 400 lines changed. Large PRs get rubber-stamped, not reviewed.
📝 Write a descriptive PR body. Explain what changed, why, and how to test it. Don't make reviewers guess.
🧪 Include tests. A PR without tests is a PR that will need a follow-up bugfix.
🙏 Don't take feedback personally. Reviews are about the code, not you. Every comment is a learning opportunity.

What You Practiced

👀 Reading a PR diff to understand scope before diving into details
🔍 /agile-code-pr-review for structured, severity-classified review
🔴 Identifying critical issues (SQL injection) that block merges
🟠 Catching design flaws (SRP violation) that must be fixed
🟡 Flagging improvements (naming) that should be fixed
🔵 Noting nits (formatting) without blocking progress
🔄 Re-reviewing after fixes to verify all findings are resolved
🔀 /agile-code-merge to merge the approved PR after all blockers are resolved

Knowledge Check

Which severity levels block a PR merge?

← Emergency Hotfix Refactor Technical Debt →