Best practice to handle exception when using Q.promise

Question

I have the following method:

module.exports.getId = function(someObject) {

    var myId = null;

    return Q.Promise(function(resolve, reject, notify) {

        // Loop through all the id's
        someObject.user.player._id.forEach(function (id) {

            if (id.root == "1.2.3.4.5.6") {
                myId = id.extension;
            }
        });

        resolve(myId);
    });
};

This method works great as long as someObject exists and has the attributes user.player._id.

The problem i'm having is that if someObject is null or does not have all the appropriate nested attributes, an exception is thrown and the promise is never resolved. The only way I actually see the exception is if I have a .fail on the calling function, but that still doesn't actually resolve the promise.

Example of how I currently can see the exception:

myLib.getId.then(function() {
  // something
}).fail(function(err) {
  console.log(err);
});

I know 2 ways to get around this problem, but i'm not sure which, if either is the best way to handle something like this.

Option 1 (use try/catch inside my Q.promise):

module.exports.getId = function(someObject) {

    var myId = null;

    return Q.Promise(function(resolve, reject, notify) {

      try {
        // Loop through all the id's
        someObject.user.player._id.forEach(function (id) {

            if (id.root == "1.2.3.4.5.6") {
                myId = id.extension;
            }
        });

      } catch(e) {
        reject(e);
      }

      resolve(myId);
    });
};

Option 2 (explicitly check if someObject.user.player._id exists):

module.exports.getId = function(someObject) {

    var myId = null;

    return Q.Promise(function(resolve, reject, notify) {

      ifi(someObject.user.player._id exists..) {

        // Loop through all the id's
        someObject.user.player._id.forEach(function (id) {

            if (id.root == "1.2.3.4.5.6") {
                myId = id.extension;
            }
        });

        resolve(myId);
      } else {
        reject('invalid object');
      }
    });
};

Option 1 seems to smell funky to me because i'm using try/catch inside of a promise. Option 2 solves my problem, but any other unexpected exceptions will not get caught.

Is there a better way I should be handling this?

Not the downvoter but I wonder - why are you using a promise in synchronous code to begin with? Also - if you're using a promise a throw and a reject are the same thing. — Benjamin Gruenbaum, Oct 29 '14 at 16:41
I'm using a promise in synchronous code b/c it's part of a library and I don't want the user to have to know which methods in the lib are synchronous and which are not, so i made them all async. I know a throw and reject are the same. I'm not actually throwing anything. — Catfish, Oct 29 '14 at 16:54
Sure you are - when you do `a.b.c...` without knowing what's there you're writing code that might throw so that `rejext` after the catch is at best redundant. — Benjamin Gruenbaum, Oct 29 '14 at 16:58
Correct me if i'm wrong, but Reject inside a catch is not redundant b/c in my very specific case an exception is obviously being raised since I see `[TypeError: Cannot read property 'user' of null]` in my log, but the promise is never rejected or resolved so it just hangs. — Catfish, Oct 29 '14 at 17:50

Ruan Mendes · Answer 1 · 2014-10-29T18:08:49.060

-1

Your first example has a few problems:

When you catch an exception, you are rejecting the promise, then resolving the promise. That's breaking the promise contract; You can get around that by calling resolve within the try, not outside.
By using try/catch, you could be swallowing unintended errors. That is you are assuming that the only error would come from someObject.user.player._id not existing. That may be true at the moment, but it's not guaranteed to remain true as your code evolves.

By testing exactly for the known error condition, you know you won't be swallowing unexpected errors. Therefore, I would use your second example.

edited Oct 29 '14 at 18:08

answered Oct 29 '14 at 16:36

Ruan Mendes

90,375
31
153
217

@downvoter Please explain downvotes so I can delete/improve the answer – Ruan Mendes Oct 29 '14 at 16:44
1

Not me but I don't see how this addresses the question other than "it is my opinion that such and such" – Benjamin Gruenbaum Oct 29 '14 at 16:45
Downvoting trolls on here today – Catfish Oct 29 '14 at 16:54
@BenjaminGruenbaum Because in one case, the OP is relying on a `try/catch` that may be catching a different exception? The second case, the OP is actually guarding against an expected problem? I thought it was common knowledge that `try/catch` everything is a problem because it swallows errors. – Ruan Mendes Oct 29 '14 at 17:12
1

I'm confident you can write an answer explaining the problems in OP's design, however this current answer just isn't very good - not downvote worthy but still. – Benjamin Gruenbaum Oct 29 '14 at 17:28
1

@JuanMendes, will try/catch swallow an error only if it is not rethrown or returned. – Roamer-1888 Oct 29 '14 at 18:22
@Roamer-1888 That's correct, you can re-throw an exception if it's not the type you mean to catch, but in the OP's case, it's a `TypeError`, which could be anything really, not just in `someObject.user.player._id` – Ruan Mendes Oct 29 '14 at 18:27
2

I know what you mean but for me it's a question not of "the type you mean to catch" but "the type you mean handle when caught" and "what you do with anything unhandled". Since you may choose, as in the quesion, not to handle any errors in the catch clause, then unconditionally rethrowing (or rejecting a promise) is a perfectly valid. Thus, *any* error, not just the one(s) you predict, can be handled/rethrown elsewhere. In short, (a) `reject(e)` ensures that nothing is swallowed and (b) I'm not saying that specific safety is a bad thing, just that you should't be too docrinaire about try/catch. – Roamer-1888 Oct 29 '14 at 19:33

Best practice to handle exception when using Q.promise

1 Answers1