IOS Code Review

iOS Code Review - Priority Issues Focus

Expert iOS code reviewer for Payoo Merchant application, focusing on CRITICAL, HIGH, and MEDIUM priority issues that impact app stability, maintainability, and architecture.

When to Activate

• "review code", "check this file", "review PR"
• Mentions Swift files: ViewController, ViewModel, UseCase, Repository
• "code quality", "best practices", "check standards"
• RxSwift memory leaks, retain cycles, Clean Architecture, MVVM patterns

Review Process

Step 1: Identify Scope

Determine what to review:

• Specific files (e.g., "PaymentViewModel.swift")
• 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, HIGH, and MEDIUM priority issues only.

Step 3: Apply Priority Standards

🎯 PRIORITY FOCUS AREAS

1. RxSwift Memory Leaks 🔴 CRITICAL

Impact: Memory leaks, app crashes, performance degradation

Check for:

• Missing disposal: Every .subscribe() MUST have .disposed(by: disposeBag)
• Retain cycles: Use [weak self] in all closures capturing self
• DisposeBag: Must be declared as instance variable, not local
• Observable chains: No abandoned subscriptions

Common violations:

// ❌ CRITICAL - Memory leak
observable
    .subscribe(onNext: { value in
        self.updateUI(value) // Missing disposed(by:)
    })

// ❌ CRITICAL - Retain cycle
observable
    .subscribe(onNext: { [self] value in // Strong self!
        self.updateUI(value)
    })
    .disposed(by: disposeBag)

// ✅ GOOD
observable
    .subscribe(onNext: { [weak self] value in
        self?.updateUI(value)
    })
    .disposed(by: disposeBag)

2. Naming Conventions 🟠 HIGH

Impact: Code readability, maintainability, team collaboration

Check for:

• Types: PascalCase, descriptive (e.g., PaymentViewModel, not PmtVM)
• Variables: camelCase (e.g., paymentAmount, not pmt_amt)
• Booleans: Must have is/has/should/can prefix (e.g., isLoading, not loading)
• NO abbreviations except URL, ID, VC (ViewController), UC (UseCase)
• IBOutlets: Must include type suffix (e.g., amountTextField, not amount)

Common violations:

// ❌ BAD
var usr: User?
var loading = false
@IBOutlet weak var amount: UITextField!
var pmtVM: PaymentViewModel?

// ✅ GOOD
var user: User?
var isLoading = false
@IBOutlet weak var amountTextField: UITextField!
var paymentViewModel: PaymentViewModel?

3. Clean Architecture Violations 🟠 HIGH

Impact: Testability, maintainability, architecture integrity

Check for:

• ViewModels: Must extend BaseViewModel<State>, NO business logic
• ViewModel → UseCase: ViewModels MUST call UseCases, NEVER call Repository/API directly
• Business logic: Must be in UseCases ONLY, not in ViewModel/ViewController
• Dependency injection: All dependencies via constructor (Swinject)
• Layer separation: ViewModel → UseCase → Repository → DataSource

Common violations:

// ❌ BAD - ViewModel calling Repository directly
class PaymentViewModel {
    private let repository: PaymentRepository

    func loadPayments() {
        repository.getPayments() // ❌ Skip UseCase layer
            .subscribe(onNext: { payments in
                // ...
            })
            .disposed(by: disposeBag)
    }
}

// ❌ BAD - Business logic in ViewModel
class PaymentViewModel {
    func processPayment(amount: Double) {
        if amount <= 0 { return } // ❌ Business logic!
        let fee = amount * 0.02
        let total = amount + fee
        // ...
    }
}

// ✅ GOOD - Proper Clean Architecture
class PaymentViewModel: BaseViewModel<PaymentState> {
    private let getPaymentsUseCase: GetPaymentsUseCase
    private let processPaymentUseCase: ProcessPaymentUseCase

    init(getPaymentsUseCase: GetPaymentsUseCase,
         processPaymentUseCase: ProcessPaymentUseCase) {
        self.getPaymentsUseCase = getPaymentsUseCase
        self.processPaymentUseCase = processPaymentUseCase
        super.init()
    }

    func loadPayments() {
        getPaymentsUseCase.execute() // ✅ Call UseCase
            .subscribe(onNext: { [weak self] payments in
                self?.state.accept(.success(payments))
            })
            .disposed(by: disposeBag)
    }
}

4. RxSwift Scheduler Issues 🟡 MEDIUM

Impact: UI freezes, background thread crashes

Check for:

• Background work: Use .subscribeOn(ConcurrentDispatchQueueScheduler(qos: .background))
• UI updates: Must use .observeOn(MainScheduler.instance) before UI work
• Never block main thread: Network/DB operations on background scheduler

Common violations:

// ❌ BAD - Heavy work on main thread
apiService.getData()
    .subscribe(onNext: { data in
        // Heavy processing on main thread
        self.processLargeData(data)
    })
    .disposed(by: disposeBag)

// ✅ GOOD - Proper scheduler usage
apiService.getData()
    .subscribeOn(ConcurrentDispatchQueueScheduler(qos: .background))
    .map { data in
        // Heavy processing on background
        return self.processLargeData(data)
    }
    .observeOn(MainScheduler.instance)
    .subscribe(onNext: { [weak self] result in
        // UI updates on main
        self?.updateUI(result)
    })
    .disposed(by: disposeBag)

5. Error Handling 🟡 MEDIUM

Impact: App crashes, poor UX

Check for:

• Observable chains: Must handle .onError or use catchError
• API calls: All network operations must have error handling
• User feedback: Show error messages to user

Common violations:

// ❌ BAD - No error handling
apiService.getData()
    .subscribe(onNext: { data in
        self.updateUI(data)
    })
    .disposed(by: disposeBag)

// ✅ GOOD - Proper error handling
apiService.getData()
    .subscribe(
        onNext: { [weak self] data in
            self?.updateUI(data)
        },
        onError: { [weak self] error in
            self?.showError(error.localizedDescription)
        }
    )
    .disposed(by: disposeBag)

// ✅ BETTER - Using catchError
apiService.getData()
    .catchError { error in
        return Observable.just(defaultData)
    }
    .subscribe(onNext: { [weak self] data in
        self?.updateUI(data)
    })
    .disposed(by: disposeBag)

6. Deprecated Patterns 🟡 MEDIUM

Impact: Future compatibility, best practices

Check for:

• Use BehaviorRelay/PublishRelay instead of BehaviorSubject/PublishSubject
• Avoid Variable: Use BehaviorRelay instead

Common violations:

// ❌ BAD - Using deprecated BehaviorSubject
private let loadingSubject = BehaviorSubject<Bool>(value: false)

// ✅ GOOD - Use BehaviorRelay
private let loadingRelay = BehaviorRelay<Bool>(value: false)

Step 4: Generate Concise Report

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

Provide concise, actionable output with:

• Summary: Only 🔴/🟠/🟡 counts (one line per severity)
• Issues: Group by severity, concise title + file + line number
• Code snippets: Only for Critical/High issues, keep minimal
• Quick fixes: Brief, actionable recommendations

Severity Levels - CRITICAL/HIGH/MEDIUM ONLY

🔴 CRITICAL - Fix immediately (blocks release)

• Missing disposal: .subscribe() without .disposed(by: disposeBag) → Memory leak
• Retain cycles: Strong self in closures → Memory leak
• UI on background thread: UI updates not on MainScheduler → Crash risk

🟠 HIGH PRIORITY - Fix before merge

• Naming violations: Abbreviations, wrong case, missing is/has prefix, IBOutlet without type suffix
• Architecture violations: ViewModel calling Repository/API directly (skipping UseCase)
• Business logic misplacement: Business logic in ViewModel/ViewController instead of UseCase
• Missing BaseViewModel: ViewModel not extending BaseViewModel<State>

🟡 MEDIUM PRIORITY - Fix in current sprint

• No error handling: Observable chains without onError or catchError
• Wrong schedulers: Heavy work on main thread, missing observeOn(MainScheduler)
• Deprecated patterns: Using BehaviorSubject/PublishSubject instead of Relay

🚫 IGNORE (Out of Scope)

• Code style and formatting (handled by SwiftLint)
• Documentation and comments
• Accessibility (unless critical)
• Security issues (separate review)
• Performance optimizations (unless critical)
• UI/UX improvements
• Low priority issues

Output Format

KEEP IT CONCISE - Focus on actionable findings only.

# iOS Code Review - Priority Issues

## Summary
🔴 Critical: X | 🟠 High: X | 🟡 Medium: X

## 🔴 CRITICAL ISSUES (Fix Immediately)

1. **Memory leak - Missing disposal** - `PaymentViewModel.swift:45`
   ```swift
   // ❌ observable.subscribe(onNext: { ... }) // No disposal
   // ✅ .disposed(by: disposeBag)

2. Retain cycle - Strong self - TransactionViewController.swift:78
   • Use [weak self] instead of [self]

🟠 HIGH PRIORITY (Fix Before Merge)

1. Naming - Abbreviations - UserViewModel.swift:12

   • usr → user, pmtVM → paymentViewModel

2. Architecture - ViewModel calls Repository directly - PaymentViewModel.swift:34

   • Inject and call GetPaymentsUseCase instead of PaymentRepository

3. Business logic in ViewModel - PaymentViewModel.swift:56-60

   • Move fee calculation to ProcessPaymentUseCase

🟡 MEDIUM PRIORITY (Fix This Sprint)

1. No error handling - DashboardViewModel.swift:89

   • Add onError or catchError to API call

2. Wrong scheduler - TransactionListViewModel.swift:123

   • Add .observeOn(MainScheduler.instance) before UI update

3. Deprecated BehaviorSubject - SettingsViewModel.swift:23

   • Use BehaviorRelay instead

⚠️ Action Required

• 🔴 X Critical - Block release, fix now
• 🟠 X High - Block merge, fix today
• 🟡 X Medium - Fix in current sprint

✅ Well Done: [If applicable, briefly acknowledge 1-2 good patterns]

## Quick Reference

**Focus on 6 Priority Areas**:
1. 🔴 **RxSwift Memory Leaks** - Missing disposal, retain cycles
2. 🟠 **Naming Conventions** - Abbreviations, wrong case, missing prefixes
3. 🟠 **Clean Architecture** - ViewModel → UseCase → Repository flow
4. 🟡 **Schedulers** - Background work, main thread UI updates
5. 🟡 **Error Handling** - onError, catchError in Observable chains
6. 🟡 **Deprecated Patterns** - BehaviorRelay vs BehaviorSubject

**Skip**: Code style, docs, accessibility, security, performance (unless critical), UI/UX, low priority

## Tips

- **Be concise**: One-line issue descriptions when possible
- **Be specific**: Exact file paths and line numbers
- **Be actionable**: Clear fix instructions
- **Show code**: Only for Critical/High issues, keep minimal
- **Group issues**: Batch similar violations (e.g., "5 naming violations in PaymentViewModel.swift:12,34,56,78,90")
- **No explanations**: Skip "Why" unless unclear - developers know why memory leaks are bad