Code Review

Systematic code review methodology for identifying issues, security vulnerabilities, and optimization opportunities.

Team Review Guidelines

Communication Style

When providing review feedback:

1. Be friendly and constructive

   • Good: “What do you think about using X here? It might help with Y.”
   • Good: “Nice solution! One small suggestion…”
   • Avoid: “This is wrong.”
   • Avoid: “You should have done X.”

2. Ask before assuming

   • Good: “I’m curious about this approach – could you explain the reasoning?”
   • Avoid: “Why didn’t you just…?”

3. Acknowledge good work

   • “Great catch on the edge case!”
   • “Clean and readable!”
   • “I learned something from this.”

Double Verification Process

Every PR requires TWO passes:

Pass 1 – Security (Blocking)

• All security checklist items MUST pass
• Any security issue = Request Changes

Pass 2 – Quality (Advisory)

• Standards, performance, maintainability
• These are suggestions, not blockers (unless severe)

Review Phases

Phase 1: Preparation

1. Understand Context

   • Read PR description and linked issues
   • Understand the feature/fix being implemented
   • Check if there are related changes in other PRs
   • Review the commit history for context

2. Identify Standards

   • Check project’s coding standards
   • Review existing patterns in the codebase
   • Note any specific requirements (performance, security)

Phase 2: Systematic Analysis

Work through each category methodically:

Security Review

Input Validation

“`javascript // BAD: No validation app.post(‘/user’, (req, res) => { db.query(`SELECT * FROM users WHERE id = ${req.body.id}`); });

// GOOD: Parameterized + validated app.post(‘/user’, (req, res) => { const schema = z.object({ id: z.number().positive() }); const { id } = schema.parse(req.body); db.query(‘SELECT * FROM users WHERE id = ?’, [id]); }); “`

Authentication & Authorization

“`javascript // BAD: Missing auth check app.delete(‘/api/posts/:id’, async (req, res) => { await Post.findByIdAndDelete(req.params.id); });

// GOOD: Verify ownership app.delete(‘/api/posts/:id’, authenticate, async (req, res) => { const post = await Post.findById(req.params.id); if (post.authorId !== req.user.id && !req.user.isAdmin) { return res.status(403).json({ error: ‘Forbidden’ }); } await post.delete(); }); “`

Security Checklist

• SQL/NoSQL injection prevented (parameterized queries)
• XSS prevented (output encoding, CSP headers)
• CSRF protection in place
• Sensitive data not logged or exposed
• API keys/secrets not hardcoded
• File uploads validated and sanitized
• Rate limiting on sensitive endpoints
• Proper error messages (no stack traces in production)

Performance Review

N+1 Query Detection

“`javascript // BAD: N+1 queries const posts = await Post.find(); for (const post of posts) { const author = await User.findById(post.authorId); // Query per post! }

// GOOD: Eager loading const posts = await Post.find().populate(‘author’); “`

Performance Checklist

• No N+1 queries (use eager loading)
• Expensive operations cached appropriately
• Database queries use proper indexes
• Large datasets paginated
• Async operations properly awaited
• No unnecessary re-renders (React)
• Memory leaks prevented (cleanup handlers)
• Heavy computations memoized or offloaded

Code Quality Checklist

• Clear, descriptive naming conventions
• Functions are single-purpose (< 30 lines ideal)
• Cyclomatic complexity reasonable (< 10)
• No code duplication (DRY principle)
• Proper error handling throughout
• Edge cases considered
• Magic numbers/strings extracted to constants
• Comments explain “why”, not “what”

Edge Cases Checklist

Always verify these scenarios are handled:

Data Edge Cases

• Empty arrays/collections
• Null or undefined values
• Zero values (especially in division)
• Negative numbers where only positive expected
• Empty strings vs null vs undefined
• Unicode and special characters in strings

User Input Edge Cases

• Missing required fields
• Fields with only whitespace
• Duplicate submissions (double-click)
• Concurrent updates to same resource
• Invalid date formats
• Timezone handling
• File upload with no file selected

API Edge Cases

• Network timeout handling
• Rate limit exceeded responses
• API returns unexpected data structure
• Partial success in batch operations
• Graceful degradation when service unavailable

WordPress-Specific Edge Cases

• Post with no featured image
• User with no display name
• Taxonomy with no posts
• Widget in empty sidebar
• Shortcode with no attributes
• Multisite subdirectory vs subdomain
• Translation missing for locale
• Cron job running during high traffic
• Option not yet saved (first install)
• Meta value of 0 vs meta not existing
• get_post() returning null
• WP_Query with no results
• Current user not logged in (ID = 0)

Language-Specific Checks

PHP/WordPress

• Escaping output (esc_html, esc_attr, etc.)
• Nonces for form submissions
• Capability checks for actions
• Prepared statements for queries
• WordPress coding standards followed
• Cron hooks: No name collision between cron hook and internal do_action()
• Cron scheduling: Events scheduled on activation, cleared on deactivation
• Hook callbacks: No recursive/self-triggering patterns
• Settings sanitization handles unchecked checkboxes
• Transient expiration set appropriately
• Object cache checked before expensive queries
• REST endpoints have permission_callback
• AJAX handlers verify nonce and capability

JavaScript/TypeScript

• === used instead of ==
• Proper TypeScript types (no any abuse)
• async/await error handling
• No prototype pollution risks
• ESLint/Prettier rules followed

React

• Hooks rules followed
• Keys provided for lists
• useEffect dependencies correct
• No state mutations
• Proper component composition

Review Output Format

Structure feedback as:

Critical Issues (Must Fix)

• Security vulnerabilities
• Data loss risks
• Breaking changes without migration
• Failing tests

Warnings (Should Fix)

• Performance concerns
• Code maintainability
• Missing error handling

Suggestions (Nice to Have)

• Refactoring opportunities
• Better naming
• Additional documentation

Positive Notes

• Good patterns followed
• Clean code
• Thorough testing