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
8 Comments
I haven’t really written any Swift so I’m sure familiarity is a huge part of it, but I do not find the rewritten code easier to read. Not saying what you are doing is wrong of course. It just feels like obfuscated code to me. I always wonder how easy functional code is to work with a year later.
That’s a really valid point. To my eyes, this reads as: “the first item of songs where the cover file image name isn’t empty”, with all those textual cues in-place. Putting the original one in sentence form is much harder. “Make a space for a possible file name, then if the songs collection is not nil, iterate over each song in the collection, checking if the cover file image name isn’t empty. If you find one, then store that name and stop what you’re doing”
I have to agree with this. I find all the functional stuff immensely *un*readable. All of the chaining without an intermediate steps is just brevity at the cost of readability.
I think closures are really great in so many contexts but here the syntax really gets in the way:
.first(where: { !$0.coverImageFileName.isEmpty })
Part of the illegibility for me comes from how the text is organized.
I feel like
let firstCoverImageFileName: String? =
songs?.first(where: …)..
is fare more readable. I can see without moving my eyes to the end of the previous line what I am looking for the first matching element of.
I would of course want the final .coverImageFilename on the same line for the same reason… the local context is lost when it is all split out line by line like this.
Which brings me to why I don’t like this at all..
It’s a run-on sentence of code. A breathless, pause-less statement.
There should be intermediate variables which declare (and document) the intent of each step of the computation. And the compiler should be smart enough to optimize it all away.
—–
let firstSongWithCoverImage: Song?
let firstCoverImageFilename: String?
firstSongWithCoverImage = songs?.first(where: {$0.coverImageFileName.isEmpty})
firstCoverImageFilename = firstSongWithCoverImage?.coverImageFileName
—-
This, to me is, clear, concise, and self documenting code.
If the compiler doesn’t create the same code from this and your initial refactor then that’s a compiler problem.. not a code problem.
I think that the String? type can be omitted since compiler now knows that firstCoverImageFileName is now optional string
“Make it Schwiftier” Rick and Morty style.
In the lazy version, besides being able to omit the type (as Kamil Pyc already pointed out) we can use map, no reason to flatMap.
let firstCoverImageURLString = songs?
.lazy.map { $0.coverImageURL }
.first { !$0.isEmpty }
Also, the absence of a cover image should be represented by nil, hence coverImageFileName should be Optional so the code would be:
let firstCoverImageFileName = songs?
.lazy
.flatMap { $0.coverImageURL }
.first
Any reason you initialized imageName to nil?