code-quality-review

What this skill does

# Code Quality Review

Systematic code quality assessment for ultrawork verification phase.

## What This Skill Provides

Structured checklists for evaluating code quality across multiple dimensions:
- Logic and readability
- Error handling and resilience
- Code style and maintainability
- Performance and efficiency

## When to Use This Skill

- During VERIFICATION phase after workers complete tasks
- When reviewing evidence before marking tasks as complete
- Before approving design decisions that impact code quality
- When auditing implementations for production readiness

## Quality Dimensions

### 1. Logic and Readability

**Checklist:**

```
[ ] Functions have single, clear responsibilities
[ ] Functions are under 50 lines
[ ] Files are under 800 lines
[ ] Nesting depth is 4 levels or less
[ ] Control flow is straightforward (no convoluted conditionals)
[ ] Variable and function names are descriptive
[ ] No magic numbers (constants are named)
[ ] Complex logic has explanatory comments
[ ] Boolean conditions are readable (avoid double negatives)
[ ] Early returns reduce nesting
```

**Red Flags:**
- Functions doing multiple unrelated things
- Variable names like `x`, `tmp`, `data`, `thing`
- Magic numbers: `if (status === 42)`
- Deep nesting: `if { if { if { if { ... }}}}`
- Complex conditions: `if (!(!isValid && !isExpired))`

**Examples:**

```javascript
// ❌ Bad: Unclear logic, magic numbers
function process(x) {
  if (x > 100 && x < 500) {
    return x * 0.95;
  }
  return x;
}

// ✅ Good: Clear intent, named constants
const BULK_ORDER_MIN = 100;
const BULK_ORDER_MAX = 500;
const BULK_DISCOUNT_RATE = 0.95;

function applyBulkDiscount(orderTotal) {
  const isBulkOrder = orderTotal >= BULK_ORDER_MIN && orderTotal <= BULK_ORDER_MAX;
  return isBulkOrder ? orderTotal * BULK_DISCOUNT_RATE : orderTotal;
}
```

### 2. Error Handling

**Checklist:**

```
[ ] All async operations have error handling
[ ] try/catch blocks used for risky operations
[ ] Errors are logged with context
[ ] User-facing errors are sanitized
[ ] Error messages are actionable
[ ] No silent failures (empty catch blocks)
[ ] Edge cases are handled (null, undefined, empty arrays)
[ ] Input validation exists for external data
[ ] Database operations handle connection failures
[ ] API calls have timeout handling
```

**Red Flags:**
- Empty catch blocks: `catch (e) {}`
- Unhandled promise rejections
- Missing null checks before property access
- Generic error messages: `throw new Error('error')`
- Exposing stack traces to users

**Examples:**

```javascript
// ❌ Bad: Silent failure
try {
  await saveUser(user);
} catch (e) {
  // Silent failure - data loss risk
}

// ✅ Good: Proper error handling
try {
  await saveUser(user);
} catch (error) {
  logger.error('Failed to save user', { userId: user.id, error });
  throw new Error('Unable to save user data. Please try again.');
}
```

### 3. Code Duplication

**Checklist:**

```
[ ] No copy-pasted code blocks
[ ] Repeated logic is extracted to functions
[ ] Similar patterns use shared utilities
[ ] Constants are defined once
[ ] Configuration is centralized
[ ] Validation logic is reusable
[ ] Data transformation uses shared helpers
```

**Red Flags:**
- Same code in multiple places with minor variations
- Repeated validation patterns
- Multiple implementations of same algorithm
- Copy-pasted API error handling

**Examples:**

```javascript
// ❌ Bad: Duplication
function validateEmail(email) {
  return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email);
}
function isValidEmail(email) {
  return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email);
}

// ✅ Good: Single source of truth
const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
function validateEmail(email) {
  return EMAIL_REGEX.test(email);
}
```

### 4. Code Style

**Checklist:**

```
[ ] Consistent indentation (spaces/tabs)
[ ] Consistent naming convention (camelCase, PascalCase, etc.)
[ ] Consistent quote style (single/double)
[ ] No trailing whitespace
[ ] Proper spacing around operators
[ ] Consistent brace style
[ ] No commented-out code
[ ] No console.log statements in production code
[ ] Imports are organized (grouped, sorted)
[ ] No unused imports or variables
```

**Red Flags:**
- Mixed quote styles in same file
- Inconsistent naming: `getUserData()`, `get_user_info()`
- Large blocks of commented code
- Debug statements: `console.log('HERE')`

### 5. Performance

**Checklist:**

```
[ ] Algorithms use appropriate complexity (O(n) vs O(n²))
[ ] Expensive operations are cached
[ ] Database queries use indexes
[ ] N+1 query problems avoided
[ ] Large datasets are paginated
[ ] Unnecessary re-renders prevented (React)
[ ] Heavy computations are memoized
[ ] File operations are batched
[ ] Network requests are minimized
```

**Red Flags:**
- Nested loops over large datasets
- Database queries inside loops
- Missing pagination on list endpoints
- Re-computing same values in renders
- Loading entire dataset when pagination possible

**Examples:**

```javascript
// ❌ Bad: O(n²) complexity, N+1 queries
for (const order of orders) {
  const user = await db.users.findById(order.userId); // N+1!
  results.push({ ...order, user });
}

// ✅ Good: Single query with join
const ordersWithUsers = await db.orders
  .find({ id: { $in: orderIds } })
  .populate('user');
```

### 6. Testing

**Checklist:**

```
[ ] New code has corresponding tests
[ ] Tests verify actual behavior (not just pass)
[ ] Edge cases are tested
[ ] Error paths are tested
[ ] Test names describe what is tested
[ ] Tests are isolated (no shared state)
[ ] Mocks are used for external dependencies
[ ] Test coverage is adequate (not necessarily 100%)
```

**Red Flags:**
- No tests for new functionality
- Tests that always pass (no assertions)
- Flaky tests (sometimes pass/fail)
- Tests depending on execution order
- Testing implementation details instead of behavior

## Review Process

### Step 1: Static Analysis

```bash
# Check for syntax errors
npm run lint

# Check for type errors (TypeScript)
npm run type-check

# Check for security issues
npm audit
```

### Step 2: Code Reading

Read the modified files completely. Check:
- Does the code do what it claims?
- Are edge cases handled?
- Is error handling comprehensive?
- Is the logic clear?

### Step 3: Test Review

```bash
# Run tests
npm test

# Check coverage
npm run test:coverage
```

Verify:
- Tests actually test behavior
- Edge cases are covered
- Error conditions are tested

### Step 4: Performance Check

Look for:
- Algorithm complexity
- Database query patterns
- Caching opportunities
- Resource leaks

## Quality Scoring

| Category | Weight | Score |
|----------|--------|-------|
| Logic Clarity | 25% | 0-10 |
| Error Handling | 25% | 0-10 |
| Maintainability | 20% | 0-10 |
| Performance | 15% | 0-10 |
| Testing | 15% | 0-10 |

**Overall Score = Weighted Average**

- **8-10**: Excellent, ready to merge
- **6-7**: Good, minor improvements recommended
- **4-5**: Fair, significant improvements needed
- **0-3**: Poor, major rework required

## Common Issues and Fixes

| Issue | Fix |
|-------|-----|
| Large function | Extract smaller functions |
| Deep nesting | Use early returns, extract conditions |
| Magic numbers | Define named constants |
| No error handling | Add try/catch, validate inputs |
| Duplicated code | Extract shared utility |
| Poor naming | Rename to describe purpose |
| Missing tests | Write tests for new code |
| O(n²) loops | Use hash maps, optimize algorithm |

## Evidence Requirements

When reviewing code quality, collect evidence:

```
### Code Quality Assessment
- Logic Clarity: 8/10
  - Functions under 50 lines: ✓
  - Clear naming: ✓
  - No magic numbers: ✓

- Error Handling: 7/10
  - Try/catch present: ✓
  - Null checks: ✓
  - Missing timeout handling: ✗

- Maintainability: 9/10
  - No duplication: ✓
  - Consistent style: ✓
  - Well-tested: ✓

Overall: 8/10 - Ready to merge
```

## Integration with Ultrawork

During VERIFICATION phase:

```bash
# Review imp