Fetch nearby chat users using Firebase and nested completion handlers

Question

I have a method that calls three functions that each make a request to Firebase and pass back data in a completion handler. Once the data from 2 of the completion handlers is sent back, I call another method to pass in the data and send back a valid result. Is nesting blocks like this a sign of poor design?

func fetchNearbyUsers(for user: User, completionHandler: usersCompletionHandler?) {
 self.fetchAllUsers(completionHandler: { (users: [User]) in
 ChatProvider.sharedInstance.fetchAllChatrooms(completionHandler: { (chatrooms: [Chatroom]) in
 self.validateNewUsers(currentUser: user, users: users, chatrooms: chatrooms, completionHandler: { (validUsers: [User]) in
 guard validUsers.isEmpty == false else {
 completionHandler?([])
 return
 }
 completionHandler?(validUsers)
 })
 })
 })
}

Answer 1

Nesting

In general, I find it harder to read nested closures. I think it's even worse when calling APIs that run the closures asynchronously because it makes it harder to understand what's happening when. That said, these 3 aren't that bad. (And despite my dislike of nesting them, I've done it myself!)

You can use the various shorthands to make this slightly easier to read by inferring types and using the rules for trailing closures. I think your code could be rewritten like this:

func fetchNearbyUsers(for user: User, completionHandler: userCompletionHandler?) {
 fetchAllUsers() { users in
 ChatProvider.sharedInstance.fetchAllChatrooms() { chatrooms in
 validateNewUsers(currentUser:user, users:users, chatrooms:chatrooms) { validUsers in
 guard validUsers.isEmpty == false else {
 completionHandler?([])
 return
 }
 completionHandler?(validUsers)
 }
 }
 }
}

I've used type inference on users, chatrooms and validUsers to shorten the closures' code. And I used trailing closure notation to further reduce the number of characters needed. Whether it's more or less readable is a judgement call, but it looks cleaner to me without being too sparse.

Naming

If it were me, I'd change the name of the function to fetchUsers(nearby user: User, completionHandler: usersCompletionHandler?). I say that because for is a keyword, and even if the compiler doesn't mind it, it looks weird to me to have it there naked. If it was forUser: it would be less weird to me. Also, I think fetchUsers(nearby:, completionHandler:) flows better.

Is That guard Needed?

Why guard if validUsers.isEmpty? Inside of it you simply call the completionHandler with an empty array. But if validUsers.isEmpty is true and the guard isn't there, you'd still just be calling the completionHandler with an empty array. So you might be able to further shorten it to:

func fetchNearbyUsers(for user: User, completionHandler: userCompletionHandler?) {
 fetchAllUsers() { users in
 ChatProvider.sharedInstance.fetchAllChatrooms() { chatrooms in
 validateNewUsers(currentUser:user, users:users, chatrooms:chatrooms) { validUsers in
 completionHandler?(validUsers)
 }
 }
 }
}

• AnonProgrammer: Thanks for your insight. your refactored version is much cleaner. I'm also curious to know if you found it odd seeing the singleton 'ChatProvider'. I have a few more singletons like UserProvider, and ImageProvider, which handle interactions with server like networking helper classes i suppose. This approach seems to be frowned upon. Do you agree? Thanks for your time.
• user1118321: I don't generally have a problem with singletons. They're pretty common throughout much of Cocoa. (Things like [+NSNotificationCenter defaultCenter], or [+AVAudioCenter sharedInstance] for example.) The downside of a singleton is that it is global state, so it can be difficult to track where it is changed. But if it's used appropriately, then it doesn't seem problematic to me.
• AnonProgrammer: Thanks. I'll provide a link to a question that gives more visualization into my simple approach. I will also look into MVCS. stackoverflow.com/questions/44381560/…
• AnonProgrammer: If you check out the link, you'll see how using multiple singleton networking classes propelled me to make this function of nested closures.