Arimunandar code review checklist

Code Review Checklist

Comprehensive checklist for reviewing iOS/Swift code. Use this systematically to ensure thorough reviews.

Quick Review Checklist

Must Check (Every PR)

• Compiles - No build errors or warnings
• Tests pass - All existing tests green
• No crashes - No force unwraps in new code (!)
• Memory safe - Weak references where needed
• Thread safe - UI updates on main thread

Should Check

• Architecture - Follows VIP+W pattern
• Naming - Clear, descriptive names
• Tests - New code has tests
• Documentation - Complex logic explained

---

Detailed Review Categories

1. Architecture Compliance

VIP+W Pattern

✅ CORRECT                          ❌ INCORRECT
─────────────────────────────────────────────────────────────
ViewController only displays        ViewController has business logic
Interactor orchestrates logic       Interactor makes API calls directly
Worker handles async operations     Worker knows about Presenter
Presenter formats for display       Presenter has business logic
Router handles navigation           Navigation in ViewController

Review Questions:

• Does ViewController only call interactor?.methodName(request:)?
• Does Interactor delegate async work to Worker?
• Does Presenter only format data, no business logic?
• Is navigation handled by Router only?
• Are all dependencies injected via Configurator?

Protocol Usage

// ✅ GOOD - Protocol-based dependency
protocol UserServiceProtocol {
    func fetchUser(id: String) async throws -> User
}

class UserInteractor {
    private let service: UserServiceProtocol  // Abstraction

    init(service: UserServiceProtocol) {
        self.service = service
    }
}

// ❌ BAD - Concrete dependency
class UserInteractor {
    private let service = UserService()  // Concrete type
}

Checklist:

• Dependencies injected, not created internally
• Protocols defined for all dependencies
• No singletons accessed directly (inject instead)

---

2. Memory Management

Retain Cycles

// ❌ RETAIN CYCLE
button.onTap = {
    self.handleTap()  // Strong capture
}

// ✅ FIXED
button.onTap = { [weak self] in
    self?.handleTap()
}

Checklist:

• Closures use [weak self] or [unowned self]
• Delegates are weak
• Timers are invalidated in deinit
• NotificationCenter observers removed
• No circular references between objects

Memory Leaks Detection

// Add to ViewControllers during development
deinit {
    print("✅ \(Self.self) deallocated")
}

Review for:

• ViewControllers have deinit logging (development)
• Closures stored as properties use weak capture
• Parent-child relationships use weak for child→parent

---

3. Concurrency & Thread Safety

Main Thread UI Updates

// ❌ WRONG - UI update from background
Task {
    let data = await fetchData()
    label.text = data.title  // Crash risk!
}

// ✅ CORRECT
Task {
    let data = await fetchData()
    await MainActor.run {
        label.text = data.title
    }
}

// ✅ ALSO CORRECT
@MainActor
func updateUI(with data: Data) {
    label.text = data.title
}

Checklist:

• UI updates on main thread/MainActor
• Heavy work on background thread
• No data races (use actors or locks)
• Proper async/await usage

Actor Isolation

// ✅ GOOD - Actor for shared mutable state
actor UserCache {
    private var cache: [String: User] = [:]

    func get(_ id: String) -> User? {
        cache[id]
    }

    func set(_ user: User) {
        cache[user.id] = user
    }
}

---

4. Error Handling

Proper Error Propagation

// ❌ BAD - Silent failure
func loadData() {
    do {
        let data = try fetchData()
        process(data)
    } catch {
        // Silent failure!
    }
}

// ❌ BAD - Force try
func loadData() {
    let data = try! fetchData()  // Crash if error
}

// ✅ GOOD - Proper handling
func loadData() {
    do {
        let data = try fetchData()
        process(data)
    } catch {
        presenter?.presentError(error)
    }
}

Checklist:

• No empty catch blocks
• No try! or try? without justification
• Errors propagated to user appropriately
• Network errors handled gracefully
• Offline state handled

Result Type Usage

// ✅ GOOD - Explicit success/failure
func authenticate() async -> Result<User, AuthError> {
    do {
        let user = try await worker.login()
        return .success(user)
    } catch let error as AuthError {
        return .failure(error)
    } catch {
        return .failure(.unknown)
    }
}

---

5. Optionals & Safety

Force Unwrapping

// ❌ DANGEROUS
let name = user.name!
let cell = tableView.dequeueReusableCell(withIdentifier: "Cell")!

// ✅ SAFE
guard let name = user.name else { return }

guard let cell = tableView.dequeueReusableCell(withIdentifier: "Cell") as? MyCell else {
    fatalError("Cell not registered") // Explicit failure with message
}

// ✅ ALSO GOOD - Nil coalescing
let name = user.name ?? "Unknown"

Checklist:

• No ! force unwraps (except IBOutlets)
• Guard statements for early returns
• Optional chaining used appropriately
• Nil coalescing for defaults

Implicitly Unwrapped Optionals

// ❌ AVOID (except IBOutlets)
var viewModel: ViewModel!

// ✅ PREFER
var viewModel: ViewModel?  // Or make non-optional with proper init

---

6. Naming Conventions

Swift Naming Guidelines

// ✅ GOOD NAMES
func fetchUser(byID id: String) -> User?
var isLoading: Bool
let maximumRetryCount = 3
class UserProfileViewController
protocol UserServiceProtocol

// ❌ BAD NAMES
func getUser(_ id: String) -> User?  // Missing label
var loading: Bool                      // Not a question
let MAX_RETRY = 3                      // Not camelCase
class UserProfileVC                    // Abbreviated
protocol UserServiceDelegate           // Wrong suffix for non-delegate

Checklist:

• Methods describe action (verbs): fetchUser, updateProfile
• Booleans read as questions: isLoading, hasError, canSubmit
• Constants in camelCase: maximumCount, not MAX_COUNT
• Types in PascalCase: UserProfile
• Protocols: Protocol suffix for abstractions, Delegate for delegates

VIP+W Naming

Component	Naming Pattern
ViewController	LoginViewController
Interactor	LoginInteractor
Presenter	LoginPresenter
Worker	LoginWorker
Router	LoginRouter
Models	Login.Request, Login.Response, Login.ViewModel
Protocols	LoginDisplayLogic, LoginBusinessLogic

---

7. Performance

Collection Operations

// ❌ SLOW - Multiple passes
let filtered = users.filter { $0.isActive }
let mapped = filtered.map { $0.name }
let sorted = mapped.sorted()

// ✅ FAST - Single pass with lazy
let result = users.lazy
    .filter { $0.isActive }
    .map { $0.name }
    .sorted()

Checklist:

• Large collections use lazy for chained operations
• first(where:) instead of filter().first
• contains(where:) instead of filter().isEmpty == false
• Avoid repeated array lookups in loops

Image Handling

// ❌ BAD - Load full image for thumbnail
imageView.image = UIImage(named: "large_photo")

// ✅ GOOD - Downscale to display size
imageView.image = UIImage(named: "large_photo")?.downscaled(to: imageView.bounds.size)

Checklist:

• Images downscaled to display size
• Images loaded asynchronously
• Image caching implemented
• Large images not held in memory

---

8. Security

Sensitive Data

// ❌ BAD - Hardcoded secrets
let apiKey = "sk_live_abc123"

// ✅ GOOD - From secure storage
let apiKey = KeychainService.shared.get("api_key")

// ❌ BAD - Logging sensitive data
print("User token: \(token)")

// ✅ GOOD - Masked logging
print("User authenticated successfully")

Checklist:

• No hardcoded API keys or secrets
• Sensitive data stored in Keychain
• No sensitive data in logs
• HTTPS for all network requests
• User input sanitized

Data Protection

// ✅ GOOD - Secure file storage
let fileURL = try FileManager.default.url(
    for: .applicationSupportDirectory,
    in: .userDomainMask,
    appropriateFor: nil,
    create: true
)
try data.write(to: fileURL, options: .completeFileProtection)

---

9. Code Quality

SOLID Principles

Principle	Check
Single Responsibility	Does this class/function do ONE thing?
Open/Closed	Can behavior be extended without modification?
Liskov Substitution	Can subclasses replace base classes?
Interface Segregation	Are protocols focused and minimal?
Dependency Inversion	Are we depending on abstractions?

Code Smells

• No methods > 30 lines
• No classes > 300 lines
• No more than 3 parameters per function
• No nested conditionals > 2 levels deep
• No duplicate code blocks

Magic Numbers

// ❌ BAD
if retryCount > 3 { ... }
view.layer.cornerRadius = 8

// ✅ GOOD
private let maximumRetryCount = 3
private enum Layout {
    static let cornerRadius: CGFloat = 8
}

if retryCount > maximumRetryCount { ... }
view.layer.cornerRadius = Layout.cornerRadius

---

10. Testing

Test Coverage

• New public methods have tests
• Edge cases covered (empty, nil, error)
• Interactor logic tested
• Presenter formatting tested
• Worker async operations tested

Test Quality

// ✅ GOOD TEST - Clear naming, single assertion
func test_authenticate_withInvalidPassword_shouldReturnError() {
    // Arrange
    let request = Login.Request(email: "[email protected]", password: "wrong")
    mockWorker.result = .failure(.invalidCredentials)

    // Act
    sut.authenticate(request: request)

    // Assert
    XCTAssertTrue(mockPresenter.presentErrorCalled)
}

// ❌ BAD TEST - Multiple assertions, unclear name
func testLogin() {
    sut.authenticate(request: request)
    XCTAssertTrue(mockPresenter.called)
    XCTAssertEqual(mockPresenter.result, expected)
    XCTAssertFalse(mockPresenter.errorCalled)
}

---

Review Comment Templates

Request Changes

🔴 **Change Required**

[Description of issue]

**Current:**
```code```

**Suggested:**
```code```

**Reason:** [Why this change is needed]

Suggestions

🟡 **Suggestion**

Consider [suggestion] because [reason].

This would [benefit].

Questions

🔵 **Question**

Could you explain [specific part]?

I'm wondering about [concern].

Approvals

🟢 **LGTM**

Looks good! Nice work on [specific positive aspect].

---

Review Severity Levels

Level	Icon	Meaning	Action
Blocker	🔴	Must fix before merge	Requires changes
Warning	🟡	Should fix, can discuss	Suggestion
Info	🔵	FYI, question	Discussion
Praise	🟢	Good work	Approval