3
\$\begingroup\$

I'm building a Slack bot to facilitate the standups for teams of developers.

I want the bot to run async and not have to wait for one person's response before it starts interviewing others.

Here's what I think is the solution for it, however I'm not really sure. I would appreciate a second eye on this.

const users = await Users.getActive();

  for(let user of users) {
    const message_obj = { user: user.slack_id };

    const questions = await Questions.getByTeamId(user.team.id);

    bot.startPrivateConversation(message_obj, function(err, convo) {
      if(!err){
        for (let question of questions) {
          convo.addQuestion(question.text, function (response, convo) {
            convo.next();

          }, {key: `${question.id}`}, `default`);
        }

        convo.on(`end`, async function (convo) {
          if (convo.status == `completed`) {
            const report =  await Reports.createReport(user.id);
            for (let question_id in convo.extractResponses()) {
              if (convo.extractResponses().hasOwnProperty(question_id)) {
                await Responses.createResponse(question_id, report.id, convo.extractResponses()[question_id]);
              }
            }
            convo.say(`Nice Job! 👍`);

          } else {
            // this happens if the conversation ended prematurely for some reason
            convo.say(`OK, nevermind!`);
          }
        });

      }else {
        bot.botkit.log(`Failed to start a conversation :(`, err);
      }
    });

  }
\$\endgroup\$
4
\$\begingroup\$

It seems okay at first glance.

One thing that is kind of bothering me is how you use the beautiful async/await format for some of it then switch back to callback hell for other pieces. Perhaps it's worth integrating Bluebird or using native Promises to make the code read more consistently.

Another standout is this line:

const questions = await Questions.getByTeamId(user.team.id);

This will block, so perhaps you should move it inside of your callback so all the users questions can be queried asynchronously.

Update

Here's a code block showing how you might implement the questions query more efficiently...

const users = await Users.getActive();

for(let user of users) {
    const message_obj = { user: user.slack_id };        

    bot.startPrivateConversation(message_obj, async (err, convo) => {
        const questions = await Questions.getByTeamId(user.team.id);
        // the rest of your convo code below...
    });  
}

What's the difference?

In your example the for loop will wait each iteration until the questions are returned for that user before initiating the async conversation. This means that you are synchronously querying each user's questions. This may not be a problem (perhaps the questions return so fast it's not noticable). However, if for some reason one of the questions queries was slower, it would then increase the time it took for subsequent user conversations to begin.

In my update example I've moved the questions query inside of your asynchronous conversation block so it will no longer block the remaining users conversations from starting.

Here's the same thing with full promise support

const startConvo = Promise.promisify(bot.startPrivateConversation);
const users = await Users.getActive();

for(let user of users) {
    const message_obj = { user: user.slack_id };        
    startConvo(message_obj)
    .then(Questions.getByTeamId(user.team.id))
    .then((questions) => {
        // the rest of your convo code below...
    }).catch((error) => {
        //handle errors
    });  
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you, Eric, for your time. I was using callbacks as this is the way supported by the API I'm using. I'm trying to read more on promisifying callbacks. Also, I'm not sure where exactly should I put the const questions = await Questions.getByTeamId(user.team.id); given that I'm promisifiying the callbacks now In my thread above, I updated the code after my trials. \$\endgroup\$ – YGilany Nov 17 '17 at 22:49
  • \$\begingroup\$ I've added an update to my original answer to illustrate how you could modify the questions query. \$\endgroup\$ – Eric Uldall Nov 20 '17 at 21:51

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.