Code Review And Quality

---
name: code-review-and-quality
description: >-
  Use when reviewing Android code (own or others'). Five-axis review
  framework: Correctness, Readability, Architecture, Security, Performance.
  Categorized findings with Kotlin/Compose-specific checks.
---

# 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:**

```kotlin
// 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:**

| Category | Description | Action Required |
|----------|-------------|----------------|
| **Critical** | Security vulnerability, data loss, crash | Must fix before merge |
| **Important** | Missing tests, architecture violation, bug risk | Should fix before merge |
| **Suggestion** | Better Kotlin idiom, readability improvement | Optional, author's discretion |
| **Nit** | Formatting, naming preference | Optional |
| **FYI** | Context or explanation, no action needed | Informational |

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

| Shortcut | Why It Fails |
|----------|-------------|
| "LGTM" without reading the code | Rubber-stamp reviews miss bugs. They also train teammates to skip reviews. |
| "I'll clean it up later" | Later never comes. Fix it now or create a tracked issue. |
| "It works, so it's fine" | Working code with poor architecture becomes non-working code during the next change. |
| "The author knows best" | Fresh eyes catch blind spots. That's the point of review. |
| "It's just a small change" | Small changes in the wrong layer create architectural debt. |
| "It's just a version bump" | A bump is a behavior change you didn't write. Read the changelog; semver doesn't guarantee no breakage. |
| "The tests cover it" | Prove it — mutate the new condition and see if anything goes red. |

## 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)