CODEWITHSUNDEEP
iosarraysswiftfunctional-programming
Alexander Leontev
Alexander Leontev

Reputation: 477

Swift - avoiding nested forEach closures?

Let's say I have an array of closures that I want to run on every UITouch. Here's the code I use:

touches.filter { touch in
    return touch.phase == .Ended && touch.tapCount == 1
}.forEach { touch in
    actionsOnTap.forEach { action in
        action(touch)
    }
}

It bugs me that there's nested forEach statement, and I guess there's some clean way that can be applied exactly for that case, but I can't think of it. Can anyone give me a hint?

Upvotes: 9

Views: 4803

Answers (4)

Rob Napier
Rob Napier

Reputation: 299455

This is a good example of why forEach is not a universal (or even appropriately common) replacement for for-in. This code become shorter (140 chars vs 186 chars) and clearer just using a traditional for loop:

for touch in touches where touch.phase == .Ended && touch.tapCount == 1 {    
    for action in actionsOnTap {
        action(touch)
    }
}

It also doesn't create an extra array copy the way the filter does. This isn't a general reason not to use filter. filter is a very powerful tool that should be used often, but in this case, it's clearer and more efficient to use for.

Edited to use @originaluser2's suggestion of where rather than guard. That probably is better Swift.

Upvotes: 6

Hamish
Hamish

Reputation: 80901

You should definitely eliminate the filter from your logic and possibly use a guard inside the first loop instead, for the sake of efficiency and conciseness. I also agree with @Rob's and @matt's suggestion of using a traditional for loop instead of forEach – at least for the first loop.

Although a (maybe even cleaner) alternative is the integrate the touch conditional logic into the for loop directly through using the where clause, as well as possibly folding your forEach into a single line (whichever you find more readable).

I'd write it like this:

for touch in touches where touch.phase == .Ended && touch.tapCount == 1 {
    actionsOnTap.forEach{$0(touch)}
}

Upvotes: 9

matt
matt

Reputation: 535566

Personally, I like the nesting. I would write:

for touch in touches {
    if touch.phase == .Ended {
        if touch.tapCount == 1 {
            actionsOnTap.forEach {$0(touch)}
        }
    }
}

To me, that's clean and (above all) clear.

Upvotes: 11

Jans
Jans

Reputation: 11250

As you have two heterogeneous array types. another solution that avoid the extra iteration that filter do is filter yourself the touches you want to check.

touches.forEach{
  guard $0.phase == .Ended && $0.tapCount == 1 else { return }

  actions.forEach{ action in
     action($0)
  }
}

Upvotes: 1

Related Questions