Code Review Plus#

Enhanced code review covering security, performance, correctness, maintainability, testing, AI/ML, and language-specific best practices.

Review Dimensions#

DimensionFocusPrioritySecurityVulnerabilities, auth, data exposureCriticalPerformanceSpeed, memory, scalability bottlenecksHighCorrectnessLogic errors, edge cases, data integrityHighMaintainabilityReadability, structure, future-proofingMediumTestingCoverage, quality, reliability of testsMediumAI/MLModel safety, data leakage, biasHighLanguageIdiomatic patterns, best practicesMedium

Security Checklist#

• SQL Injection — All queries use parameterized statements or ORM
• XSS — User content escaped/sanitized before rendering
• CSRF Protection — State-changing requests require valid CSRF tokens
• Authentication — Protected endpoints verify authentication
• Authorization — Resource access scoped to requesting user
• Input Validation — All external input validated server-side
• Secrets Management — No credentials in source code
• Dependency Safety — Dependencies from trusted sources, no known CVEs
• Sensitive Data — PII/tokens never logged or in error messages
• Rate Limiting — Public/auth endpoints have rate limits
• File Upload Safety — Files validated for type/size, stored outside webroot
• HTTP Security Headers — CSP, X-Content-Type-Options, HSTS set

AI/ML-Specific Checklist#

• Data Leakage — Training data not exposed in predictions
• Prompt Injection — User input not injected into prompts without sanitization
• Model Safety — Output filtering for harmful content
• Bias Detection — Model outputs checked for demographic bias
• Cost Control — API calls budgeted, loops prevented
• Fallback Handling — Graceful degradation when AI services fail
• Token Limits — Input/output token limits enforced
• Caching — Expensive AI calls cached where appropriate
• Rate Limiting — AI API calls rate-limited
• Logging — AI interactions logged for debugging/audit

Performance Checklist#

• N+1 Queries — Database access patterns batched/joined
• Unnecessary Re-renders — Components re-render only when needed
• Memory Leaks — Event listeners, subscriptions cleaned up
• Bundle Size — Dependencies tree-shakeable, large libs loaded dynamically
• Lazy Loading — Heavy components use code splitting
• Caching Strategy — Expensive computations use caching
• Database Indexing — Queries filter/sort on indexed columns
• Pagination — List endpoints use pagination
• Async Operations — Long tasks offloaded to background jobs
• Image Optimization — Proper sizing, modern formats, CDN

Correctness Checklist#

• Edge Cases — Empty arrays, zero values, max values handled
• Null/Undefined — Nullable values checked before access
• Off-by-One — Loop bounds, pagination offsets verified
• Race Conditions — Concurrent access uses locks/transactions
• Timezone Handling — Dates stored in UTC
• Unicode — String ops handle multi-byte characters
• Integer Overflow — Large number arithmetic uses BigInt/Decimal
• Error Propagation — Async errors caught and handled
• State Consistency — Multi-step mutations transactional
• Boundary Validation — Boundary values tested

Maintainability Checklist#

• Naming Clarity — Descriptive names revealing intent
• Single Responsibility — Each function/class does one thing
• DRY — Duplicated logic extracted to utilities
• Cyclomatic Complexity — Low branching complexity
• Error Handling — Errors caught, logged, surfaced meaningfully
• Dead Code — Commented code, unused imports removed
• Magic Numbers — Literals extracted to named constants
• Consistent Patterns — New code follows codebase conventions
• Function Length — Short enough to understand at a glance
• Dependency Direction — Dependencies point inward

Testing Checklist#

• Test Coverage — New logic paths have tests
• Edge Case Tests — Boundary values, empty inputs tested
• No Flaky Tests — Deterministic, no timing reliance
• Test Independence — Tests set up/tear down own state
• Meaningful Assertions — Assert on behavior, not implementation
• Test Readability — Arrange-Act-Assert pattern
• Mocking Discipline — Only external boundaries mocked
• Regression Tests — Bug fixes include reproducing tests

Language-Specific Patterns#

Python#

• Type hints for public functions
• Context managers for resource cleanup
• Virtual environments for dependency isolation
• Black/flake8 formatting
• Docstrings for public APIs

JavaScript/TypeScript#

• ESLint/Prettier formatting
• TypeScript strict mode
• Async/await over raw promises
• Optional chaining for null safety
• Proper error boundaries in React

Go#

• Error handling with errors.Is/As
• Context propagation
• Proper goroutine lifecycle
• gofmt/goimports formatting
• Interface design patterns

Review Process#

PassFocusTimeWhat to Look ForFirstHigh-level structure2-5 minArchitecture, file organization, API designSecondLine-by-line detailBulkLogic, security, performance, edge casesThirdEdge cases & hardening5 minFailure modes, concurrency, boundary values

Severity Levels#

LevelLabelMeaningBlocks Merge?Critical[CRITICAL]Security vulnerability, data loss, crashYesMajor[MAJOR]Bug, logic error, performance regressionYesMinor[MINOR]Future maintenance cost reductionNoNitpick[NIT]Style preference, naming suggestionNo

Giving Feedback#

Principles#

• Be specific — Point to exact line and explain issue
• Explain why — State risk or consequence
• Suggest a fix — Offer concrete alternative or code snippet
• Ask, don't demand — Use questions for subjective points
• Acknowledge good work — Call out clean solutions
• Separate blocking from non-blocking — Use severity labels

Example Comments#

Bad:

This is wrong. Fix it.

Good:

[MAJOR] This query interpolates user input directly into SQL (line 42), vulnerable to SQL injection. Consider:

sql

SELECT * FROM users WHERE id = $1

Anti-Patterns#

Anti-PatternDescriptionRubber-StampingApproving without readingBikesheddingDebating variable names while ignoring bugsBlocking on StyleRefusing over formatting a linter should enforceGatekeepingRequiring personal preferred approachDrive-by ReviewsOne vague comment and disappearingScope CreepRequesting unrelated refactorsStale ReviewsLetting PRs sit for daysEmotional Language"This is terrible" or "obviously wrong"

NEVER Do#

1. NEVER approve without reading every changed line
2. NEVER block a PR solely for style preferences
3. NEVER leave feedback without severity level
4. NEVER request changes without explaining why
5. NEVER review more than 400 lines in one sitting
6. NEVER skip the security checklist
7. NEVER make it personal