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

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:

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)