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)
            }
        }
    }
}