5

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

// Other Methods

    func fetchAllUsers(completionHandler: usersCompletionHandler?) {

        var users: [User] = []

        let group = DispatchGroup()
        let serialQueue = DispatchQueue(label: "serialQueue")

        DatabaseProvider.sharedInstance.usersDatabaseReference.observe(.value, with: {
            snapshot in
            guard let data = snapshot.value as? [String: AnyObject] else { return }

            for (_, value) in data {

                guard let values = value as? [String: AnyObject] else { return }
                guard let newUser = User(data: values) else { return }
                group.enter()
                newUser.fetchProfileImage(completionHandler: { (image) in
                    serialQueue.async {
                        newUser.profileImage = image
                        users.append(newUser)
                        group.leave()
                    }
                })
            }

            group.notify(queue: .main, execute: {
                completionHandler?(users)
            })

        })

    }



    func fetchAllChatrooms (completionHandler: chatroomsHandler?)  {
        var chatrooms = [Chatroom]()
        let group = DispatchGroup()
        let serialQueue = DispatchQueue(label: "serialQueue")

        DatabaseProvider.sharedInstance.chatroomsDatabaseReference.observe(.value, with: { snapshot  in
            guard let data = snapshot.value as? [String: AnyObject] else { return }

            for (_, value) in data {
                guard let value = value as? [String: AnyObject] else { return }
                guard let newChatroom = Chatroom(data: value) else { return }

                group.enter()
                self.fetchChatroomUsers(chatroom: newChatroom, completionHandler: { (chatroom) in
                    serialQueue.async {
                        chatrooms.append(newChatroom)
                        group.leave()
                    }
                })
            }
            group.notify(queue: .main, execute: {
                completionHandler?(Array(Set<Chatroom>(chatrooms)))
            })
        })
    }


private func validateNewUsers(currentUser: User, users: [User], chatrooms: [Chatroom], completionHandler: usersCompletionHandler?) {
    let chatPartners = self.chatPartners(currentUser: currentUser, chatrooms: chatrooms)
    let validUsers = users
        .filter { currentUser.id != $0.id }
        .filter { currentUser.distanceFromUser(atLocation: $0.coordinates) <= 8046.72 }
        .filter { !chatPartners.contains($0.id) }
    completionHandler?(validUsers)
}

private func chatPartners (currentUser: User, chatrooms: [Chatroom]) -> Set<String> {
    var results = [String]()
    for chatroom in chatrooms {
        if currentUser.id == chatroom.firstUserId {
            results.append(chatroom.secondUserId)
        } else if currentUser.id == chatroom.secondUserId {
            results.append(chatroom.firstUserId)
        }
    }
    return Set(results)
}
  • 4
    Not necessarily, but unless they're really simple, they should probably be dedicated methods passed as arguments. Complex, nested closures are very difficult to reason about and troublesome to debug, so we strongly discourage it in practice. – Josh at The Nerdery Jun 06 '17 at 02:28
  • @JoshatTheNerdery, I presume my methods count as being too complex for nested closures. Any tip on how you would approach a refactor would be greatly appreciated. I'm trying hard to find a balance between writing clean code and getting things to work properly. –  Jun 06 '17 at 02:54
  • 1
    There is a dedicated SE page for code review : https://codereview.stackexchange.com/ . While your question seems on topic here, you might get good insighys there too. – Losiowaty Jun 06 '17 at 05:35

0 Answers0