Skip to Content
Course content

170: Code Review Checklist for Idiomatic Swift

Click on the "Edit" button in the top corner of the screen to edit your slide content.

By the time you're performing code reviews for a professional Swift project, you've likely mastered the syntax. But there is a massive gulf between code that compiles and code that is idiomatic. When I review a PR, I'm not just looking for bugs; I'm looking for "Swiftiness." I want to see if the developer is fighting the language or leaning into it. If I see code that looks like it was written in Java or C++ and then translated literally into Swift, that's where I start leaving comments.

Escaping the Pyramid of Doom

One of the first things I look for is the "Pyramid of Doom"—those deeply nested if let statements that push your actual logic halfway across the screen to the right. It happens because we're trying to be safe with optionals, but it makes the "happy path" of the function harder to find.

// The Naive Way: Deep Nesting
func processUserAvatar(user: User?) {
    if let user = user {
        if let profile = user.profile {
            if let url = profile.avatarURL {
                // Finally, the actual logic
                downloadImage(from: url)
            }
        }
    }
}

When I see this, I'll ask you to flip the logic using guard. The trade-off here is clarity versus verbosity. While guard requires an explicit return or throw, it allows the happy path to stay flush against the left margin. It turns the function into a series of requirements that must be met before the real work begins. I find this much easier to scan during a review because the edge cases are handled upfront and then forgotten.

// The Idiomatic Way: Guard Statements
func processUserAvatar(user: User?) {
    guard let user = user, 
          let profile = user.profile, 
          let url = profile.avatarURL else { 
        return 
    }
    
    downloadImage(from: url)
}

Filtering Noise without the Boilerplate

Another common pattern I see from developers transitioning to Swift is the manual for-in loop used for transformation or filtering. It's not "wrong," and in some complex cases, it's actually more performant, but for 90% of our daily work, it's just noise. It forces the reader to track the state of a mutable temporary array.

// The Naive Way: Imperative Filtering
var activeEmailAddresses: [String] = []
for user in users {
    if let email = user.email, user.isActive {
        activeEmailAddresses.append(email)
    }
}

I prefer using functional chains like compactMap and filter. The beauty of compactMap is that it handles the optional unwrapping and the filtering of nils in one go. By chaining these, you describe what you want to happen rather than how to loop through the memory. It transforms the code from a set of instructions into a declaration of intent.

// The Idiomatic Way: Functional Chaining
let activeEmailAddresses = users
    .filter { $0.isActive }
    .compactMap { $0.email }

Value Semantics over Class Hierarchies

Finally, I keep a sharp eye on the use of class versus struct. A lot of engineers default to classes because that's what they're used to in OOP. They create complex inheritance trees for things that are essentially just data containers. This introduces unnecessary heap allocation and the headache of reference counting (ARC).

If you have a class UserProfile that just holds a name, an age, and an email, you're paying a performance tax for no reason. Unless you specifically need identity (where two objects are the "same" even if their values are identical) or you're working with a framework like UIKit that requires NSObject, use a struct. Value types are safer in multi-threaded environments because you're passing copies, not pointers. When I see a class that doesn't manage a shared state or a complex lifecycle, I'll almost always suggest a struct.




📋 Practical Task

Refactoring the Legacy Order Processor

You have been assigned to review a piece of legacy code in the OrderManager class. The current implementation is functional but is considered "non-idiomatic" Swift. Your task is to rewrite the summarizeOrder function to be more idiomatic.

Current Implementation:

class OrderItem {
    var name: String?
    var price: Double?
    var isTaxable: Bool = false
    
    init(name: String?, price: Double?, isTaxable: Bool) {
        self.name = name
        self.price = price
        self.isTaxable = isTaxable
    }
}

func summarizeOrder(items: [OrderItem]?) {
    if let items = items {
        var taxableTotal: Double = 0
        for item in items {
            if let price = item.price {
                if item.isTaxable {
                    taxableTotal += price
                }
            }
        }
        print("Taxable total is \(taxableTotal)")
    }
}

Your Requirements:

  • Convert the OrderItem from a class to a struct.
  • Replace the nested if let in summarizeOrder with a guard statement.
  • Replace the for-in loop and nested if logic with a functional chain (using filter, compactMap, or reduce).
  • Ensure the final taxableTotal is calculated in a concise, declarative manner.
Rating
0 0

There are no comments for now.

to be the first to leave a comment.