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

4. Start with test files — they reveal the intended behavior

5. 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)?
   • Would a test fail if the new behavior regressed?

   Answer the last question by experiment, not by reading: invert one condition the change adds (drop a !, swap && for ||, flip a >=), run that module's tests (./gradlew :module:testDebugUnitTest), then restore the file. A mutation that stays green is a finding — name the missing test case. For a project-wide mutation score, see constraint-driven-development.

Step 3: Five-Axis Review

Axis 1: Correctness

6. 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

7. 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

8. 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)

Axis 3: Architecture

9. Verify layer boundaries:
   • UI layer only calls ViewModel (never Repository/DAO directly)
   • Domain layer has no Android dependencies
   • Data layer implements domain interfaces
   • Feature modules don't depend on each other
   • No business logic in Composables
10. Check for:
    • Proper use of @Inject constructor (not field injection)
    • StateFlow exposed from ViewModel (not MutableStateFlow)
    • Repository pattern for data access
    • Single source of truth (local DB for offline-first)

Axis 4: Security

11. Check for:
    • No hardcoded secrets, API keys, or passwords
    • Input validation for user-provided data
    • Proper intent validation (exported components)
    • No logging of sensitive data (Log.d with tokens, passwords)
    • Secure storage (EncryptedSharedPreferences for sensitive data)
    • See security-and-hardening for comprehensive checklist

Axis 5: Performance

12. Check for:
    • N+1 query patterns in Room
    • Unbounded data loading (should use Paging3 for large datasets)
    • Unnecessary recompositions in Compose (unstable parameters, lambda allocations)
    • Heavy work on main thread (use withContext(Dispatchers.IO))
    • Memory leaks (Activity/Context references in singletons)
    • See performance-optimization for comprehensive checklist

Dependency Upgrades

An upgrade is a behavior change you didn't write. The riskiest ones are bulk "bump deps" PRs. Review them with the same discipline:

• Read the changelog, not just the version. A "patch" can change behavior; a major bump (Kotlin, AGP, Compose BOM, Room) needs its migration notes read and the breakage found.
• One dependency per change (or one tightly related group, e.g. a BOM and its artifacts). When a bulk bump breaks the build, you've lost which one did it.
• Let the tests decide. Green suite before and after. If coverage around the dependency's behavior is thin, that gap is the finding — add a test first.
• Mind the transitive graph. Review the resolved diff, not just gradle/libs.versions.toml: ./gradlew :app:dependencies --configuration releaseRuntimeClasspath before and after. One direct bump can move dozens of transitive versions — and can raise your effective minSdk or compileSdk.
• Keep verification honest. If the project uses dependency locking or gradle/verification-metadata.xml, regenerate them with Gradle (never hand-edit) and review their diff. For supply-chain risk, follow security-and-hardening.

Step 4: Categorize Findings

13. Use severity categories:

14. Format findings:

**[Critical]** `TaskRepository.kt:45` — API key hardcoded in source.
Move to `local.properties` and access via `BuildConfig`.

**[Important]** `TaskListViewModel.kt:23` — Uses `GlobalScope.launch`.
Use `viewModelScope.launch` for proper lifecycle management.

**[Suggestion]** `TaskMapper.kt:12` — Could use `copy()` instead
of manual field mapping for partial updates.

Step 5: Verify Build and Tests

15. Before approving:
    • ./gradlew test passes
    • ./gradlew assembleDebug builds
    • ./gradlew lint has no new warnings
    • ./gradlew detekt passes (if configured)

Common Rationalizations

Red Flags

• PR over 1000 lines without justification
• No tests in the PR
• Tests that only cover happy path
• !! (non-null assertion) without justification
• GlobalScope usage
• Mutable state exposed from ViewModel
• Business logic in Composables
• Feature module depending on another feature module
• Secrets or API keys in source code
• @Suppress annotations without explanatory comments
• A bulk "bump dependencies" PR with no changelog review and no per-dependency isolation
• verification-metadata.xml or lockfiles hand-edited or merged without reviewing their diff

Verification

• All five axes reviewed (Correctness, Readability, Architecture, Security, Performance)
• Tests reviewed first (coverage, edge cases, naming) — at least one new condition mutated to confirm a test catches its regression
• Dependency upgrades reviewed against changelogs, isolated per dependency, with the resolved-dependency diff checked
• Findings categorized (Critical, Important, Suggestion, Nit, FYI)
• Critical findings resolved before approval
• ./gradlew test passes
• ./gradlew assembleDebug builds
• ./gradlew lint clean
• PR size reasonable (~100 lines, flagged if >1000)