3
\$\begingroup\$

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)
            })
        })
    })
}
\$\endgroup\$
0
\$\begingroup\$

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)
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – AnonProgrammer Jun 7 '17 at 4:02
  • \$\begingroup\$ 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. \$\endgroup\$ – user1118321 Jun 7 '17 at 4:09
  • \$\begingroup\$ 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/… \$\endgroup\$ – AnonProgrammer Jun 7 '17 at 4:48
  • \$\begingroup\$ If you check out the link, you'll see how using multiple singleton networking classes propelled me to make this function of nested closures. \$\endgroup\$ – AnonProgrammer Jun 7 '17 at 4:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.