code-review-and-quality

Code Review and Quality

Overview

Review code across five axes: Correctness, Readability, Architecture, Security, and Performance. The approval standard: "Approve when it definitely improves overall code health of the system." Every finding is categorized and actionable.

When to Use

• Reviewing a pull request (own or teammate's)
• Self-review before creating a PR
• Requested code quality check on a specific file or module
• After completing a feature (final quality gate)

Skip when: Not reviewing code (this is a review skill, not a writing skill).

Core Process

Step 1: Understand Context

1. Read the PR description — what problem does it solve?
2. Read the spec or ticket — does the PR match the stated goal?
3. Check the diff size — target ~100 lines, flag >1000 lines for splitting

Step 2: Review Tests First

1. Start with test files — they reveal the intended behavior
2. Check for:
   • Are critical paths tested?
   • Are edge cases covered (null, empty, error, boundary values)?
   • Do test names describe behavior?
   • Are tests independent (no shared mutable state)?

Step 3: Five-Axis Review

Axis 1: Correctness

1. Verify behavior matches intent:
   • Does the code handle all states? (loading, success, error, empty)
   • Are nulls handled safely? (no !!, proper ?. chains)
   • Are coroutines structured correctly? (proper scope, cancellation)
   • Are lifecycle-aware collections used? (collectAsStateWithLifecycle)
   • Do Room queries match the schema?
   • Are migrations correct and tested?

Axis 2: Readability

1. Kotlin-specific readability:

// GOOD: idiomatic Kotlin
val activeTask = tasks.firstOrNull { !it.completed }
    ?: return TaskListUiState.Empty

// BAD: Java-style
var activeTask: Task? = null
for (task in tasks) {
    if (!task.completed) {
        activeTask = task
        break
    }
}
if (activeTask == null) return TaskListUiState.Empty

1. Check for:
   • Clear naming (functions describe actions, variables describe content)
   • Appropriate use of when expressions, let/also/apply, extension functions
   • Functions under ~40 lines
   • Sealed classes/interfaces for exhaustive state handling
   • No nested callbacks (use coroutines)