2
\$\begingroup\$

Not confident enough if this would be the correct 'node way' of writing Promises:

Essentially, there are two async database calls, the result from both of these calls has to be collected and used in rendering the view:

Creating ...

exports.getIndustryById = function(industryId) {
   return new Promise(function(resolve, reject) {
    Industry.findOne({
      value: industryId
}, function(err, industry) {
  if (err) {
    return reject(err);
  }
  return resolve(industry);
});
})
}

exports.getCountryById = function(countryId) {
 return new Promise(function(resolve, reject) {
  Country.findOne({
   value: countryId
  }, function(err, country) {
  if (err) {
    return reject(err);
  }
   return resolve(country);
  });
 })
}

And using ...

  exports.renderEditProfile = function(req, res, next) {
  var i, c;
  ct.getIndustryById(req.user.industry)
    .then(function(industry) {
      i = industry;

      return ct.getCountryById(req.user.country);
    }).then(function(country) {
      c = country;

      res.render('profile_edit', {
        profile: req.user,
        industry: i,
        country: c
      });
    })
    .catch(function(err) {
      res.status(500).json({'message': err});
    });
};

This works, but am I getting Promises right?

\$\endgroup\$
  • \$\begingroup\$ If you look at A+ Promises lib, they have some great integration with node. It also allows to convert a node-like asyn/callback function to return a promise \$\endgroup\$ – Zorgatone Feb 11 '16 at 19:07
2
\$\begingroup\$

Promises benefit from the new ES6 syntax, making callbacks a bit compact. If you can use Node.js 4+, then by all means take advantage of the new syntax.

Your indents could need a bit of work. You're mixing 2-space and 4-space. It doesn't really matter which one you chose as long as you are consistent.

var i, c;
ct.getIndustryById(req.user.industry)
  .then(function(industry) {
    i = industry;

Instead of putting an outsider i and c to store the values temporarily until the last then, why not pass it through as a resolved value? That way, your functions don't need to use temporary variables.

exports.renderEditProfile = function(req, res, next) {
  ct.getIndustryById(req.user.industry).then( industry => {
    return ct.getCountryById(req.user.country).then( country => ({ country, industry }) );
  }).then( data => {
    res.render('profile_edit', {
      profile: req.user,
      industry: data.industry,
      country: data.country
    });
  }).catch(function(err) {
    res.status(500).json({'message': err});
  });
};

Alternatively, both requests don't seem to depend on each other. Therefore, it makes no sense to fire them sequentially. Suggesting you fire them in parallel, and use Promise.all to listen for the response.

exports.renderEditProfile = function(req, res, next) {

  const industryPromise = ct.getIndustryById(req.user.industry);
  const countryPromise = ct.getCountryById(req.user.country)

  Promise.all([ industryPromise, countryPromise ]).then(resolution => {
    res.render('profile_edit', {
      profile: req.user,
      industry: resolution[0],
      country: resolution[1],
    });
  }).catch(function(err) {
    res.status(500).json({'message': err});
  });

};
\$\endgroup\$
  • \$\begingroup\$ Very helpful comments. Really appreciate it. Thank you. \$\endgroup\$ – kmansoor Feb 13 '16 at 4:55

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.