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 ( 3 levels)
• Limit function parameters ( 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 ( 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 Compatibili

…

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: tomas-u
• Source: tomas-u/claude-skills
• License: MIT

Install and usage instructions live in the source repository linked above.