How to optimize this Node.js + q code to prevent callback hell?

Question

I am using Q to prevent callback hell but I have reached a part of my code that I don't know how to arrange:

I am searching for scheduled messages to be delivered. For each of them, I try to send them one by one and if it could be sent, removes it from the database. The ugly part is to have a then() inside a for loop. This way I end up having nested promises instead of nested callbacks!!!! Suggestions?

appLog.debug("Looking for scheduled messages");
var messages = messageService.findScheduled()
.then(function(messages){
    appLog.debug("found [%d] stored messages",messages.length);
    for(var i = 0; i<messages.length; i++){
        messageService.send(msg.namespace, msg.message, msg.data)
        .then(function(result) {
            if (result == constants.EVENT_EMIT_SENT) {
                appLog.debug("Message [%s] sent!!!", msg._id);
                messageService.remove(msg._id)
                    .then(function(result) {
                         appLog.debug("Message deleted: [%s]", msg._id);
                    })
                    .fail(function(err) {
                        appLog.error("The message couldn't be deleted: [%s]", msg._id);
                    });

            }else if (result == constants.EVENT_EMIT_NOT_SENT_AND_NOT_STORED) {
                appLog.debug("Message [%s] not sent", msg._id);
            }

        });
    }
});

One way is by not using `q` and using `bluebird` instead. `q` is slow. — majidarif, Oct 15 '14 at 12:58
@majidarif: Using bluebird doesn't solve your code layout, though. — Bergi, Oct 15 '14 at 13:02

score 1 · Accepted Answer · edited May 23 '17 at 12:11

The ugly part is to have a then() inside a for loop

No, that can happen. Though the functional programming style of promises usually leads to using .map() with a callback anyway :-) Looping is a control structure, and does require nesting (except you use exceptions for control flow, i.e. branching). What you don't have to do is to nest promises in promise callbacks, though.

I'd simplify your loop body to

function sendAndDelete(msg) {
    return messageService.send(msg.namespace, msg.message, msg.data)
    .then(function(result) {
        if (result == constants.EVENT_EMIT_SENT) {
            appLog.debug("Message [%s] sent!!!", msg._id);
            return msg._id;
        } else if (result == constants.EVENT_EMIT_NOT_SENT_AND_NOT_STORED) {
            appLog.debug("Message [%s] not sent", msg._id);
            throw new Error("Message not sent");
        }
    })
    .then(messageService.remove)
    .then(function(result) {
         appLog.debug("Message deleted: [%s]", msg._id);
    }, function(err) {
         appLog.error("The message has not been deleted: [%s]", msg._id);
    });
}

Now you can do something like

messageService.findScheduled()
.then(function(messages){
    appLog.debug("found [%d] stored messages",messages.length);
    return Q.all(messages.map(sendAndDelete));
})
.then(function() {
    appLog.debug("all messages done");
});

Hi Bergi! Thanks for your solution. I am testing it but with this line -> return Q.all(messages.map(sendAndDelete)); the method sendAndDelete is never called. If I remove Q.all, leaving: return messages.map(sendAndDelete), it works although I see the "all messages done" in the logs before "Message [%s] sent!!!" . Do you know why? it should be the last message. Why doesn't Q.all work? — codependent, Oct 15 '14 at 13:47
It doesn't make sense that `messages.map(sendAndDelete)` calls it but `Q.all(messages.map(sendAndDelete));` doesn't, except there's something else like a syntax error. If you omit it (and don't return a promise fro the `then` callback), then it doesn't know there is something to wait for and just resolves the next promise with `undefined`, calling the "done" message immediately. Notice that I have updated `sendAndDelete`, it needs to return a promise of course as well. — Bergi, Oct 15 '14 at 14:31

score 0 · Answer 2 · answered Oct 15 '14 at 12:56

Yeah, one way to clean it up would be to defined a named function:

function SuccessfulSend(result) {
        if (result == constants.EVENT_EMIT_SENT) {
            appLog.debug("Message [%s] sent!!!", msg._id);
            messageService.remove(msg._id)
                .then(function(result) {
                     appLog.debug("Message deleted: [%s]", msg._id);
                })
                .fail(function(err) {
                    appLog.error("The message couldn't be deleted: [%s]", msg._id);
                });

        }else if (result == constants.EVENT_EMIT_NOT_SENT_AND_NOT_STORED) {
            appLog.debug("Message [%s] not sent", msg._id);
        }

    }

and then:

.then(SuccessfulSend);

score 0 · Answer 3 · answered Oct 15 '14 at 13:04

Simply refactoring your functions into named functions and trying to make better use of promise chaining would be a vast improvement.

var messageService, appLog, constants;

appLog.debug("Looking for scheduled messages");

messageService.findScheduled()
  .then(function(messages){
    appLog.debug("found [%d] stored messages", messages.length);
    messages.map(processMessage);
  });

function processMessage(msg) {

  msg.data.dontStore = true;
  messageService

    .send(msg.namespace, msg.message, msg.data)

    .then(function(result) {

      if (result !== constants.EVENT_EMIT_SENT) {
        throw new Error(constants.EVENT_EMIT_SENT);
      }

      appLog.debug("Message [%s] sent!!!", msg._id);
      return removeMessage(msg);

    })

    .fail(function(err) {
      appLog.debug("Message [%s] not sent", msg._id);
      throw err;
    });

}

function removeMessage(msg) {
  return messageService.remove(msg._id)
    .then(function() {
      appLog.message("Message deleted: [%s]", msg._id);
    })
    .fail(function(err) {
      appLog.message("The message couldn't be deleted: [%s]", msg._id);
      throw err;
    });
}

How to optimize this Node.js + q code to prevent callback hell?

3 Answers3

Linked