0

I'm very new in NodeJS and following is my coding and you guys can catch up what is the purpose of that coding. My code checker told me that to use promise chaining and that can use a single .catch() at the end of a chain when I use that. Please let me know how to convert my current coding by using promise chaining.

jobSeekerService.findJobSeekerUserByEmail(user.username).then(function (jsFromDb) {
    if (jsFromDb) {
        jobAlertsService.findJobAlertWithCategoriesByJobSeekerId(jsFromDb.jobSeeker.id).then(function (subscription) {
            if (subscription.categories && subscription.categories != 0) {
                res.redirect(redirectUrl);
            } else {
                res.redirect('/subscriptions');
            }
        }, function (err) {
            winston.error('applications.controller findJobAlertWithCategoriesByJobSeekerId : %s', err);
            res.send(500);
        });
    } else {
        res.redirect(redirectUrl);
    }
}, function (err) {
    winston.error('applications.controller findJobSeekerUserByEmail : %s', err);
    res.send(500);
});
PPShein
  • 13,309
  • 42
  • 142
  • 227

2 Answers2

1

You can/should use promises like this

promise.then(function(res) {
  /* do whatever you want with the res */
  return promise2(foo);
}).then(function(data) {
  /* data from promise2 are here */
}).catch(function(err) {
  /* err parameter contains error from reject() */
});

You can use .then() infinite number of times, but catch() can be called just once for error handling (it will contain error from the corresponding reject() in the Promise() object.

Edited the example for better understanding.

Paulooze
  • 1,147
  • 10
  • 13
  • Can you transform my coding into promise chaining? – PPShein Mar 16 '16 at 09:34
  • @Paulooze: Please put that in your answer, not somewhere else – Bergi Mar 16 '16 at 09:47
  • @Bergi: It says the answer is too long to put it here :(. – Paulooze Mar 16 '16 at 09:48
  • 1
    @Paulooze: It surely isn't. Maybe don't format everything as code? What exactly does the popup say? – Bergi Mar 16 '16 at 10:08
  • `jobSeekerService.findJobSeekerUserByEmail(user.username).then(function (jsFromDb) { if (jsFromDb) { return jobAlertsService.findJobAlertWithCategoriesByJobSeekerId(jsFromDb.jobSeeker.id); } else { res.redirect(redirectUrl); } }).then(function (subscription) { if (subscription.categories && subscription.categories != 0) { res.redirect(redirectUrl); } else { res.redirect('/subscriptions'); } }).catch(function(err) { winston.error('applications.controller findJobSeekerUserByEmail : %s', err); res.send(500); });` – Paulooze Mar 16 '16 at 10:12
  • @Paulooze res.redirect(redirectUrl) will be done twice if jsFromDb is NULL. – PPShein Mar 16 '16 at 10:56
  • @ppshein: Weird, I can't find a reason from just looking at the code. From what I understand, that redirect should be called only if jsFromDb is null, or there are subscription.categories (and they are not equal to 0). Maybe if you found out if the double redirect occurs only in `if (jsFromDb) {}` or it gets called in the second `then()`. That way you should be able to fix that. – Paulooze Mar 16 '16 at 11:31
1

You might use

jobSeekerService.findJobSeekerUserByEmail(user.username).then(function (jsFromDb) {
    if (jsFromDb)
        return jobAlertsService.findJobAlertWithCategoriesByJobSeekerId(jsFromDb.jobSeeker.id).then(function (subscription) {
            if (subscription.categories && subscription.categories != 0)
                return redirectUrl;
            else
                return '/subscriptions';
        }, function (err) {
            winston.error('applications.controller findJobAlertWithCategoriesByJobSeekerId : %s', err);
            throw err;
        });
    else
        return redirectUrl;
}, function (err) {
    winston.error('applications.controller findJobSeekerUserByEmail : %s', err);
    throw err;
}).then(function(url) {
    res.redirect(url);
}, function() {
    res.send(500);
});

The important things for chaining are those returns. Admittedly, this solution is not very exciting and much of an improvement over what you already have. The problem is that distinguishing error conditions always requires nesting with promises. You might be able to avoid that by finding the name of the respective method within the error object (e.g. in the stack trace) so that you can combine the error handler into a generic one.

A second thing you could do is to flatten the success handler for the category search, but I'm not sure whether it's worth it. With both of these, your code would now look like this:

jobSeekerService.findJobSeekerUserByEmail(user.username).then(function (jsFromDb) {
    return jsFromDb
      ? jobAlertsService.findJobAlertWithCategoriesByJobSeekerId(jsFromDb.jobSeeker.id)
      : {categories: 1};
}).then(function (subscription) {
    return (subscription.categories && subscription.categories != 0)
      ? redirectUrl
      : '/subscriptions';
}).then(function(url) {
    res.redirect(url);
}, function(err) {
    winston.error('applications.controller: %s', err);
    res.send(500);
});

This also has the benefit that it will catch exceptions from subscription being null or so.

Community
  • 1
  • 1
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • 1
    I think it would be a good idea to flatten and turn the last `, function(err) ` to a `catch` to log redirect errors too. I'd also use arrow functions because why not. – Benjamin Gruenbaum Mar 16 '16 at 10:28
  • @BenjaminGruenbaum: I dunno, does `res.redirect` throw errors that `res.send` could handle? I might lack node proficiency, but I wouldn't *expect* it to throw, so I'd rather not handle them and get them reported as unhandled rejections. – Bergi Mar 16 '16 at 10:40