The Code

This episode of “Style this” stars code from Brandon Trussell, who asked to “make this Swiftier”:

// Get the image name
var imageName: String? = nil
if let collection = songs {
    for song in collection {
        if (song.cover_url != "") {
            imageName = song.cover_url
            break;
        }
    }
}

The Issues

So what’s wrong with this code? Here’s a quick overview.

Naming. There’s a mismatch here between imageName and cover_url. Is it a name? A URL? What? Turns out that this is a string, such as “ExperienceHendrix.jpg”. Both the cover_url property and the local imageName variable should reflect this better. And they do so without unsightly snake case naming. Fix: Rejigger the names to reflect that these are local file names and use standard Swift camelCase naming.

Parenthesized conditions and semicolons. The parentheses in the empty string check and the semicolon after the break are holdovers from Objective-C. Train yourself away from these reflexive habits. They’ll compile in Swift but they’re unnecessary. Fix: Ditch the parens and semicolon.

Comparing against an empty string. It’s bad form to test against an empty string or check if a collection’s count is zero. Prefer using the isEmpty property, which offers better semantics and performance. Fix: Refactor the empty string check.

Using an empty string instead of an optional. There’s a design flaw here in that this model should really use an optional String instead of an empty string to begin with (thanks Dave DeLong), but that’s another fish to fry that this redesign won’t address. In Swift, an optional represents the absence of a value in a way that an empty string does not convey.

Using the wrong type. The right type for a local file asset should be a file URL, not a string. Again, this redesign won’t address that.

Complex iteration: When you’re looking for the first member of a collection that satisfies a predicate, go functional. The Standard Library offers many ways to convert iteration that uses extra state (in this case the imageName variable) into a cleaner function call. Fix: use first(where:), which you get for free as part of the standard library.

Unnecessary conditional binding: If you’re going to use an optional value exactly once to extract some value and you can transform that binding into a function call, consider optional chaining instead. Fix: use songs? with a function.

The Refactor

Here’s what a refactored and Swiftier version of Brandon’s code might look like. This rewrite returns an optional string. That string contains the first non-empty coverImageFileName property within the songs collection. I think it’s a simpler approach with better naming and tighter code.

// Extract the file name for the first cover image
// found within the optional `songs` collection
let firstCoverImageFileName: String? = songs?
    .first(where: { !$0.coverImageFileName.isEmpty })?
    .coverImageFileName

Note that both calls in the functional chain require question marks. The first one, after songs, reflects that songs is an optional collection. The second, after the first(where:) call supports potentially nil searches (for example, where a collection has no covers).

This code won’t compile without that second question mark. Swift will, however, warn you at compile time that you’re attempting to make a call on an optional type if you omit it.

Soroush Khanlou, Dave DeLong, and Tim Vermeulen put their heads together for a slightly different approach:

let firstCoverImageFileName2: String? = songs?
    .lazy.flatMap({ $0.coverImageFileName })
    .first(where: { !$0.isEmpty })

Using a lazy flatMap may be a bit harder to wrap your head around, but it’s beautiful in that it pulls each image name, and then returns the first non-empty one, ensuring that you don’t create any intermediate storage.

Tim further adds:

extension Sequence {
    func firstResult<T>(_ transform: (Iterator.Element) -> T?) -> T? {
        for element in self {
            if let result = transform(element) {
                return result
            }
        }

        return nil
    }
}
let firstCoverImageFileName: String? = songs?
    .firstResult { 
        $0.coverImageFileName.isEmpty ? nil : $0.coverImageFileName }

and, as an alternative:

extension Optional {
    init(_ value: Wrapped, where predicate: (Wrapped) -> Bool) {
        self = predicate(value) ? value : nil
    }
}

songs?.firstResult { 
    Optional($0.coverImageFileName, where: { !$0.isEmpty }) 
}

Also, I threw together this, which I’m not sure if I hate or not:

extension Sequence {
    func first<T>(
        apply transform: @escaping (Iterator.Element) -> T?, 
        where predicate: @escaping (T) -> Bool) -> T? {
        return self.lazy
            .flatMap({ transform($0) })
            .first(where: predicate)
     }
}

songs?.first(apply: { song in song.coverImageFileName }, 
    where: { name in !name.isEmpty } )

Swift Style

Agree with these refactoring recommendations? Disagree? Would you redesign this differently? Drop a comment or send a tweet. Swift Style is now available to order as a paper book, an ebook, and a paper/ebook bundle.

Tip of the hat to Mike Ash who was also offering feedback. Thanks to Soroush Khanlou, Dave DeLong, Tim Vermeulen, Zev Eisenberg