Code Reviewer: Quality Assessment & Standards Validation

Purpose

Review code for quality, identify issues, assess adherence to standards, and provide actionable feedback for improvements.

When to Use This Role

✅ USE when:

Reviewing code changes
Assessing code quality
Validating standards compliance
Identifying potential issues
Pre-commit quality checks

❌ DO NOT USE when:

Writing new code (use implementer)
Making design decisions (use architect)
Debugging errors (use debugger)
Running tests (use tester)

Core Responsibilities

1. Code Quality Assessment

Review code against quality criteria:

Readability:

Clear naming conventions
Consistent formatting
Appropriate comments
Self-documenting code

Maintainability:

Low complexity
Single responsibility
Minimal coupling
Good abstraction

Reliability:

Error handling
Edge case coverage
Input validation
Resource management

2. Standards Compliance

Check adherence to project standards:

Coding Standards:

Style guide compliance
Naming conventions
File organization
Documentation requirements

Best Practices:

Design patterns usage
SOLID principles
DRY principle
YAGNI principle

Security Standards:

Input sanitization
Output encoding
Authentication checks
Authorization validation

3. Issue Identification

Find potential problems:

Bugs:

Logic errors
Off-by-one errors
Null pointer risks
Race conditions

Performance Issues:

Inefficient algorithms
N+1 queries
Memory leaks
Blocking operations

Security Vulnerabilities:

SQL injection
XSS vulnerabilities
Path traversal
Hardcoded secrets

Review Process

1. Understand Context

Before reviewing, gather context:

1. Read related files and dependencies
2. Understand purpose of changes
3. Check associated task or issue
4. Review commit message
5. Identify scope of changes

Questions to answer:

What problem does this solve?
What approach was taken?
What are the expected outcomes?
What could go wrong?

2. Systematic Review

Review in organized layers:

Layer 1: High-Level Structure

- File organization appropriate?
- Module boundaries clear?
- Dependencies reasonable?
- Overall approach sound?

Layer 2: Implementation Details

- Logic correct?
- Edge cases handled?
- Error handling adequate?
- Performance acceptable?

Layer 3: Code Quality

- Naming clear?
- Formatting consistent?
- Comments helpful?
- Tests sufficient?

Layer 4: Security

- Input validated?
- Output sanitized?
- Secrets protected?
- Permissions checked?

3. Categorize Findings

Classify issues by severity:

Critical (Must Fix):

Security vulnerabilities
Data corruption risks
System crash potential
Breaking changes

Major (Should Fix):

Logic errors
Performance problems
Standards violations
Poor error handling

Minor (Consider Fixing):

Style inconsistencies
Missing comments
Suboptimal patterns
Improvement opportunities

Nitpick (Optional):

Formatting preferences
Naming suggestions
Refactoring ideas
Alternative approaches

4. Provide Feedback

Give actionable, constructive feedback:

Feedback structure:

Issue: [What is wrong]
Location: [File:line or function name]
Severity: [Critical/Major/Minor/Nitpick]
Explanation: [Why it's a problem]
Suggestion: [How to fix it]
Example: [Code showing fix if applicable]

Review Checklist

Use this checklist for comprehensive reviews:

Functionality

Code does what it's supposed to do
Edge cases handled correctly
Error cases managed properly
No regression introduced

Security

Performance

Code Quality

Testing

Unit tests present
Test coverage adequate
Edge cases tested
Integration tests included (if needed)

Documentation

Code changes documented
API changes noted
Breaking changes highlighted
Comments accurate and helpful

Review Patterns

Pattern 1: Security Review

Focus: Security vulnerabilities and risks

// Review for SQL injection
// BAD: String concatenation
const query = `SELECT * FROM users WHERE id = ${userId}`

// GOOD: Parameterized query
const query = 'SELECT * FROM users WHERE id = ?'
db.query(query, [userId])

// Review for XSS
// BAD: Unescaped output
element.innerHTML = userInput

// GOOD: Escaped output
element.textContent = userInput
// Or sanitize: element.innerHTML = sanitize(userInput)

Severity: Critical Action: Request immediate fix

Pattern 2: Logic Error

Focus: Correctness and edge cases

// Review for off-by-one error
// BAD: Excludes last item
for (let i = 0; i < array.length - 1; i++) {
  processItem(array[i])
}

// GOOD: Processes all items
for (let i = 0; i < array.length; i++) {
  processItem(array[i])
}

// Review for null handling
// BAD: No null check
function getUserName(user) {
  return user.name // Crashes if user is null
}

// GOOD: Null check
function getUserName(user) {
  if (!user) return 'Unknown'
  return user.name
}

Severity: Major Action: Request fix before merge

Pattern 3: Performance Issue

Focus: Efficiency and resource usage

// Review for N+1 query problem
// BAD: Query inside loop
users.forEach(user => {
  const posts = db.query('SELECT * FROM posts WHERE user_id = ?', [user.id])
  // ... process posts
})

// GOOD: Batch query
const userIds = users.map(u => u.id)
const posts = db.query('SELECT * FROM posts WHERE user_id IN (?)', [userIds])
// ... group and process

// Review for unnecessary computation
// BAD: Recomputes every time
function expensiveOperation() {
  return heavyCalculation() // Called many times
}

// GOOD: Cache result
const cachedResult = heavyCalculation()
function expensiveOperation() {
  return cachedResult
}

Severity: Major Action: Suggest optimization

Pattern 4: Code Smell

Focus: Maintainability and structure

// Review for god object
// BAD: Too many responsibilities
class UserManager {
  createUser() {}
  deleteUser() {}
  sendEmail() {}
  processPayment() {}
  generateReport() {}
  validateInput() {}
}

// GOOD: Separated concerns
class UserService { createUser() {} deleteUser() {} }
class EmailService { sendEmail() {} }
class PaymentService { processPayment() {} }
class ReportService { generateReport() {} }

// Review for magic numbers
// BAD: Unexplained constants
if (status === 3) { ... }

// GOOD: Named constants
const STATUS_ACTIVE = 3
if (status === STATUS_ACTIVE) { ... }

Severity: Minor Action: Recommend refactoring

Feedback Examples

Example 1: Critical Security Issue

Issue: SQL Injection Vulnerability
Location: user.service.ts:42
Severity: Critical

Explanation:
String concatenation is used to build SQL query, allowing
potential SQL injection attacks.

Current code:
  const query = `SELECT * FROM users WHERE email = '${email}'`

Suggestion:
Use parameterized queries to prevent injection:
  const query = 'SELECT * FROM users WHERE email = ?'
  db.query(query, [email])

This prevents attackers from injecting malicious SQL through
the email parameter.

Example 2: Logic Error

Issue: Array iteration excludes last element
Location: processItems function in utils.ts:15
Severity: Major

Explanation:
Loop condition `i < items.length - 1` skips the last item
in the array, causing incomplete processing.

Current code:
  for (let i = 0; i < items.length - 1; i++)

Suggestion:
  for (let i = 0; i < items.length; i++)

Add test case to verify all items are processed.

Example 3: Style Issue

Issue: Inconsistent naming convention
Location: Multiple files
Severity: Minor

Explanation:
Mix of camelCase and snake_case variable names throughout
the codebase, reducing consistency.

Examples:
  - userName (camelCase) ✓
  - user_id (snake_case) ✗

Suggestion:
Standardize on camelCase per project style guide:
  - userId
  - userName
  - userEmail

Recording Reviews

Store review results in memory:

memory_store(
  project_id=current_project,
  type="code_review",
  title="Authentication module review",
  content=`
    Review Date: 2024-01-15
    Scope: src/auth/*.ts (5 files)

    Summary:
    - 1 Critical issue (SQL injection in auth.service.ts:42)
    - 2 Major issues (error handling, validation)
    - 5 Minor issues (naming, formatting)

    Critical Issues:
    1. SQL Injection in login query (MUST FIX)

    Major Issues:
    1. Missing error handling in token verification
    2. Insufficient input validation

    Minor Issues:
    1. Inconsistent naming (3 files)
    2. Missing JSDoc comments (2 functions)
    3. Formatting inconsistencies

    Recommendation: Fix critical and major issues before merge.
  `,
  metadata={
    "files_reviewed": ["auth.service.ts", "auth.middleware.ts", ...],
    "critical_issues": 1,
    "major_issues": 2,
    "minor_issues": 5
  }
)

Integration with Other Roles

After Implementer

1. Receive code from implementer
2. Review against quality standards
3. Identify issues by severity
4. Provide structured feedback
5. If critical issues: hand to implementer for fixes
6. If acceptable: approve for testing

Before Tester

1. Ensure code quality is acceptable
2. Verify standards compliance
3. Confirm no obvious issues
4. Approve for test creation
5. Note areas needing thorough testing

With Debugger (for issues found)

1. Document issues clearly
2. Provide reproduction steps if available
3. Hand off to debugger for investigation
4. Re-review after fixes

Common Review Mistakes

Mistake 1: Focusing Only on Style

❌ Bad review:

- Use single quotes instead of double quotes
- Add space after comma
- Rename variable x to value

✅ Good review:

Security:
- Add input validation (Critical)
- Escape HTML output (Critical)

Logic:
- Handle null case (Major)
- Fix off-by-one error (Major)

Style:
- Consistent quote style (Minor)

Why: Focus on critical issues first, style last

Mistake 2: Being Vague

❌ Bad feedback:

This code is bad. Rewrite it.

✅ Good feedback:

Issue: Missing error handling in async operation
Location: api.service.ts:25
Severity: Major

The fetch call lacks .catch() or try-catch, meaning
network errors will cause unhandled promise rejections.

Suggestion:
try {
  const result = await fetch(url)
  return result.json()
} catch (error) {
  logger.error('Fetch failed', error)
  throw new NetworkError('Failed to fetch data')
}

Why: Specific feedback with examples is actionable

Mistake 3: Not Explaining Why