pavel-molyanov/code-reviewing

Code Review Methodology

Comprehensive code review methodology for ensuring production-ready quality and maintainable architecture.

Review Dimensions

Perform systematic analysis across these 11 dimensions:

1. Architectural Patterns

• Evaluate adherence to established architectural patterns (MVC, MVVM, Clean Architecture, etc.)
• Assess design patterns usage (Factory, Strategy, Observer, etc.)
• Verify layer separation and dependency direction
• Check for architectural anti-patterns (circular dependencies, god objects, tight coupling)

2. Separation of Concerns

• Validate single responsibility principle compliance
• Examine module boundaries and cohesion
• Review business logic vs presentation logic separation
• Assess data layer abstraction and persistence logic isolation

Good practices:

• One file = one responsibility (UserService in one file, PaymentService in another)
• Functions < 50 lines; if larger, break into smaller functions
• Maximum 3 levels of nesting; use early returns to reduce nesting
• High-level modules should not depend on low-level details

3. Code Readability & Maintainability

• Evaluate naming conventions (variables, functions, classes)
• Assess code organization and file structure
• Check for appropriate use of comments and documentation
• Review complexity metrics (cyclomatic complexity, nesting depth)
• Verify consistent code style and formatting

Good practices:

• Meaningful comments focus on "why" rather than obvious "what"
• DRY principle: extract repeated code into functions/modules
• Readable > clever: clear code is better than short but cryptic code
• No magic numbers: extract to named constants (MAX_UPLOAD_SIZE not 5242880)

4. Error Handling & Logging

• Examine error propagation strategy
• Verify appropriate use of try-catch blocks
• Check error messages clarity and actionability
• Assess graceful degradation and fallback mechanisms

Good practices (error handling):

• Always use try-catch for operations that can fail (API calls, DB operations, file I/O)
• Don't swallow errors: always re-throw after logging (unless explicitly handling)
• Fail fast: validate inputs early; throw errors immediately when invalid
• User-friendly errors: show generic message to users, log details internally

Logging review checklist:

• Key operations have logs (external calls, auth events, state transitions, business operations)
• Structured format used (JSON / logger library), not string concatenation or console.log
• Every log includes context: userId, action, resourceId (not just a bare message)
• Correlation/request ID propagated through call chain
• Log levels used correctly (info for success, warn for recoverable, error for failures)
• Error logs include stack traces
• No secrets or PII in logs (passwords, tokens, API keys, emails, phone numbers)
• No empty catch blocks (catch (e) {} — silent error swallowing)
• No logging inside tight loops (generates thousands of duplicate lines)

Automatic severity mappings:

| Pattern | Severity | |---------|----------| | Secrets or PII logged (tokens, passwords, emails in plaintext) | critical | | Empty catch block — error swallowed without logging | major | | External call (API, DB) without any logging | major | | Missing correlation/request ID in service handling requests | minor | | console.log / print used instead of structured logger | minor |

5. Type Safety (TypeScript/typed languages)

For TypeScript or other typed codebases:

• Validate type definitions completeness and accuracy
• Check for inappropriate use of any type (TypeScript) or equivalent loose typing
• Assess interface and type alias design
• Review generic type usage and constraints
• Verify null/undefined handling and optional chaining
• Check for type assertions and their justification

6. Testing Coverage

• Evaluate unit test presence and quality
• Assess test coverage for critical paths
• Review test organization and naming
• Check for integration and E2E test needs
• Verify mocking strategies and test isolation
• Assess edge case and error scenario coverage

Good practices:

• Tests needed for: business logic, validations, transforms, error handling
• Tests not needed for: simple getters/setters, one-line configs, trivial updates
• Rule: if mocking >3 dependencies → wrong test type, use integration test

7. Dependencies Management

• Review new dependencies necessity and appropriateness
• Check for dependency version conflicts
• Assess bundle size impact
• Verify security vulnerabilities (outdated packages)
• Evaluate licensing compatibility

Good practices:

• Verify imports exist before using: read source files to confirm exports match expected usage
• Check function signatures: ensure signatures match how you're calling them
• Prefer well-maintained packages: check npm/PyPI activity, security advisories
• Pin major versions: use ^ (caret) for npm to allow patch updates

8. Security Considerations

• Check for security vulnerabilities (injection, XSS, CSRF)
• Verify secrets management (no hardcoded credentials)
• Assess input validation and sanitization
• Review authentication and authorization logic
• Check for sensitive data exposure

Good practices:

• Never hardcode secrets: use environment variables (.env) for all sensitive data
• Always validate input: check types, formats, ranges before processing
• Sanitize user data: before database operations, API calls, or displaying
• Add to .gitignore: .env, *.key, credentials.json, secrets/

9. Performance Implications

• Identify potential performance bottlenecks
• Review algorithmic complexity
• Check for unnecessary re-renders (React) or recomputations
• Assess memory leak risks
• Evaluate database query efficiency

Good practices:

• Avoid N+1 queries: use batch operations, eager loading, or caching
• Cache expensive computations: use memoization for functions
• Prevent memory leaks: clean up event listeners, timers, subscriptions in cleanup functions
• Use pagination for large datasets: don't load all records at once
• Profile before optimizing: measure actual bottlenecks before making changes

10. Cross-File Consistency

For the code under review, verify correctness of function/class usage:

Process:

1. When code CALLS a function from another file → Read that file, verify signature matches
2. When code USES a class/method → Read class definition, verify method exists and signature matches
3. When code IMPORTS something → Verify import path is correct

What to check:

• Function called with correct arguments
• Method exists on the class
• Import paths are valid
• Types match (if TypeScript)

Report as issue if:

• Function called with wrong arguments (runtime crash)
• Method doesn't exist (runtime crash)
• Import path broken (load failure)

Read the source files where functions/classes are defined to verify signatures match.

11. Resource Management

• Identify heavy resources: ML models, database connection pools, browser instances, API clients, large caches
• Check if heavy resources are created as singletons (one instance shared) or duplicated across files/components
• When code creates a heavy resource (new Model(), ModelClass(...), create_pool()): search the project for other instantiations of the same class
• Verify resource lifecycle: who creates, who consumes, when disposed
• Check for resource leaks: opened connections/files/handles that are never closed

Automatic severity mappings:

| Pattern | Severity | |---------|----------| | Same heavy resource class instantiated in multiple files without shared instance | major | | Heavy resource created inside a loop or per-request handler | critical | | Resource opened but never closed (connection, file handle, cursor) | major |

Dimension Prioritization

Focus on dimensions based on code context:

| Context | Prioritize | Reason | |---------|------------|--------| | Auth/login code | Security (8), Error Handling (4) | Auth vulnerabilities are critical | | User input handling | Security (8), Type Safety (5) | Input validation prevents attacks | | Database queries | Security (8), Performance (9) | SQL injection, N+1 queries | | New feature | Architecture (1), Testing (6) | Foundation for future changes | | Refactoring | Cross-File (10), Testing (6) | Avoid breaking existing code | | Performance fix | Performance (9), Dependencies (7) | Target the actual bottleneck | | Typed codebase | Type Safety (5), Cross-File (10) | Type errors cause runtime crashes | | ML/AI pipeline | Resource Mgmt (11), Performance (9) | Heavy models duplicated waste memory | | Microservice init | Resource Mgmt (11), Architecture (1) | Connection pools and clients should be shared |

Review Process

1. Initial Scan: Quick overview to understand scope and context
2. Deep Analysis: Systematic review of each dimension listed above
3. Cross-Reference: Compare implementation against userspec, techspec, and project standards
4. Issue Categorization: Classify findings by severity:
   • critical → blocking issues that must be fixed
   • major → significant concerns that should be addressed
   • minor → improvements that are valuable but optional
5. Recommendation Formulation: Provide specific, actionable suggestions

Quality Standards

Be thorough but pragmatic:

• Focus on issues that materially impact code quality, security, or maintainability
• Distinguish between critical problems and stylistic preferences
• Provide constructive feedback with specific examples
• Acknowledge good practices when present
• Consider project context and constraints from project documentation (if available)
• Balance idealism with practical delivery needs

Communication Style

• Be direct and specific - avoid vague feedback
• Use technical precision appropriate for senior developers
• Provide code examples in recommendations when helpful
• Explain the "why" behind each issue, not just the "what"
• Maintain professional, respectful tone
• Prioritize actionability over completeness

Goal: ensure production-ready code that is secure, maintainable, and aligned with project standards. Be thorough in analysis but efficient in communication.