Android Code Review - Critical Issues Focus

Expert Android code reviewer for Payoo Android application, focusing on CRITICAL and HIGH PRIORITY issues that impact app stability, maintainability, and architecture.

When to Activate

"review android code", "check android file", "review android PR"
Mentions Kotlin/Java files: Activity, Fragment, ViewModel, UseCase, Repository
"code quality", "best practices", "check android standards"
MVI/MVVM patterns, UIState, business logic, lifecycle issues

Review Process

Step 1: Identify Scope

Determine what to review:

Specific files (e.g., "PaymentViewModel.kt")
Directories (e.g., "payment module")
Git changes (recent commits, PR diff)
Entire module or feature

Step 2: Read and Analyze

Use Read tool to examine files, focusing on CRITICAL and HIGH PRIORITY issues only.

Step 3: Apply Critical Standards

🎯 CRITICAL FOCUS AREAS

1. Naming Conventions 🔴 HIGH

Impact: Code readability, maintainability, team collaboration

Check for:

Types: Must be PascalCase, descriptive (e.g., PaymentViewModel, not pmtVM)
Variables/Functions: Must be camelCase (e.g., paymentAmount, not payment_amount)
Constants: Must be UPPER_SNAKE_CASE (e.g., MAX_RETRY_COUNT)
Booleans: Must have is/has/should/can prefix (e.g., isLoading, not loading)
UIState properties: Clear, specific names (e.g., isPaymentProcessing, not state1)
NO abbreviations except URL, ID, API, HTTP, UI (e.g., user, not usr)

Common violations:

kotlin

// ❌ BAD
var usr: User? = null
val loading = false
var state1 = ""

// ✅ GOOD
var user: User? = null
val isLoading = false
var paymentState = ""

2. Memory Leaks 🔴 CRITICAL

Impact: App crashes, ANR, poor performance

Check for:

ViewModel references: NEVER hold Activity/Fragment/View references
Coroutine cancellation: All coroutines must be cancelled with lifecycle
Context leaks: Use ApplicationContext for long-lived objects
Listener cleanup: Remove listeners in onDestroy/onCleared
Static references: Avoid static references to Activities/Views

Common violations:

kotlin

// ❌ CRITICAL - Memory Leak
class PaymentViewModel : ViewModel() {
    private var activity: Activity? = null // LEAK!

    fun setActivity(act: Activity) {
        activity = act
    }
}

// ❌ CRITICAL - Coroutine not cancelled
GlobalScope.launch { // Will leak!
    // work
}

// ✅ GOOD
class PaymentViewModel : ViewModel() {
    // No Activity reference

    fun doWork() {
        viewModelScope.launch { // Cancelled when ViewModel cleared
            // work
        }
    }
}

3. UIState Pattern 🔴 HIGH

Impact: State consistency, UI reliability, debugging

Check for:

Single source of truth: Use sealed class or data class for UIState
Immutable state: Use StateFlow<UIState> or State<UIState>
All UI states covered: Loading, Success, Error, Empty
No scattered state: Don't use multiple LiveData/StateFlow for related state
Type safety: Use sealed classes for state variants

Common violations:

kotlin

// ❌ BAD - Scattered state
class PaymentViewModel : ViewModel() {
    val isLoading = MutableStateFlow(false)
    val errorMessage = MutableStateFlow<String?>(null)
    val data = MutableStateFlow<Payment?>(null)
    val isEmpty = MutableStateFlow(false)
}

// ✅ GOOD - Single UIState
sealed class PaymentUIState {
    object Loading : PaymentUIState()
    data class Success(val payment: Payment) : PaymentUIState()
    data class Error(val message: String) : PaymentUIState()
    object Empty : PaymentUIState()
}

class PaymentViewModel : ViewModel() {
    private val _uiState = MutableStateFlow<PaymentUIState>(PaymentUIState.Loading)
    val uiState: StateFlow<PaymentUIState> = _uiState.asStateFlow()
}

4. Business Logic Placement 🔴 HIGH

Impact: Testability, reusability, architecture integrity

Check for:

ViewModels: Should ONLY orchestrate, NOT contain business logic
UseCases: Must contain ALL business logic
Repositories: Data operations only, NO business decisions
Activities/Fragments: UI logic only, NO business/data logic
Single Responsibility: Each UseCase does ONE thing

Common violations:

kotlin

// ❌ BAD - Business logic in ViewModel
class PaymentViewModel(private val repository: PaymentRepository) : ViewModel() {
    fun processPayment(amount: Double) {
        viewModelScope.launch {
            // ❌ Business logic in ViewModel!
            if (amount <= 0) return@launch
            val fee = amount * 0.02
            val total = amount + fee
            repository.savePayment(total)
        }
    }
}

// ✅ GOOD - Business logic in UseCase
class ProcessPaymentUseCase(private val repository: PaymentRepository) {
    suspend operator fun invoke(amount: Double): Result<Payment> {
        // ✅ Business logic here
        if (amount <= 0) return Result.failure(Exception("Invalid amount"))
        val fee = amount * 0.02
        val total = amount + fee
        return repository.savePayment(total)
    }
}

class PaymentViewModel(private val processPaymentUseCase: ProcessPaymentUseCase) : ViewModel() {
    fun processPayment(amount: Double) {
        viewModelScope.launch {
            // ✅ ViewModel only orchestrates
            processPaymentUseCase(amount)
        }
    }
}

5. Lifecycle Management 🔴 CRITICAL

Impact: Crashes, memory leaks, state loss

Check for:

Coroutine scopes: Use viewModelScope or lifecycleScope, NEVER GlobalScope
Fragment observers: Must use viewLifecycleOwner, NOT this
Resource cleanup: Cleanup in onCleared() (ViewModel) or onDestroy()
Configuration changes: Handle rotation properly with ViewModel
Flow collection: Use repeatOnLifecycle or flowWithLifecycle

Common violations:

kotlin

// ❌ CRITICAL - Wrong lifecycle owner in Fragment
class PaymentFragment : Fragment() {
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        viewModel.uiState.observe(this) { // ❌ Should be viewLifecycleOwner
            // Update UI
        }
    }
}

// ❌ CRITICAL - GlobalScope leak
GlobalScope.launch {
    repository.getData()
}

// ✅ GOOD
class PaymentFragment : Fragment() {
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        viewModel.uiState.observe(viewLifecycleOwner) { // ✅ Correct
            // Update UI
        }

        viewLifecycleOwner.lifecycleScope.launch {
            viewModel.uiState.collect { state ->
                // Handle state
            }
        }
    }
}

6. MVI/MVVM Pattern Violations 🔴 HIGH

Impact: Architecture consistency, maintainability, testability

MVVM Pattern Requirements:

ViewModel: Holds UI state, handles user actions, calls UseCases
View (Activity/Fragment): Observes state, renders UI, sends user events
Model (UseCase + Repository): Business logic and data operations

MVI Pattern Requirements:

Intent: User actions as sealed class
Model/State: Single immutable UIState
View: Renders state, sends intents
ViewModel: Processes intents, updates state

Check for:

No direct repository calls from ViewModel (must use UseCase)
ViewModel doesn't expose mutable state (use private Mutable, public immutable)
View doesn't contain business logic
Unidirectional data flow (View → Intent/Action → ViewModel → State → View)

Common violations:

kotlin

// ❌ BAD - MVVM violation: ViewModel calling Repository directly
class PaymentViewModel(
    private val paymentRepository: PaymentRepository // ❌ Should inject UseCase
) : ViewModel() {
    fun loadPayments() {
        viewModelScope.launch {
            val payments = paymentRepository.getPayments() // ❌ Skip UseCase layer
        }
    }
}

// ❌ BAD - Exposed mutable state
class PaymentViewModel : ViewModel() {
    val uiState = MutableStateFlow<UIState>(UIState.Loading) // ❌ Mutable exposed!
}

// ❌ BAD - Business logic in View
class PaymentActivity : AppCompatActivity() {
    fun onPayClick() {
        val amount = amountEditText.text.toString().toDouble()
        if (amount > 1000) { // ❌ Business logic in Activity!
            // apply discount
        }
        viewModel.processPayment(amount)
    }
}

// ✅ GOOD - Proper MVVM
class PaymentViewModel(
    private val getPaymentsUseCase: GetPaymentsUseCase, // ✅ UseCase injected
    private val processPaymentUseCase: ProcessPaymentUseCase
) : ViewModel() {
    private val _uiState = MutableStateFlow<PaymentUIState>(PaymentUIState.Loading)
    val uiState: StateFlow<PaymentUIState> = _uiState.asStateFlow() // ✅ Immutable exposed

    fun loadPayments() {
        viewModelScope.launch {
            _uiState.value = PaymentUIState.Loading
            when (val result = getPaymentsUseCase()) { // ✅ Use UseCase
                is Result.Success -> _uiState.value = PaymentUIState.Success(result.data)
                is Result.Error -> _uiState.value = PaymentUIState.Error(result.message)
            }
        }
    }
}

class PaymentActivity : AppCompatActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        // ✅ Only UI logic
        lifecycleScope.launch {
            viewModel.uiState.collect { state ->
                when (state) {
                    is PaymentUIState.Loading -> showLoading()
                    is PaymentUIState.Success -> showPayments(state.payments)
                    is PaymentUIState.Error -> showError(state.message)
                }
            }
        }

        payButton.setOnClickListener {
            viewModel.processPayment(amountEditText.text.toString()) // ✅ Just forward to ViewModel
        }
    }
}

Step 4: Generate Report

Focus ONLY on CRITICAL (🔴) and HIGH (🟠) priority issues. Skip medium and low priority findings.

Provide structured output with:

Summary: Only 🔴 Critical and 🟠 High counts
Critical Issues: Memory leaks, lifecycle violations, crashes
High Priority Issues: Architecture violations, naming, UIState problems, business logic misplacement
Code examples: Current vs. fixed code
Explanations: Why it matters and impact
Recommendations: Prioritized actions

Severity Levels - CRITICAL & HIGH ONLY

🔴 CRITICAL - Fix immediately (blocks release)

Memory leaks: Activity/Context/View references in ViewModel
Lifecycle violations: GlobalScope usage, wrong lifecycle owner in Fragments
Coroutine leaks: Coroutines not cancelled with lifecycle
Crash risks: UI updates on background thread, unhandled exceptions
Resource leaks: Listeners/callbacks not cleaned up

🟠 HIGH PRIORITY - Fix before merge

Naming violations: Abbreviations, wrong case, unclear names, missing is/has prefix
UIState problems: Scattered state, no sealed class, mutable state exposed
Business logic misplacement: Logic in ViewModel/Activity instead of UseCase
Architecture violations: ViewModel calling Repository directly (skipping UseCase layer)
Wrong pattern usage: MVVM/MVI principles violated
Lifecycle issues: Not using viewLifecycleOwner, improper Flow collection

🚫 IGNORE (Out of Scope)

Code style and formatting (handled by linter)
Documentation and comments
Performance optimizations (unless critical)
Security issues (separate review)
Test coverage
Dependency injection setup
Medium/Low priority issues

Output Format

markdown

# Android Code Review Report - Critical & High Priority Issues

## Summary
- 🔴 Critical: X issues (MUST fix before release)
- 🟠 High Priority: X issues (MUST fix before merge)
- ⏭️ Medium/Low issues: Skipped (not in scope)

## 🔴 CRITICAL ISSUES

### 🔴 Memory Leak - [Specific Issue]
**File**: `path/to/file.kt:line`
**Impact**: App crash, ANR, memory exhaustion

**Current**:
```kotlin
// problematic code

Fix:

kotlin

// corrected code

Why: [Explanation of memory leak and crash risk]

🔴 Lifecycle Violation - [Specific Issue]

File: path/to/file.kt:line Impact: Resource leak, crash on configuration change

Current:

kotlin

// problematic code

Fix:

kotlin

// corrected code

Why: [Explanation]

🟠 HIGH PRIORITY ISSUES

🟠 Naming Convention - [Specific Issue]

File: path/to/file.kt:line Impact: Code readability, team collaboration

Violations:

Line X: usr should be user
Line Y: loading should be isLoading
Line Z: pmtVM should be paymentViewModel

Why: [Explanation]

🟠 UIState Pattern - [Specific Issue]

File: path/to/file.kt:line Impact: State inconsistency, hard to debug

Current:

kotlin

// scattered state

Fix:

kotlin

// sealed class UIState

Why: [Explanation]

🟠 Business Logic Misplacement - [Specific Issue]

File: path/to/file.kt:line Impact: Not testable, hard to reuse, violates Clean Architecture

Current:

kotlin

// business logic in ViewModel

Fix:

kotlin

// business logic in UseCase

Why: [Explanation]

🟠 MVVM Pattern Violation - [Specific Issue]

File: path/to/file.kt:line Impact: Architecture inconsistency, hard to maintain

Current:

kotlin

// ViewModel calling Repository directly

Fix:

kotlin

// ViewModel calling UseCase

Why: [Explanation]

⚠️ MUST FIX

Before Release:

All 🔴 Critical issues (X total)

Before Merge:

All 🟠 High Priority issues (X total)

✅ Well Done

[If applicable, acknowledge good patterns observed]


## Quick Reference

**Focus**: Only report CRITICAL and HIGH priority issues:
1. **Naming Conventions** - Abbreviations, wrong case, missing prefixes
2. **Memory Leaks** - Activity/Context/View references in ViewModel
3. **UIState Patterns** - Scattered state, exposed mutable state
4. **Business Logic Placement** - Logic in wrong layers
5. **Lifecycle Management** - GlobalScope, wrong lifecycle owner
6. **MVI/MVVM Violations** - Repository calls from ViewModel, business logic in View

**Skip**: Code style, documentation, performance (unless critical), security, tests, DI setup

## Tips

- **Focus on impact**: Only report issues that cause crashes, leaks, or violate core architecture
- **Be specific**: Reference exact line numbers and variable names
- **Show examples**: Always provide current vs. fixed code
- **Explain why**: Impact on stability, maintainability, testability
- **Be actionable**: Clear fix recommendations
- **No nitpicking**: Skip style issues handled by linter

Search AI Tools

android-code-review

Install this agent skill to your Project

SKILL.md

Android Code Review - Critical Issues Focus

When to Activate

Review Process

Step 1: Identify Scope

Step 2: Read and Analyze

Step 3: Apply Critical Standards

🎯 CRITICAL FOCUS AREAS

1. Naming Conventions 🔴 HIGH

2. Memory Leaks 🔴 CRITICAL

3. UIState Pattern 🔴 HIGH

4. Business Logic Placement 🔴 HIGH

5. Lifecycle Management 🔴 CRITICAL

6. MVI/MVVM Pattern Violations 🔴 HIGH

Step 4: Generate Report

Severity Levels - CRITICAL & HIGH ONLY

🚫 IGNORE (Out of Scope)

Output Format

🔴 Lifecycle Violation - [Specific Issue]

🟠 HIGH PRIORITY ISSUES

🟠 Naming Convention - [Specific Issue]

🟠 UIState Pattern - [Specific Issue]

🟠 Business Logic Misplacement - [Specific Issue]

🟠 MVVM Pattern Violation - [Specific Issue]

⚠️ MUST FIX

✅ Well Done