🔍 Pull Request Review Guide

Overview

Code review is one of the most important quality gates in software development. A well-conducted PR review catches bugs, improves code quality, shares knowledge, and maintains consistency across the codebase.

This guide provides a comprehensive framework for conducting effective pull request reviews in our Android/Kotlin codebase.

┌─────────────────────────────────────────────────────────────────────────┐
│                        PR REVIEW WORKFLOW                                │
├─────────────────────────────────────────────────────────────────────────┤
│                                                                          │
│   PR Created → Context Check → Code Review → Testing → Feedback → Merge │
│       │              │              │           │          │         │   │
│       ▼              ▼              ▼           ▼          ▼         ▼   │
│    Author       Understand      Review     Verify    Approve/    CI/CD  │
│    Submits      the "Why"       Changes    Works     Request     Runs   │
│                                                      Changes            │
└─────────────────────────────────────────────────────────────────────────┘

The 7-Step PR Review Framework

Quick Reference Table

Step	Focus Area	Key Questions
1. Context	Purpose & Background	Why is this change needed?
2. Correctness	Technical Accuracy	Does it work correctly?
3. Completeness	Full Implementation	Are all affected areas updated?
4. Compatibility	Breaking Changes	Does it break existing code?
5. Consistency	Code Style	Does it follow conventions?
6. Clarity	Readability	Is it understandable?
7. Considerations	Edge Cases	What about performance, security?

Step 1: Context Analysis 📋

Goal: Understand the purpose before reading code.

Checklist

Read the PR title and description thoroughly
Understand the problem being solved
Review linked issues, tickets, or documentation
Check if this is a bug fix, feature, refactor, or dependency update
Verify the scope is appropriate (not too large, not too small)

Questions to Ask

✓ What problem does this PR solve?
✓ Why was this approach chosen over alternatives?
✓ Is there sufficient context for reviewers?
✓ Does the PR description explain the "why"?

Red Flags 🚩

PR with no description
Very large PRs (500+ lines without justification)
Scope creep (mixing unrelated changes)
Missing ticket/issue reference

Example: Good PR Description

## Summary
Add NDK version configuration for Android 15 compatibility

## Problem
Android 15 requires 16KB page size alignment for native libraries.
Without this change, the app will crash on Android 15+ devices.

## Solution
- Set NDK version to 29.0.14206865 in app/build.gradle.kts
- Add linker option for 16KB page alignment in CMakeLists.txt

## Testing
- Tested on Android 15 emulator
- Verified native library loads successfully
- All existing tests pass

## Related
- Fixes #123
- Docs: https://developer.android.com/guide/practices/page-sizes

Step 2: Correctness Analysis ✅

Goal: Verify the implementation is technically correct.

Checklist

Logic is correct and handles expected use cases
Edge cases are handled appropriately
Error handling is present and correct
Null safety is properly managed (no unnecessary !!)
Threading is correct (main vs background thread)
Memory management is proper (no leaks)

Android/Kotlin Specific Checks

// ❌ BAD: Force unwrap can cause crashes
val user = repository.getUser()!!

// ✅ GOOD: Safe handling
val user = repository.getUser() ?: return

// ❌ BAD: Blocking main thread
fun loadData() {
    val data = networkCall() // Blocks UI!
}

// ✅ GOOD: Proper coroutine usage
suspend fun loadData() = withContext(Dispatchers.IO) {
    networkCall()
}

Common Correctness Issues

Issue	Example	Solution
Race conditions	Concurrent state updates	Use `Mutex` or single-threaded dispatcher
Memory leaks	Holding Activity reference	Use `WeakReference` or scoped coroutines
Incorrect scope	`GlobalScope.launch`	Use `viewModelScope` or lifecycle-aware scope
State loss	Data lost on configuration change	Use `ViewModel` + `SavedStateHandle`

Step 3: Completeness Analysis 🧩

Goal: Ensure all necessary changes are included.

Checklist

All affected files are modified
Tests are added/updated for new functionality
Documentation is updated if needed
Migration scripts provided if needed (database changes)
Related modules updated consistently
No TODO comments left without tracking issue

Multi-Module Project Considerations

When changing a core module, check:
├── core/network    → Did API interfaces change?
├── core/database   → Did entities/DAOs change?
├── core/common     → Did shared utilities change?
├── features/*      → Are feature modules updated?
└── app             → Is app module configuration updated?

Example: Incomplete PR

// PR adds new field to API response but forgets:
// ❌ Missing: Database entity update
// ❌ Missing: Room migration
// ❌ Missing: Domain model update
// ❌ Missing: UI layer handling

data class RecipeDto(
    val id: String,
    val name: String,
    val calories: Int  // NEW FIELD - where else needs updating?
)

Step 4: Compatibility Analysis 🔄

Goal: Verify no breaking changes are introduced.

Checklist

Public APIs remain backward compatible
Database schema changes include migrations
Shared preferences changes handle old data
Feature flags used for gradual rollout (if needed)
minSdk/targetSdk implications considered

Breaking Change Categories

Type	Impact	Mitigation
API signature change	Compile-time error	Deprecate old, add new
Database schema change	Runtime crash	Room migration
Behavior change	Logic errors	Feature flag
Dependency update	Various	Test thoroughly

Database Migration Example

// ✅ GOOD: Proper migration
val MIGRATION_1_2 = object : Migration(1, 2) {
    override fun migrate(database: SupportSQLiteDatabase) {
        database.execSQL(
            "ALTER TABLE recipes ADD COLUMN calories INTEGER DEFAULT 0 NOT NULL"
        )
    }
}

// ❌ BAD: Destructive migration
fallbackToDestructiveMigration() // Loses user data!

Step 5: Consistency Analysis 📐

Goal: Ensure code follows project conventions.

Project Conventions Checklist

Naming conventions followed
Package structure maintained
Code formatting applied (ktlint/detekt)
Architectural patterns followed (MVI, Clean Architecture)
Dependency injection patterns consistent

Naming Conventions

// Files & Classes
HomeViewModel.kt        // ViewModel suffix
RecipeRepository.kt     // Interface, no Impl suffix
RecipeRepositoryImpl.kt // Implementation with Impl suffix
GetRecipesUseCase.kt    // UseCase suffix

// Functions
suspend fun fetchRecipes()     // Suspend for coroutines
fun validateEmail()            // Verb for actions
fun isEmailValid(): Boolean    // is/has for boolean returns

// State
data class HomeUiState(...)    // UiState suffix
sealed class HomeEvent(...)    // Event suffix for user actions

Architecture Consistency

// ✅ GOOD: Follows Clean Architecture
class HomeViewModel @Inject constructor(
    private val getRecipesUseCase: GetRecipesUseCase  // UseCase, not Repository
) : ViewModel()

// ❌ BAD: Bypasses domain layer
class HomeViewModel @Inject constructor(
    private val recipeRepository: RecipeRepository,   // Direct repo access
    private val recipeApi: RecipeApi                  // Direct API access!
) : ViewModel()

Step 6: Clarity Analysis 📖

Goal: Code should be self-documenting and easy to understand.

Checklist

Variable/function names are descriptive
Complex logic has explanatory comments
Public APIs have KDoc documentation
Magic numbers are replaced with constants
Code intent is clear without reading implementation

Documentation Standards

/**
 * Validates user credentials and initiates login flow.
 *
 * @param email User's email address
 * @param password User's password (plain text, will be hashed)
 * @return [Result] containing [LoginResult] on success or [AuthError] on failure
 * @throws NetworkException if network is unavailable
 *
 * @see LoginResult
 * @see AuthError
 */
suspend fun login(email: String, password: String): Result<LoginResult>

Self-Documenting Code

// ❌ BAD: Magic numbers and unclear intent
if (password.length >= 8 && password.any { it.isDigit() }) { ... }

// ✅ GOOD: Clear intent through naming
private const val MIN_PASSWORD_LENGTH = 8

fun isPasswordStrong(password: String): Boolean {
    val hasMinimumLength = password.length >= MIN_PASSWORD_LENGTH
    val containsDigit = password.any { it.isDigit() }
    val containsUppercase = password.any { it.isUpperCase() }
    
    return hasMinimumLength && containsDigit && containsUppercase
}

Step 7: Considerations Analysis 💭

Goal: Think about non-functional requirements and edge cases.

Performance Checklist

No unnecessary object allocations in loops
Database queries are optimized (indexed columns)
Images are properly sized/cached
Network calls are batched when possible
Compose recomposition is minimized

Security Checklist

Sensitive data not logged
API keys not hardcoded
User input is validated/sanitized
HTTPS enforced for network calls
Proper authentication token handling

Compose Performance

// ❌ BAD: Creates new lambda every recomposition
@Composable
fun RecipeList(recipes: List<Recipe>) {
    LazyColumn {
        items(recipes) { recipe ->
            RecipeItem(
                recipe = recipe,
                onClick = { viewModel.onRecipeClick(recipe.id) }  // New lambda!
            )
        }
    }
}

// ✅ GOOD: Stable reference
@Composable
fun RecipeList(
    recipes: List<Recipe>,
    onRecipeClick: (String) -> Unit  // Hoisted
) {
    LazyColumn {
        items(
            items = recipes,
            key = { it.id }  // Stable key!
        ) { recipe ->
            RecipeItem(
                recipe = recipe,
                onClick = { onRecipeClick(recipe.id) }
            )
        }
    }
}

Providing Feedback

The Right Tone

✅ DO:
- "Consider using X because..."
- "Have you thought about...?"
- "This works, but we could improve it by..."
- "Great solution! One small suggestion..."

❌ DON'T:
- "This is wrong."
- "Why didn't you do X?"
- "This doesn't make sense."
- "I would never do it this way."

Feedback Categories

Use prefixes to indicate severity:

Prefix	Meaning	Required?
`[Blocker]`	Must fix before merge	Yes
`[Major]`	Should fix, significant issue	Usually
`[Minor]`	Nice to fix, small improvement	No
`[Nit]`	Nitpick, personal preference	No
`[Question]`	Seeking clarification	N/A
`[Praise]`	Calling out good work!	N/A

Example Review Comments

[Blocker] This will cause a crash on null input:
`val user = users.first()` should be `users.firstOrNull()`

[Major] Consider using `remember { }` here to avoid creating 
a new object on every recomposition.

[Minor] Could extract this to a constant for clarity:
`const val MAX_RETRY_COUNT = 3`

[Nit] Personal preference, but I find `when` more readable 
than chained `if-else` here.

[Question] Why is this using `runBlocking` instead of 
`viewModelScope.launch`?

[Praise] Really clean solution! Love how you decomposed 
this into smaller functions. 👏

PR Review Checklist Template

Copy this template for your reviews:

## PR Review: [PR Title]

### Context ✓
- [ ] Purpose is clear
- [ ] Scope is appropriate
- [ ] Description is sufficient

### Correctness ✓
- [ ] Logic is correct
- [ ] Edge cases handled
- [ ] Error handling present
- [ ] Thread safety correct

### Completeness ✓
- [ ] All files updated
- [ ] Tests included
- [ ] Documentation updated
- [ ] Migrations provided (if needed)

### Compatibility ✓
- [ ] No breaking changes
- [ ] Backward compatible
- [ ] Database migrations included

### Consistency ✓
- [ ] Naming conventions
- [ ] Architecture patterns
- [ ] Code formatting

### Clarity ✓
- [ ] Self-documenting code
- [ ] Comments for complexity
- [ ] Public API documented

### Considerations ✓
- [ ] Performance OK
- [ ] Security OK
- [ ] Accessibility OK

### Verdict
- [ ] ✅ Approved
- [ ] 🔄 Request Changes
- [ ] 💬 Comment Only

Android-Specific Review Points

Gradle/Build Configuration

// Version Management: Check if using version catalog
// ❌ Hardcoded version
implementation("com.squareup.retrofit2:retrofit:2.9.0")

// ✅ Version catalog reference
implementation(libs.retrofit)

// NDK Version: Should be explicit for native modules
android {
    ndkVersion = libs.versions.ndk.get()  // Good
}

Jetpack Compose

Check	Why
`remember` usage	Prevents unnecessary recomposition
`derivedStateOf` for computed values	Efficient state derivation
`collectAsStateWithLifecycle()`	Lifecycle-aware collection
Stable/Immutable annotations	Compose compiler optimizations
LazyColumn keys	Efficient diff/reorder

Hilt/Dependency Injection

// Check scoping is correct
@Singleton           // App-level singleton
@ActivityScoped      // Activity lifecycle
@ViewModelScoped     // ViewModel lifecycle
@ActivityRetainedScoped  // Survives configuration changes

// Check bindings are in correct modules
@Module
@InstallIn(SingletonComponent::class)  // Match scope with @InstallIn
abstract class NetworkModule { ... }

Common PR Anti-Patterns

1. The Monster PR

❌ PR with 50+ files, 2000+ lines
→ Solution: Break into smaller, focused PRs

2. The Sneaky Refactor

❌ Bug fix PR that also refactors unrelated code
→ Solution: Separate refactoring into its own PR

3. The Copy-Paste

❌ Duplicated code instead of abstraction
→ Solution: Create shared utility or component

4. The Missing Tests

❌ New feature with no test coverage
→ Solution: Add unit tests for business logic

5. The Silent Dependency Update

❌ Updating dependencies without changelog review
→ Solution: Document breaking changes and migration

Automated Checks

Before manual review, ensure these pass:

# Linting
./gradlew ktlintCheck

# Static Analysis
./gradlew detekt

# Unit Tests
./gradlew testDebugUnitTest

# Build
./gradlew assembleDebug

CI Pipeline Requirements

Check	Tool	Required
Lint	ktlint	✅
Static Analysis	Detekt	✅
Unit Tests	JUnit	✅
Build	Gradle	✅
Coverage	JaCoCo	Recommended
UI Tests	Espresso/Compose	On critical paths

Review Turnaround Guidelines

PR Size	Expected Review Time
XS (< 50 lines)	Same day
S (50-200 lines)	1 business day
M (200-500 lines)	2 business days
L (500+ lines)	Consider splitting

Summary

Effective code review is a skill that improves with practice. Remember:

Be thorough but efficient — Focus on what matters most
Be kind but honest — Critique code, not people
Be curious — Ask questions to understand intent
Be educational — Share knowledge through reviews
Be responsive — Review PRs promptly

"A good code review is a conversation, not an inspection."

FilesExpand file tree

pr_review_guide.md

Latest commit

History

pr_review_guide.md

File metadata and controls

🔍 Pull Request Review Guide

Overview

The 7-Step PR Review Framework

Quick Reference Table

Step 1: Context Analysis 📋

Checklist

Questions to Ask

Red Flags 🚩

Example: Good PR Description

Step 2: Correctness Analysis ✅

Checklist

Android/Kotlin Specific Checks

Common Correctness Issues

Step 3: Completeness Analysis 🧩

Checklist

Multi-Module Project Considerations

Example: Incomplete PR

Step 4: Compatibility Analysis 🔄

Checklist

Breaking Change Categories

Database Migration Example

Step 5: Consistency Analysis 📐

Project Conventions Checklist

Naming Conventions

Architecture Consistency

Step 6: Clarity Analysis 📖

Checklist

Documentation Standards

Self-Documenting Code

Step 7: Considerations Analysis 💭

Performance Checklist

Security Checklist

Compose Performance

Providing Feedback

The Right Tone

Feedback Categories

Example Review Comments

PR Review Checklist Template

Android-Specific Review Points

Gradle/Build Configuration

Jetpack Compose

Hilt/Dependency Injection

Common PR Anti-Patterns

1. The Monster PR

2. The Sneaky Refactor

3. The Copy-Paste

4. The Missing Tests

5. The Silent Dependency Update

Automated Checks

CI Pipeline Requirements

Review Turnaround Guidelines

Summary

Related Documentation