I wrote a post earlier which outlined the thought process I went through while slimming down some code in the KeySmash toy project.  Make sure you read Part 1 first.

I received several great pieces of feedback about that blog post which inspired some thoughts on making the code better.

Getting functional with .map()

It was pointed out to me by teequ on reddit that the .map() function of Array could reduce the code even further from

var keys = [UIKeyCommand]()

"ABCDEFGHIJKLMNOPQRSTUVWXYZ".each({  
    keys += [UIKeyCommand($0, .Command, "keyPressed:"),
             UIKeyCommand($0, .Control, "keyPressed:"),
             UIKeyCommand($0, nil, "keyPressed:")]
})

to

var keys = [UIKeyCommand]()

"ABCDEFGHIJKLMNOPQRSTUVWXYZ".each { letter in
    keys += [.Command, .Control, nil].map { UIKeyCommand(letter, $0, "keyPressed:") }
}

.map() is a functional programming style operation (on Array) which accepts a closure parameter, executes that closure with each element of the array (.Command, .Control, and nil in this case), and populates an array with the result.  If a programmer and their team are comfortable with .map() or higher order functions in general, then you could definately argue that not much readability is lost here and the whole thing can collapse from three lines to one.  The benefit of this more functional style approach seems to go way up when we think about capturing more modifier+key combinations which, hey!, is exactly what I am trying to do in KeySmash.

For example

var keys = [UIKeyCommand]()

"ABCDEFGHIJKLMNOPQRSTUVWXYZ".each { letter in
    keys += [nil, .Command, .Control, .AlphaShift, .Alternate, .Shift,
             .Command | .Control, .Command | .Control | .Shift].map 
    		{ UIKeyCommand(letter, $0, "keyPressed:") }
}

would capture 'A', Command-A, Control-A, Capslock-A, Alt-A, Shift-A, Command-Control-A, Command-Control-Shift-A, and so on for each letter.  You could keep going here with every unique combination of modifier keys spelled out or even generate all the unique combinations as a base array to work from rather than typing them in manually.  The code above is enough to illustrate my point though.  The functional .map operation is folding down what would otherwise be 9 lines of UIKeyCommand's in this case.  Pretty freaking awesome.  As a side benefit, we declared "keyPressed:" in only one place so it's a bit more DRY, thus easier to maintain/change.   To see how this affected the KeySmash project check out this diff and note that the newly added lines cover a LOT more combinations than the many lines they replaced.

Declare and create the Selector once

Even without functional programming operations like .map(), we should consider pulling the creation of the "keyPressed:" selector out to make the code more "robust" (to quote @Aurelius61 ).  I agree it is worth consideration, and the more repeated lines of UIKeyCommand you have, the more consideration it should get IMHO.

var keys = [UIKeyCommand]()
let action = Selector("keyPressed:")		// new line

"ABCDEFGHIJKLMNOPQRSTUVWXYZ".each({  
    keys += [UIKeyCommand($0, .Command, action),
             UIKeyCommand($0, .Control, action),
             UIKeyCommand($0, nil, action)]
})

That new line is slightly more verbose, and understanding exactly what those UIKeyCommand lines connect up now requires your eyeball/brain to jump around from the action variable use to its declaration (to see what is in it), but there are two great benefits:

  1. Only creating that Selector object once rather than each iteration of the loop, 26*3 times. (unless compiler manages to optimize that out anyway)
  2. It's more DRY -- "keyPressed:" is defined in one place, which means you only have to maintain it in one place.

Does the String -> Character conversion belong in a String extension, or in a UIKeyCommand convenience constructor?

Upon revisiting the the code after sleep and examining the seperation of concerns, I discovered I had created a new looping function on String in order to more easily pass an argument to a UIKeyCommand constructor.   But, if I'm making a new UIKeyCommand  constructor anyways, why not just have it take responsibility for accepting a more convenient argument type?

What would that look like?

Rather than an extension on String

extension String {
    // convenience way to loop through with Character->String convert built in
    func each(closure: (String) -> Void ) {
        for digit in self
        {
            closure(String(digit))
        }
    }
}

to allow this

var keys = [UIKeyCommand]()

"ABCDEFGHIJKLMNOPQRSTUVWXYZ".each({  
    keys += [UIKeyCommand($0, .Command, "keyPressed:"),
             UIKeyCommand($0, .Control, "keyPressed:"),
             UIKeyCommand($0, nil, "keyPressed:")]
})

we could just make our convenience constructor even more convenient and do the Character->String conversion there...

extension UIKeyCommand {
    // allow unnamed params since we re-use this like 20 times in close proximity and the meaning is clear, AND convert Character->String
    convenience init( _ input: Character!, _ modifierFlags: UIKeyModifierFlags, _ action: Selector) {
        self.init(input: String(input), modifierFlags: modifierFlags, action: action)
    }
}

thus allowing us to use the more standard for loop again and dropping the String extension entirely!  YES!

var keys = [UIKeyCommand]()

let action = Selector("keyPressed:")

for letter in "ABCDEFGHIJKLMNOPQRSTUVWXYZ" {
    keys += [UIKeyCommand(letter, .Command, action),
             UIKeyCommand(letter, .Control, action),
             UIKeyCommand(letter, nil, action),
    ]
}

or if you prefer the more functional style:

for letter in "ABCDEFGHIJKLMNOPQRSTUVWXYZ" {
    keys += [.Command, .Control, nil].map { UIKeyCommand(letter, $0, "keyPressed:") }
}

A downside of this approach is that now the Character->String conversion happens in each invocation of UIKeyCommand(...) which would be about 3 times more often than if we did the conversion once within the loop ourselves or within the .each() String extension.

With performance not even being a concern here, that's a pretty small downside considering how much code we can shave off -- the entire String .each() extension we wrote earlier #facepalm.

Cheers!