Code Review

Code Review Skill

Provide thorough, constructive code reviews that ensure quality, security, and alignment with requirements.

Review Process Overview

Every code review follows this structured approach:

1. Story/Task Alignment - Verify implementation matches requirements
2. Code Quality - Check naming, structure, and clean code principles
3. Security Analysis - Identify vulnerabilities using OWASP guidelines
4. Maintainability - Assess readability, complexity, and long-term sustainability
5. Testing & Coverage - Evaluate test quality and coverage
6. Performance - Identify potential performance issues
7. Architecture Fit - Ensure consistency with existing patterns
8. Constructive Feedback - Provide actionable improvements with reasoning

Pre-Review Checklist

Before starting a code review, gather:

• PR/commit description and linked issues/stories
• Code changes (diff or full files)
• Related test files
• Project context (tech stack, architecture patterns)
• Existing linting/style configurations

1. Story/Task Alignment

What to Check

Requirements Coverage:

• Does the code implement all acceptance criteria from the story?
• Are edge cases mentioned in the story handled?
• Does the implementation match the story's scope (not over/under-engineered)?

Story References:

• Is the story/issue referenced in commit messages?
• Does the PR description explain what's being solved?
• Are there any implemented features NOT in the story (scope creep)?

Questions to Ask

• "What was the original requirement?"
• "Does this code solve that requirement completely?"
• "Are there parts of the story not addressed?"
• "Is there functionality beyond the story scope?"

Feedback Template

✅ Story Alignment: [Pass/Partial/Fail]

Requirement Coverage:
- ✅ Implements user authentication (Story #123)
- ⚠️ Missing password reset flow mentioned in AC3
- ❌ Added email notification feature not in story scope

Recommendation: [Specific action needed]

2. Code Quality

Naming Conventions

Variables & Functions:

• Descriptive, intention-revealing names
• Consistent with project conventions (camelCase, snake_case, etc.)
• Appropriate length (not too short, not unnecessarily long)
• Avoid abbreviations unless standard (e.g., id, url)

Classes & Components:

• Clear, singular nouns for classes
• Verb phrases for functions (e.g., getUserById, calculateTotal)
• Boolean variables start with is, has, should (e.g., isActive, hasPermission)

Constants:

• ALL_CAPS for true constants
• Descriptive names for magic numbers/strings

Examples:

❌ Bad:

function calc(u, p) {
  const x = u * p;
  return x;
}
const MAX = 10; // Max what?

✅ Good:

function calculateOrderTotal(unitPrice, quantity) {
  const totalPrice = unitPrice * quantity;
  return totalPrice;
}
const MAX_RETRY_ATTEMPTS = 10;

Clean Code Principles

Single Responsibility:

• Each function/class does ONE thing well
• Easy to name (if hard to name, probably doing too much)

DRY (Don't Repeat Yourself):

• No code duplication
• Extract common logic into reusable functions
• Use abstractions appropriately

Function Size:

• Keep functions small (< 20 lines ideally)
• One level of abstraction per function
• Extract complex logic into named helper functions

Readability:

• Code reads like prose
• Self-documenting (minimal comments needed)
• Clear flow and structure
• Appropriate whitespace and formatting

Complexity:

• Avoid deep nesting (> 3 levels)
• Limit function parameters (< 4 ideally)
• Cyclomatic complexity under control
• Extract complex conditions into named variables/functions

Code Smells to Flag

Common issues to identify:

• Long functions (> 50 lines)
• God objects (classes doing too much)
• Deep nesting (if/else > 3 levels)
• Magic numbers (unexplained constants)
• Dead code (commented out, unreachable)
• Inconsistent style (mixed conventions)
• Inappropriate intimacy (too much coupling)
• Feature envy (method uses another class's data heavily)

Feedback Template

📝 Code Quality: [Good/Needs Improvement]

Naming:
- ✅ Functions well-named and descriptive
- ⚠️ Variable 'x' on line 45 unclear - suggest 'totalRevenue'

Structure:
- ⚠️ Function `processUser` (lines 120-180) is 60 lines long
  → Recommend: Extract validation logic into `validateUserData`
  → Recommend: Extract DB operations into `saveUserToDatabase`
  
Clean Code:
- ❌ Code duplication in lines 45-52 and 78-85
  → Recommend: Extract into `formatCurrency(amount)` helper

Complexity:
- ⚠️ Deep nesting in `calculateDiscount` (4 levels)
  → Recommend: Use early returns or guard clauses

Why: These changes will make the code easier to understand, test, and maintain.

3. Security Analysis (OWASP)

Review code against OWASP Top 10 and common vulnerabilities. Reference references/owasp-checklist.md for comprehensive security checks.

Key Security Checks

A01 - Broken Access Control:

• Are authorization checks present and correct?
• Can users access resources they shouldn't?
• Are direct object references protected?
• Is there proper role-based access control?

A02 - Cryptographic Failures:

• Is sensitive data encrypted in transit (HTTPS)?
• Is sensitive data encrypted at rest?
• Are proper hashing algorithms used (bcrypt, argon2)?
• Are cryptographic keys managed securely?
• No hardcoded secrets/passwords?

A03 - Injection:

• SQL injection prevention (parameterized queries)?
• XSS prevention (output encoding, CSP)?
• Command injection prevention?
• LDAP/XML injection prevention?

A04 - Insecure Design:

• Threat modeling considered?
• Secure by default configuration?
• Proper input validation at boundaries?
• Business logic flaws addressed?

A05 - Security Misconfiguration:

• Default credentials changed?
• Error messages don't leak sensitive info?
• Security headers configured (CSP, X-Frame-Options)?
• Unnecessary features disabled?

A06 - Vulnerable Components:

• Dependencies up-to-date?
• Known vulnerabilities in dependencies?
• Using maintained libraries?
• Supply chain security considered?

A07 - Authentication Failures:

• Strong password requirements?
• Multi-factor authentication supported?
• Session management secure?
• Account lockout on failed attempts?
• No credential stuffing vulnerabilities?

A08 - Data Integrity Failures:

• Unsigned/unverified data not trusted?
• CI/CD pipeline secured?
• Auto-update without verification avoided?
• Insecure deserialization prevented?

A09 - Logging/Monitoring Failures:

• Security events logged?
• Audit trails immutable?
• Alerts configured for suspicious activity?
• Logs don't contain sensitive data?

A10 - Server-Side Request Forgery (SSRF):

• User input validated before making requests?
• Allowlist for URLs?
• Network segmentation in place?
• Metadata endpoints protected?

Additional Security Concerns

Input Validation:

• All user input validated and sanitized?
• Allowlist approach over blocklist?
• Length limits enforced?
• Type checking performed?

Output Encoding:

• All output properly encoded for context?
• XSS prevention in place?
• HTML, JavaScript, URL encoding as needed?

Error Handling:

• Errors caught and handled gracefully?
• No sensitive information in error messages?
• Generic errors shown to users?
• Detailed errors logged securely?

Session Management:

• Secure session tokens (random, sufficient entropy)?
• Tokens invalidated on logout?
• Session timeout configured?
• CSRF tokens implemented?

Feedback Template

🔒 Security Analysis: [Secure/Issues Found]

Critical Issues (Fix Immediately):
- 🚨 SQL Injection vulnerability in `getUserByEmail` (line 67)
  → Current: Direct string concatenation in query
  → Fix: Use parameterized query: `SELECT * FROM users WHERE email = ?`
  → Why: Attackers can inject malicious SQL and access all database data

High Priority:
- ⚠️ Password stored in plaintext (line 134)
  → Fix: Use bcrypt.hash() before storing
  → Why: If database is breached, all passwords are compromised

Medium Priority:
- ⚠️ Missing rate limiting on login endpoint
  → Recommend: Implement rate limiting to prevent brute force attacks
  → Why: Prevents credential stuffing and account takeover attempts

Good Practices Found:
- ✅ HTTPS enforced for all endpoints
- ✅ Input validation on user registration
- ✅ CSRF tokens implemented

OWASP Alignment: [List specific OWASP categories addressed/violated]

4. Maintainability & Readability

Readability Factors

Is the code EASY TO READ?

• Can someone unfamiliar understand the code flow in 5 minutes?
• Are variable/function names self-explanatory?
• Is the logic flow intuitive?
• Are complex operations broken down?
• Is indentation and formatting consistent?

Mental Load:

• How much context must reader hold in mind?
• Can functions be understood in isolation?
• Are abstractions appropriate (not too complex, not too simple)?

Documentation:

• Are complex algorithms explained?
• Are non-obvious decisions documented?
• Are function purposes clear from names alone?
• Are JSDoc/docstrings provided where needed?

Maintainability Factors

Is the code EASY TO MAINTAIN?

Testability:

• Can this code be unit tested easily?
• Are dependencies injectable?
• Are side effects isolated?
• Can tests verify behavior without testing implementation?

Extensibility:

• Can new features be added without major refactoring?
• Are abstractions at the right level?
• Is the code open for extension, closed for modification?

Debuggability:

• Are error messages helpful?
• Is logging sufficient for troubleshooting?
• Can issues be isolated quickly?
• Are stack traces meaningful?

Changeability:

• What's the blast radius of changes?
• How many files must change for a feature change?
• Are concerns properly separated?
• Is coupling minimized?

Technical Debt Assessment

Identify and categorize technical debt:

Type 1 - Quick Fixes (< 1 hour):

• Rename unclear variables
• Add missing comments
• Extract magic numbers to constants

Type 2 - Refactoring (1-4 hours):

• Extract long functions
• Remove code duplication
• Improve error handling

Type 3 - Restructuring (> 4 hours):

• Redesign module structure
• Refactor architecture
• Add missing abstractions

Feedback Template

🔧 Maintainability: [Excellent/Good/Needs Work]

Readability:
- ✅ Code flows naturally and is easy to follow
- ⚠️ Function `processPayment` (line 89) has high cognitive complexity
  → Multiple nested conditions make logic hard to follow
  → Recommend: Extract validation into separate functions with clear names

Testability:
- ❌ `UserService` directly instantiates `DatabaseConnection`
  → Hard to test without real database
  → Recommend: Inject dependency via constructor
  → Why: Enables unit testing with mocked database

Documentation:
- ⚠️ Complex algorithm in `calculateShippingCost` (line 234) lacks explanation
  → Add comment explaining the business rules
  → Why: Future developers need to understand the logic without reverse-engineering

Technical Debt:
- Type 1: Rename `tmp` to `temporaryUserData` (line 45) - 5 min
- Type 2: Extract duplicate validation logic (lines 67-78, 123-134) - 30 min
  → Impact: Reduces maintenance burden and potential for inconsistencies

Long-term Maintenance Score: 7/10
→ Small improvements would significantly boost maintainability

5. Testing & Coverage

Test Quality Assessment

Test Coverage:

• Are critical paths tested?
• Are edge cases covered?
• Are error conditions tested?
• What's the coverage percentage (if available)?

Test Quality:

• Are tests meaningful (not just for coverage)?
• Do tests verify behavior, not implementation?
• Are tests independent and isolated?
• Are test names descriptive?

Test Types:

• Unit tests for business logic?
• Integration tests for API endpoints?
• End-to-end tests for critical flows?
• Security tests for vulnerabilities?

Missing Tests:

• What scenarios aren't tested?
• Which error paths are untested?
• Are there untested branches?

Feedback Template

🧪 Testing: [Well Tested/Partially Tested/Needs Tests]

Coverage:
- ✅ Happy path tested for user registration
- ⚠️ Missing tests for:
  → Duplicate email registration (should return 409)
  → Invalid email format (should return 400)
  → Database connection failure (should return 500)

Test Quality:
- ✅ Tests are isolated and independent
- ⚠️ Test `test_user_creation` on line 234 tests implementation details
  → Currently: Checks internal method calls
  → Should: Verify user exists in database after creation
  → Why: Implementation can change, behavior shouldn't

Recommended Tests:
1. Edge case: Maximum password length (security)
2. Error case: Network timeout during email sending
3. Integration: Full registration flow with email verification

Priority: Add error case tests (high priority for production readiness)

6. Performance Considerations

Common Performance Issues

Database:

• N+1 query problems?
• Missing indexes?
• Inefficient queries (SELECT *)?
• Unnecessary joins?
• Missing pagination for large datasets?

API Calls:

• Redundant API requests?
• Missing caching?
• Synchronous calls that could be async?
• No timeout handling?

Frontend:

• Unnecessary re-renders (React)?
• Large bundle sizes?
• Unoptimized images?
• Missing code splitting?
• Blocking operations on main thread?

Algorithms:

• Inefficient algorithm choice (O(n²) when O(n log n) possible)?
• Unnecessary iterations?
• Memory leaks?
• Large object allocations in loops?

Feedback Template

⚡ Performance: [Optimized/Acceptable/Issues Found]

Issues:
- ⚠️ N+1 query in `getUserWithPosts` (line 89)
  → Current: Loops through users, queries posts for each (1 + N queries)
  → Fix: Use JOIN or eager loading to fetch in single query
  → Impact: For 100 users, reduces from 101 queries to 1 query
  → Why: Critical for scalability as user base grows

- ⚠️ Synchronous file read in request handler (line 134)
  → Fix: Use async file read or cache file contents
  → Impact: Blocks other requests, reduces throughput
  → Why: Each request should be non-blocking

Optimizations:
- ✅ Proper indexing on frequently queried fields
- ✅ Caching implemented for expensive operations

Recommendations:
- Consider: Add pagination to `getAllUsers` endpoint
  → Why: Will be slow when user count exceeds 1000

7. Architecture Fit

Pattern Consistency

Existing Patterns:

• Does code follow established patterns in codebase?
• Are similar problems solved similarly?
• Is the architecture style respected (MVC, Clean Architecture, etc.)?

Project Structure:

• Are files in correct directories?
• Does naming follow project conventions?
• Are imports organized consistently?

Technology Choices:

• Are the right tools used for the job?
• Are dependencies appropriate?
• Is the tech stack consistent?

Feedback Template

🏗️ Architecture Fit: [Aligned/Minor Issues/Concerns]

Pattern Consistency:
- ✅ Follows existing service layer pattern
- ⚠️ Error handling inconsistent with other controllers
  → Other controllers use try/catch with centralized error handler
  → This controller returns inline error responses
  → Recommend: Use consistent error handling pattern

Structure:
- ❌ Business logic in controller (line 67-89)
  → Should be: Extracted to service layer
  → Why: Controllers should only handle HTTP concerns
  → Example: Move to `UserService.validateAndCreateUser()`

Technology:
- ⚠️ Introduced new date library (date-fns) when moment.js already in use
  → Question: Is there a reason for the new dependency?
  → Recommend: Use existing library for consistency unless justified

Impact: Inconsistencies make codebase harder to navigate and maintain

8. Additional Considerations

Documentation

Code Comments:

• Are complex sections explained?
• Are non-obvious decisions documented?
• Are TODOs marked with ticket references?

API Documentation:

• Are endpoints documented (OpenAPI/Swagger)?
• Are request/response examples provided?
• Are error codes documented?

README/Changelog:

• Is PR description clear and complete?
• Are breaking changes noted?
• Are migration steps provided?

Dependencies

New Dependencies:

• Are new dependencies necessary?
• Are they maintained and secure?
• License compatibility?
• Bundle size impact?

Dependency Updates:

• Breaking changes handled?
• Changelog reviewed?
• Tests updated for new version?

Backward Compatibility

Breaking Changes:

• Are there breaking API changes?
• Is deprecation path provided?
• Are consumers notified?
• Version bump appropriate?

Database Changes:

• Are migrations provided?
• Is rollback possible?
• Is data migration safe?

Constructive Feedback Framework

Feedback Principles

1. Be Specific: ❌ "This code is messy" ✅ "Function processData on line 67 mixes data validation and business logic, making it hard to test each concern independently"

2. Explain Why: Always include the reasoning behind suggestions:

• Impact on maintainability
• Security implications
• Performance benefits
• User experience improvements

3. Provide Solutions: Don't just point out problems - suggest fixes: ❌ "This is inefficient" ✅ "Consider using a Map instead of Array.find() for O(1) lookups instead of O(n)"

4. Acknowledge Good Work: Point out what's done well:

• "Great use of error boundaries here"
• "Well-structured tests with clear names"
• "Excellent security considerations"

5. Prioritize Issues: Use severity levels:

• 🚨 Critical: Security vulnerabilities, data loss risks
• ⚠️ High: Bugs, significant performance issues
• 💡 Medium: Code quality, maintainability
• 📝 Low: Style preferences, minor optimizations

6. Consider Context:

• Time constraints
• Team experience level
• Project maturity
• Technical debt strategy

Feedback Structure

## Code Review Summary

**Overall Assessment:** [Approve/Approve with Changes/Request Changes]

---

### ✅ Strengths
- [What's done well]
- [Good practices observed]
- [Positive aspects]

---

### 🚨 Critical Issues (Must Fix)
1. [Issue] - Line X
   - Current: [What's wrong]
   - Fix: [How to fix]
   - Why: [Impact/reasoning]

---

### ⚠️ Important Issues (Should Fix)
1. [Issue] - Line X
   - Current: [What's wrong]
   - Fix: [How to fix]
   - Why: [Impact/reasoning]

---

### 💡 Suggestions (Nice to Have)
1. [Suggestion] - Line X
   - Current: [Current approach]
   - Alternative: [Suggested approach]
   - Benefit: [Why it's better]

---

### 📊 Metrics
- Story Alignment: ✅ Complete
- Code Quality: 8/10
- Security: ⚠️ Issues Found (2 high, 1 medium)
- Maintainability: 7/10
- Test Coverage: 75% (target: 80%)

---

### 🎯 Next Steps
1. [Action item]
2. [Action item]
3. [Action item]

---

### 📚 Resources
- [Link to relevant documentation]
- [Link to similar implementation]
- [Link to security guide]

Quick Reference Checklist

Use this for rapid reviews:

Story Alignment

• Implements all acceptance criteria
• No scope creep
• Story referenced in commit/PR

Code Quality

• Clear, descriptive names
• Functions < 20 lines
• No code duplication
• Consistent style
• Low complexity

Security (OWASP)

• Input validation
• Output encoding
• Authentication/authorization
• No injection vulnerabilities
• Secrets management
• Error handling secure

Maintainability

• Easy to read
• Easy to test
• Well documented
• Low coupling
• High cohesion

Testing

• Critical paths tested
• Edge cases covered
• Error scenarios tested
• Tests are meaningful

Performance

• No N+1 queries
• Appropriate caching
• Efficient algorithms
• No blocking operations

Architecture

• Follows project patterns
• Consistent with existing code
• Appropriate abstractions
• Proper file structure

Examples

Example 1: Full PR Review

Context: PR adds user authentication endpoint

Review:

## Code Review: User Authentication Endpoint (#234)

**Overall Assessment:** Approve with Changes (Security issues must be fixed)

---

### ✅ Strengths
- Clean separation of concerns (controller → service → repository)
- Comprehensive unit tests for service layer
- Good error handling with appropriate status codes
- Clear, descriptive function names

---

### 🚨 Critical Issues (Must Fix Before Merge)

1. **Password Storage - Line 67** (OWASP A02: Cryptographic Failures)
   - Current: Passwords stored in plaintext
   ```javascript
   user.password = req.body.password; // ❌

• Fix: Hash password before storing

user.password = await bcrypt.hash(req.body.password, 10);

• Why: If database is compromised, all user passwords are exposed. This is a critical security vulnerability.

2. SQL Injection - Line 89 (OWASP A03: Injection)
   • Current: String concatenation in query

   db.query(`SELECT * FROM users WHERE email = '${email}'`); // ❌
   

   • Fix: Use parameterized query

   db.query('SELECT * FROM users WHERE email = ?', [email]);
   

   • Why: Attackers can inject SQL commands to access or modify any data in the database.

⚠️ Important Issues (Should Fix)

1. Missing Rate Limiting - auth.controller.js

   • Current: No rate limiting on login endpoint
   • Recommendation: Add rate limiting middleware

   router.post('/login', rateLimiter({ max: 5, windowMs: 15 * 60 * 1000 }), login);
   

   • Why: Prevents brute force attacks on user accounts

2. Incomplete Story Implementation - Issue #234

   • Story AC3: "User receives email confirmation after registration"
   • Current: Email confirmation not implemented
   • Recommendation: Either implement or create follow-up ticket and note in PR

💡 Suggestions (Nice to Have)

1. Extract Validation Logic - Line 45-62

   • Current: Validation inline in controller (18 lines)
   • Suggestion: Extract to separate validator

   const errors = validateUserRegistration(req.body);
   if (errors.length > 0) return res.status(400).json({ errors });
   

   • Benefit: Reusable validation, easier to test, controller stays focused

2. Add JSDoc Comments - user.service.js

   • Functions would benefit from parameter and return type documentation
   • Example:

   /**
    * Registers a new user with the provided credentials
    * @param {Object} userData - User registration data
    * @param {string} userData.email - User's email address
    * @param {string} userData.password - User's password (will be hashed)
    * @returns {Promise<User>} Newly created user object
    * @throws {ValidationError} If email already exists
    */
   

📊 Metrics

• Story Alignment: ⚠️ Partial (missing AC3)
• Code Quality: 8/10 (clean structure, minor improvements possible)
• Security: 🚨 Critical Issues (2 must fix)
• Maintainability: 8/10 (well-organized, good naming)
• Test Coverage: 82% ✅ (exceeds 80% target)
• Performance: ✅ No issues identified

🎯 Next Steps

1. Fix critical security issues (password hashing, SQL injection)
2. Add rate limiting to prevent brute force attacks
3. Address incomplete story (email confirmation) - implement or create follow-up ticket
4. Consider extracting validation logic for reusability
5. Re-request review after changes

📚 Resources

• OWASP Top 10
• bcrypt documentation
• Express rate limiting
• Parameterized queries guide

---

## Integration with Other Skills

This skill works well with:

- **security-architect** - For detailed threat modeling and security architecture
- **technical-architecture** - For architecture pattern validation
- **devops** - For CI/CD and deployment security reviews

When detailed security architecture review is needed, recommend using the security-architect skill for comprehensive analysis.

## Output Format

Every code review should include:

1. **Summary** - Overall assessment and decision (approve/request changes)
2. **Strengths** - What's done well
3. **Critical Issues** - Must fix (security, bugs, data loss)
4. **Important Issues** - Should fix (quality, maintainability)
5. **Suggestions** - Nice to have (optimizations, improvements)
6. **Metrics** - Quantified assessment across dimensions
7. **Next Steps** - Clear action items
8. **Resources** - Helpful links and documentation

Always be constructive, specific, and explain the "why" behind every suggestion.