jordancoin/ios-skills-collection/arimunandar-code-review-checklist

✅ 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

// ✅ 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
}

// ❌ RETAIN CYCLE
button.onTap = {
    self.handleTap()  // Strong capture
}
 
// ✅ FIXED
button.onTap = { [weak self] in
    self?.handleTap()
}

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

// ❌ 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
}

// ✅ 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
    }
}

// ❌ 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)
    }
}

// ✅ 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)
    }
}

// ❌ 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"

// ❌ AVOID (except IBOutlets)
var viewModel: ViewModel!
 
// ✅ PREFER
var viewModel: ViewModel?  // Or make non-optional with proper init

// ✅ 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

// ❌ 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()

// ❌ 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)

// ❌ 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")

// ✅ 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)

// ❌ 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

// ✅ GOOD TEST - Clear naming, single assertion
func test_authenticate_withInvalidPassword_shouldReturnError() {
    // Arrange
    let request = Login.Request(email: "test@test.com", 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)
}

🔴 **Change Required**
 
[Description of issue]
 
**Current:**

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

🟡 **Suggestion**
 
Consider [suggestion] because [reason].
 
This would [benefit].

🔵 **Question**
 
Could you explain [specific part]?
 
I'm wondering about [concern].

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