Swift Code Review

Swift Code Review

Review Workflow

Follow this sequence in order. Do not emit findings until every Pass below is satisfied.

1. Swift / toolchain baseline — Establish language and tooling context: Package.swift // swift-tools-version and any per-target Swift language version or swiftSettings in the manifest; for Xcode, SWIFT_VERSION (or equivalent) in project or target build settings; note if review is single-file only. Pass: You state a concrete Swift language version or mode (e.g. Swift 6 language mode, tools 5.10) before advice that depends on strict concurrency, migration-only syntax, or SDK availability.

2. Read surrounding code — For each changed .swift file, read the full enclosing type, function, method, or property that contains the edits, not only the diff hunk. Pass: At least one full enclosing symbol (type or member) containing the change was read per changed file.

3. Scope the checklist — Using Quick Reference, decide which Review Checklist rows and references apply; open those reference files; skip rows clearly unrelated to the diff. Pass: The review (or working notes) lists which checklist areas you applied, or marks areas N/A with a one-line reason tied to the diff (e.g. “no SwiftUI / @Observable in change”).

4. Pre-report verification — Load and follow review-verification-protocol. Pass: That skill’s Hard gates (sequenced) are satisfied for each finding you will report (full symbol read, usage search before “unused”, caller checked before “missing handling”, severity calibrated, [FILE:LINE] proof).

Hard gates (same sequence, shorter)

Output format

Report findings as:

[FILE:LINE] ISSUE_TITLE
Severity: Critical | Major | Minor | Informational
Description of the issue and why it matters.

Quick Reference

Review Checklist

• [ ] No force unwraps (!) on runtime data (network, user input, files)
• [ ] Closures stored as properties use [weak self]
• [ ] Delegate properties are weak
• [ ] Independent async operations use async let or TaskGroup
• [ ] Long-running Tasks check Task.isCancelled
• [ ] Actors have mutable state to protect (no stateless actors)
• [ ] Sendable types are truly thread-safe (beware @unchecked)
• [ ] Errors handled explicitly (no empty catch blocks)
• [ ] Custom errors conform to LocalizedError with descriptive messages
• [ ] Nested @Observable objects are also marked @Observable
• [ ] @Bindable used for two-way bindings to Observable objects

When to Load References

• Reviewing async/await, actors, or TaskGroups → concurrency.md
• Reviewing @Observable or SwiftUI state → observable.md
• Reviewing error handling or throws → error-handling.md
• General Swift review → common-mistakes.md

Review Questions

1. Are async operations that could run concurrently using async let? 2. Could actor state change across suspension points (reentrancy bug)? 3. Is @unchecked Sendable backed by actual synchronization? 4. Are errors logged and presented with helpful context? 5. Could any closure or delegate create a retain cycle?

Swift Common Mistakes

Critical Anti-Patterns

1. Force Unwrapping Runtime Data

// BAD - crashes on invalid input
let url = URL(string: userProvidedString)!
let first = response.items.first!
let value = dictionary["key"]!

// GOOD - safe unwrapping
guard let url = URL(string: userProvidedString) else {
    showError("Invalid URL")
    return
}
let first = response.items.first ?? defaultItem
let value = dictionary["key", default: fallback]

// ACCEPTABLE force unwrap - compile-time verifiable
let url = URL(string: "https://apple.com")!
let image = UIImage(named: "AppIcon")!

2. Retain Cycles in Closures

// BAD - closure captures self strongly
class ViewController {
    var onComplete: (() -> Void)?
    func setup() {
        onComplete = { self.updateUI() }  // Retain cycle!
    }
}

// GOOD - weak capture
onComplete = { [weak self] in
    guard let self else { return }
    self.updateUI()
}

3. Delegate Without Weak

// BAD - strong delegate causes cycle
var delegate: SomeDelegate?

// GOOD - delegates are always weak
weak var delegate: SomeDelegate?

4. Nil Check Then Force Unwrap

// BAD - dangerous pattern
if optionalString != nil {
    print(optionalString!.count)
}

// GOOD - optional binding
if let string = optionalString {
    print(string.count)
}

// Swift 5.7+ shorthand
if let optionalString {
    print(optionalString.count)
}

5. Unnecessary Optionals

// BAD - always has value but declared optional
struct Person {
    let name: String?  // Set in init, never nil
    init(name: String) { self.name = name }
}

// GOOD - non-optional when always present
struct Person {
    let name: String
    init(name: String) { self.name = name }
}

6. Unchecked Array Access

// BAD - crashes if out of bounds
let item = items[index]
let first = items[0]  // Crashes if empty

// GOOD - bounds checking
if items.indices.contains(index) {
    let item = items[index]
}
let first = items.first  // Returns optional

// Safe subscript extension
extension Collection {
    subscript(safe index: Index) -> Element? {
        indices.contains(index) ? self[index] : nil
    }
}

7. Implicitly Unwrapped Optionals

// BAD - IUO for regular properties
class UserProfile {
    var name: String!
    var avatar: UIImage!  // Never set - crash!
}

// GOOD - proper optionals or non-optionals
class UserProfile {
    let name: String
    var avatar: UIImage?  // Truly optional
    init(name: String) { self.name = name }
}

// ACCEPTABLE IUO - @IBOutlet only
@IBOutlet weak var titleLabel: UILabel!

Naming Conventions

// BAD naming
func chkPwd(_ p: String) -> Bool  // Unclear abbreviation
let stringName: String  // Type in name
var enabled: Bool  // Missing is/has prefix
func sort() -> [Int]  // Reads like mutation

// GOOD naming
func checkPassword(_ password: String) -> Bool
let userName: String  // Name by role
var isEnabled: Bool  // Predicate prefix
func sorted() -> [Int]  // Non-mutating returns new value
mutating func sort()  // Mutating is imperative verb

Best Practices Summary

Review Questions

1. Is this force unwrap (!) backed by compile-time certainty? 2. Could this closure stored as a property cause a retain cycle? 3. Are delegate properties marked weak? 4. Is this optional really necessary, or is it always set? 5. Is collection access bounds-checked?

Swift Concurrency

Critical Anti-Patterns

1. Sequential Execution When Concurrent Is Possible

// BAD - sequential awaits on independent operations
let user = await fetchUser()
let avatar = await fetchAvatar(for: user.id)      // Waits unnecessarily
let prefs = await fetchPreferences(for: user.id)  // Waits unnecessarily

// GOOD - async let for independent operations
let user = await fetchUser()
async let avatar = fetchAvatar(for: user.id)
async let prefs = fetchPreferences(for: user.id)
return Profile(user: user, avatar: try await avatar, prefs: try await prefs)

2. Memory Leaks from Task Self-Capture

// BAD - self captured indefinitely in async sequence
Task {
    for await notification in stream {
        handleNotification(notification)  // implicit self
    }
}

// GOOD - weak self with guard inside the loop
Task { [weak self] in
    for await notification in stream {
        guard let self else { return }
        self.handleNotification(notification)
    }
}

3. Actor Reentrancy Bugs

// BAD - state check before await, mutation after
actor BankAccount {
    var balance: Double = 1000
    func withdraw(_ amount: Double) async -> Bool {
        guard balance >= amount else { return false }
        await recordTransaction(amount)  // state can change here!
        balance -= amount  // may go negative
        return true
    }
}

// GOOD - mutate state before await
func withdraw(_ amount: Double) async -> Bool {
    guard balance >= amount else { return false }
    balance -= amount  // safe - done before suspension
    await recordTransaction(amount)
    return true
}

4. Stateless Actors

// BAD - actor with nothing to protect
actor NetworkService {
    func fetchData(from url: URL) async throws -> Data {
        try await URLSession.shared.data(from: url).0
    }
}

// GOOD - use enum or struct for stateless operations
enum NetworkService {
    static func fetchData(from url: URL) async throws -> Data {
        try await URLSession.shared.data(from: url).0
    }
}

5. Ignoring Task Cancellation

// BAD - long loop ignores cancellation
for item in items {
    await process(item)
}

// GOOD - check cancellation
for item in items {
    try Task.checkCancellation()
    await process(item)
}

6. Non-Sendable Types Across Actors

// BAD - mutable class crossing boundaries
class Session { var token: String? }

// GOOD - immutable struct
struct Session: Sendable { let token: String }

// GOOD - @unchecked with actual lock
final class Session: @unchecked Sendable {
    private let lock = NSLock()
    private var _token: String?
    var token: String? {
        get { lock.withLock { _token } }
        set { lock.withLock { _token = newValue } }
    }
}

7. Errors Silently Ignored in Tasks

// BAD - error lost
Task { try await database.save(data) }

// GOOD - handle explicitly
Task {
    do { try await database.save(data) }
    catch { logger.error("Save failed: \(error)") }
}

Best Practices

• Use `async let` for 2-3 independent operations
• Use `TaskGroup` for dynamic number of concurrent tasks with result aggregation
• Apply `@MainActor` at type level for ViewModels, not scattered MainActor.run
• Use `.task` modifier in SwiftUI instead of Task in onAppear
• Use `nonisolated` for pure functions in @MainActor types
• Limit TaskGroup concurrency with iterator pattern for large workloads

Review Questions

1. Are independent async operations running concurrently? 2. Could actor state change across suspension points (reentrancy)? 3. Is @unchecked Sendable backed by actual synchronization? 4. Are long-running Tasks checking Task.isCancelled? 5. Are errors in Task closures being handled or silently lost?

Swift Error Handling

Critical Anti-Patterns

1. Force Try in Production Code

// BAD - crashes if file missing or JSON invalid
let data = try! Data(contentsOf: configURL)
let config = try! JSONDecoder().decode(Config.self, from: data)

// GOOD - handle failures
do {
    let data = try Data(contentsOf: configURL)
    let config = try JSONDecoder().decode(Config.self, from: data)
} catch {
    logger.error("Failed to load config: \(error)")
    return Config.default
}

2. Silencing Errors Without Logging

// BAD - error context lost
let user = try? fetchUser(id: userId)
if user == nil { showError("Something went wrong") }

// GOOD - log the actual error
do {
    let user = try fetchUser(id: userId)
    display(user)
} catch {
    logger.error("Failed to fetch user \(userId): \(error)")
    showError(error.localizedDescription)
}

3. Empty Catch Blocks

// BAD - user thinks save succeeded
do { try saveDocument() } catch { }

// GOOD - inform user of failure
do {
    try saveDocument()
    showSuccess("Document saved")
} catch {
    showError("Failed to save: \(error.localizedDescription)")
}

4. Generic Error Messages

// BAD - cryptic error display
enum NetworkError: Error {
    case requestFailed
}

// GOOD - LocalizedError with descriptions
enum NetworkError: LocalizedError {
    case requestFailed(statusCode: Int)

    var errorDescription: String? {
        switch self {
        case .requestFailed(let code):
            return "Request failed with status \(code)"
        }
    }
}

5. Losing Error Context When Wrapping

// BAD - original error lost
catch { throw ProfileError.loadFailed }

// GOOD - preserve underlying error
enum ProfileError: LocalizedError {
    case networkFailed(underlying: Error)

    var errorDescription: String? {
        switch self {
        case .networkFailed(let error):
            return "Network error: \(error.localizedDescription)"
        }
    }
}

6. Completion Handler Not Called on All Paths

// BAD - completion never called on guard failure
func fetchData(completion: @escaping (Result<Data, Error>) -> Void) {
    guard let url = buildURL() else { return }  // Bug!
    // ...
}

// GOOD - always call completion
guard let url = buildURL() else {
    completion(.failure(NetworkError.invalidURL))
    return
}

try, try?, try! Guidelines

// Acceptable try!
let url = URL(string: "https://api.example.com")!  // Hardcoded, verified

// Never try! with runtime data
let url = URL(string: userInput)!  // CRASH RISK

Swift 6 Typed Throws

// Typed throws - compiler enforces error type
func readFile(at path: String) throws(FileError) -> Data {
    guard fileExists(path) else { throw .notFound }
    // ...
}

// Benefits: self-documenting API, shorthand .case syntax
// Avoid for: public APIs (locks you into error contract)

Result Type vs throws

Review Questions

1. Are all try! usages backed by compile-time certainty? 2. Are errors logged with enough context to diagnose issues? 3. Do custom errors conform to LocalizedError? 4. Are completion handlers called on every code path? 5. Is the underlying error preserved when wrapping?

Swift Observation Framework

Critical Anti-Patterns

1. Incorrect @State Initialization

// BAD - model recreated on view reconstruction
struct ContentView: View {
    @State private var viewModel = ExpensiveViewModel()  // init() runs repeatedly
}

// GOOD - initialize at App level
@main
struct MyApp: App {
    @State private var viewModel = ExpensiveViewModel()
    var body: some Scene {
        WindowGroup { ContentView(viewModel: viewModel) }
    }
}

2. Using @State in Child Views

// BAD - child ignores parent's instance changes
struct ChildView: View {
    @State var model: Model  // Wrong! Preserves first instance

    var body: some View { Text(model.name) }
}

// GOOD - child receives model directly
struct ChildView: View {
    var model: Model  // No wrapper - updates when parent changes

    var body: some View { Text(model.name) }
}

3. Missing @Bindable for Two-Way Bindings

// BAD - cannot create binding
struct EditView: View {
    var user: User
    var body: some View {
        TextField("Name", text: $user.name)  // Error: cannot find '$user'
    }
}

// GOOD - use @Bindable
struct EditView: View {
    @Bindable var user: User
    var body: some View {
        TextField("Name", text: $user.name)  // Works
    }
}

4. Property Wrappers Without @ObservationIgnored

// BAD - property wrapper conflicts with @Observable
@Observable class ViewModel {
    @Injected var repository: Repository  // Error
}

// GOOD - exclude from observation
@Observable class ViewModel {
    @ObservationIgnored
    @Injected var repository: Repository
}

5. Nested Objects Not Observable

// BAD - nested object changes don't trigger updates
@Observable class Store {
    var items: [Item] = []  // Item is regular class
}
class Item { var name: String = "" }

// GOOD - nested types also @Observable
@Observable class Store {
    var items: [Item] = []
}
@Observable class Item { var name: String = "" }

6. Combining @Environment with @Bindable

// BAD - cannot combine property wrappers
struct SettingsView: View {
    @Bindable @Environment(AppSettings.self) var settings  // Error
}

// GOOD - create local @Bindable
struct SettingsView: View {
    @Environment(AppSettings.self) var settings
    var body: some View {
        @Bindable var settings = settings
        Toggle("Dark Mode", isOn: $settings.darkMode)
    }
}

7. withObservationTracking willSet Semantics

// BAD - onChange gets OLD value
withObservationTracking {
    _ = model.name
} onChange: {
    print(model.name)  // Prints old value!
}

// GOOD - dispatch to get new value
withObservationTracking {
    _ = model.name
} onChange: {
    DispatchQueue.main.async {
        print(model.name)  // Now has new value
    }
}

Migration from ObservableObject

When to Use Each Wrapper

Review Questions

1. Is @State only used in the view that creates the object? 2. Are nested observable objects also marked @Observable? 3. Is @Bindable used when two-way bindings are needed? 4. Are property wrappers marked with @ObservationIgnored? 5. Does init() have expensive side effects that could repeat?