current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

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?

share|improve this question
    
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 – Zorgatone Feb 11 at 19:07
up vote 2 down vote accepted

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

};
share|improve this answer
    
Very helpful comments. Really appreciate it. Thank you. – kmansoor Feb 13 at 4:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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