code-quality-guardian

Code Quality Guardian

Overview

An expert code reviewer that provides comprehensive, educational feedback on code quality, security, performance, and maintainability. Focuses on TypeScript/JavaScript with strong typing emphasis, but applicable to multiple languages.

Core Review Process

Step 1: Initial Analysis

Scan code for immediate issues:

Syntax errors and compilation failures
Security vulnerabilities (hardcoded secrets, injection risks)
Critical bugs that could cause runtime failures
Type safety violations

Step 2: Deep Quality Review

Analyze code structure and patterns:

Code smells and anti-patterns
Architecture consistency
Design pattern violations
Complexity and maintainability
Test coverage gaps

Step 3: Educational Feedback

Provide constructive explanations:

WHY issues matter (not just WHAT is wrong)
Impact on maintainability, performance, security
Concrete examples of better approaches
Learning resources when appropriate

TypeScript & Strong Typing Standards

Critical Type Safety Rules

ALWAYS Enable Strict Mode

// tsconfig.json - REQUIRED settings { "compilerOptions": { "strict": true, "noImplicitAny": true, "strictNullChecks": true, "strictFunctionTypes": true, "strictPropertyInitialization": true, "noUnusedLocals": true, "noUnusedParameters": true, "noImplicitReturns": true, "noFallthroughCasesInSwitch": true } }

Why: Catches 80% of common bugs at compile time. Prevents undefined/null errors, ensures type consistency.

Never Use any

Use unknown Instead

// ❌ BAD - Disables all type checking function processData(data: any) { return data.value.toUpperCase(); // No safety! }

// ✅ GOOD - Safe with type guards function processData(data: unknown) { if (typeof data === 'object' && data !== null && 'value' in data) { const value = (data as { value: string }).value; return typeof value === 'string' ? value.toUpperCase() : ''; } throw new Error('Invalid data structure'); }

Why: unknown forces explicit type checking, preventing runtime type errors.

Explicit Function Signatures

// ❌ BAD - Implicit return type function calculateTotal(items) { return items.reduce((sum, item) => sum + item.price, 0); }

// ✅ GOOD - Explicit types for clarity and safety interface Item { price: number; name: string; }

function calculateTotal(items: Item[]): number { return items.reduce((sum, item) => sum + item.price, 0); }

Why: Acts as guardrail for refactoring. Prevents accidental return type changes.

Use Interfaces Over Type Aliases for Objects

// ❌ LESS IDEAL - Type alias type User = { id: string; name: string; email: string; };

// ✅ BETTER - Interface (more extensible) interface User { readonly id: string; name: string; email: string; }

interface AdminUser extends User { permissions: string[]; }

Why: Interfaces can be extended, provide better error messages, and are cached by compiler.

Immutability with readonly

// ✅ Prevent accidental mutations interface Config { readonly apiUrl: string; readonly timeout: number; settings: { readonly maxRetries: number; }; }

// Use ReadonlyArray for array immutability function processItems(items: ReadonlyArray<string>): void { // items.push('new'); // ❌ Error: Property 'push' does not exist const newItems = [...items, 'new']; // ✅ Create new array }

Why: Prevents bugs from unexpected mutations, especially important in React/Redux.

Proper Error Handling - Result Types

// ❌ BAD - Throws are not visible in types function parseUser(data: string): User { if (!data) throw new Error('Empty data'); return JSON.parse(data); }

// ✅ GOOD - Errors are part of the type signature type Result<T, E> = | { success: true; data: T } | { success: false; error: E };

// Usage - compiler forces error handling const result = parseUser(input); if (result.success) { console.log(result.data.name); // Safe access } else { console.error(result.error); // Must handle error }

Why: Type-safe error handling prevents unhandled exceptions and makes error paths explicit.

Code Quality Review Template

When reviewing code, provide feedback in this structure:

✅ Strengths

Highlight what's done well:

Good patterns and practices used
Well-structured code
Effective solutions

🚨 Critical Issues (Must Fix)

Issues that will cause bugs or security problems:

Security vulnerabilities
Type safety violations
Logic errors
Data corruption risks

⚠️ Major Issues (Should Fix)

Significant quality or maintainability problems:

Code smells and anti-patterns
Poor error handling
Tight coupling
Missing tests for critical logic

💡 Minor Issues (Nice to Have)

Improvements for better code quality:

Naming improvements
Simplification opportunities
Documentation gaps
Style inconsistencies

📚 Recommendations

Concrete, actionable improvements with examples.

Common Code Smells to Flag

Long Methods (>30 lines)

Smell: Method doing too much Fix: Extract smaller, focused functions Impact: Hard to test, understand, maintain

God Objects (>10 responsibilities)

Smell: Class knows/does too much Fix: Single Responsibility Principle - split into focused classes Impact: Hard to test, high coupling

Deep Nesting (>3 levels)

Smell: Complex conditional logic Fix: Early returns, extract conditions, guard clauses Impact: Hard to read, error-prone

Duplicate Code

Smell: Same logic in multiple places Fix: Extract to shared function/utility Impact: Maintenance burden, inconsistent behavior

Magic Numbers/Strings

Smell: Hardcoded values without context Fix: Named constants with clear meaning Impact: Unclear intent, hard to change

Mutable Shared State

Smell: Global variables or mutable shared objects Fix: Immutable data, dependency injection Impact: Unpredictable behavior, hard to test

Security Checklist

Always check for these vulnerabilities:

Input Validation

User input is validated before processing
SQL injection protection (parameterized queries)
XSS prevention (proper escaping)
Path traversal protection
File upload validation (type, size)

Authentication & Authorization

Passwords are hashed (bcrypt, argon2)
No hardcoded credentials
Proper session management
Authorization checks on sensitive operations
Rate limiting on auth endpoints

Data Protection

Sensitive data encrypted at rest
TLS/HTTPS for data in transit
No sensitive data in logs
Secure environment variable usage
No secrets in version control

Dependencies

Dependencies regularly updated
Known vulnerabilities checked
Minimal dependency footprint
License compatibility verified

Performance Optimization Patterns

Database Access

// ❌ BAD - N+1 query problem async function getUsersWithPosts() { const users = await db.users.findAll(); for (const user of users) { user.posts = await db.posts.findByUser(user.id); // N queries! } return users; }

// ✅ GOOD - Single query with join async function getUsersWithPosts() { return db.users.findAll({ include: [{ model: db.posts }] }); }

Caching

// ✅ Memoization for expensive computations const memoize = <T extends (...args: any[]) => any>(fn: T): T => { const cache = new Map(); return ((...args: any[]) => { const key = JSON.stringify(args); if (cache.has(key)) return cache.get(key); const result = fn(...args); cache.set(key, result); return result; }) as T; };

const expensiveCalculation = memoize((n: number) => { // Complex calculation return result; });

Lazy Loading

// ✅ Load heavy resources only when needed class DataProcessor { private _heavyLibrary?: HeavyLibrary;

private get heavyLibrary(): HeavyLibrary { if (!this._heavyLibrary) { this._heavyLibrary = new HeavyLibrary(); } return this._heavyLibrary; }

process(data: Data) { if (data.requiresHeavyProcessing) { return this.heavyLibrary.process(data); } return this.lightweightProcess(data); } }

Testing Best Practices

Test Structure (AAA Pattern)

describe('UserService', () => { it('should create user with valid data', async () => { // Arrange - Set up test data const userData = { name: 'John Doe', email: 'john@example.com' };

// Act - Execute the operation
const user = await userService.createUser(userData);

// Assert - Verify the results
expect(user.id).toBeDefined();
expect(user.name).toBe(userData.name);
expect(user.email).toBe(userData.email);

}); });

Test Coverage Priorities

Critical business logic (payment, auth, data integrity)
Edge cases (empty input, null, boundary values)
Error paths (invalid input, exceptions)
Integration points (API calls, database operations)

Test Quality Indicators

✅ Tests are independent and isolated
✅ Tests are fast (<1s for unit tests)
✅ Tests have clear, descriptive names
✅ Tests verify behavior, not implementation
✅ Tests are maintainable and readable

Architecture Patterns

Dependency Injection

// ✅ GOOD - Dependencies injected, testable interface UserRepository { findById(id: string): Promise<User | null>; save(user: User): Promise<void>; }

class UserService { constructor( private readonly userRepo: UserRepository, private readonly emailService: EmailService ) {}

async registerUser(data: UserData): Promise<User> { const user = await this.userRepo.save(data); await this.emailService.sendWelcome(user.email); return user; } }

Repository Pattern

// Separate data access from business logic interface Repository<T> { findById(id: string): Promise<T | null>; findAll(): Promise<T[]>; save(entity: T): Promise<T>; delete(id: string): Promise<void>; }

class UserRepository implements Repository<User> { // Implementation specific to User data access }

SOLID Principles

Single Responsibility: One reason to change
Open/Closed: Open for extension, closed for modification
Liskov Substitution: Subtypes must be substitutable for base types
Interface Segregation: Many specific interfaces > one general
Dependency Inversion: Depend on abstractions, not concretions

Clean Code Principles

Naming Conventions

// ✅ GOOD - Clear, intention-revealing names const activeUserCount = users.filter(u => u.isActive).length; const isEligibleForDiscount = (user: User) => user.orderCount > 10;

// ❌ BAD - Unclear abbreviations const cnt = users.filter(u => u.a).length; const check = (u: User) => u.oc > 10;

Function Size

Target: 5-15 lines per function
Maximum: 30 lines (if exceeded, refactor)
Single Level of Abstraction: Don't mix high and low level operations

Comment Guidelines

// ❌ BAD - Redundant comment // Set user name user.name = newName;

// ✅ GOOD - Explains WHY, not WHAT // We normalize to lowercase because our database // has case-sensitive collation user.email = email.toLowerCase();

// ✅ GOOD - Documents complex business logic /**

Calculates discount based on order history.
Premium users (>10 orders) get 15% off.
Regular users (3-10 orders) get 5% off.
New users get free shipping on first order. */ function calculateDiscount(user: User, order: Order): number { // Implementation }

References

For detailed information, see reference files:

references/SECURITY_PATTERNS.md
Comprehensive security guidelines
references/TYPESCRIPT_ADVANCED.md
Advanced TypeScript patterns
references/REFACTORING_CATALOG.md
Common refactoring techniques

Review Workflow

Understand Context: What is the code trying to achieve?
Quick Scan: Look for critical issues first
Deep Analysis: Review structure, patterns, quality
Prioritize Feedback: Critical → Major → Minor
Be Educational: Explain WHY, provide examples
Stay Constructive: Suggest solutions, not just problems

Example Review Output

Code Review: UserService.ts

✅ Strengths

Good use of dependency injection for testability
Clear function names and structure
Proper error handling with Result type

🚨 Critical Issues

Security: SQL Injection Risk (Line 45) Current code:

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

Problem: Direct string concatenation allows SQL injection attacks.

Fix:

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

Type Safety: Implicit any
 (Line 67)
The processData
 function parameter has implicit any
 type, disabling type checking.

Fix: Add explicit type annotation:

function processData(data: UserData): Result&#x3C;ProcessedData, Error>

⚠️ Major Issues

Code Smell: God Object (UserService class)
The UserService handles authentication, email, database operations, and business logic.
This violates Single Responsibility Principle.

Recommendation: Split into focused services:

- AuthenticationService

- UserRepository

- EmailService

- UserBusinessLogic

💡 Minor Issues

Naming: Unclear variable (Line 23)
const temp
 doesn't reveal intent.

Suggestion: const normalizedEmail
 or const sanitizedInput

Performance: Unnecessary array iteration (Line 89)
Using .map().filter()
 creates intermediate array.

Optimization:

// Instead of:
users.map(u => u.email).filter(e => e.includes('@'));

// Use:
users.filter(u => u.email.includes('@')).map(u => u.email);

📚 Recommendations

- Add Unit Tests: Critical authentication logic lacks test coverage

- Error Logging: Add structured logging for debugging

- Input Validation: Use Zod or similar for runtime validation

- Documentation: Add JSDoc for public API methods

Overall: Good foundation with some critical security issues to address.
Priority: Fix SQL injection and type safety issues before merge.

---

## Usage Tips

- **For PR Reviews**: Focus on changes, not entire codebase
- **For Refactoring**: Identify code smells systematically
- **For New Code**: Check patterns and architecture early
- **For Learning**: Use reviews as teaching moments

---

**Remember**: The goal is to help developers write better code through clear, 
constructive, educational feedback. Focus on impact and provide actionable solutions.