Is this a correct usage of error handling in a promise?

Question

I am having a hard time understanding the best way to handle errors. Is the code below a good practice?

Is there a better,shorter, easier to read way to handle flow and errors?

How can you code to handle all errors so they don't get swallowed? Thank you.

  privateProperties.setSearchedData = function (data, searchFor) {
    return new Promise(function (resolve, reject) {
      switch (searchFor) {
        case "photos":
          try {
            var photoItems = [];

            if (data.items && data.items.constructor === Array) {
              data.items.forEach(function (value) {
                if (value.link) {
                  photoItems.push(value.link);
                } else {
                  console.error('Link is undefined');
                  reject(new Error('Link is undefined'));
                }
              });
              privateProperties._uiInstances['main'].uiReference.set("photos.photoItems",photoItems)
                .then(resolve(photoItems))
                .catch(function (error) {
                  reject(error);
                })
            } else {
              reject(new Error('Data.items is undefined or not array'));
            }
          } catch (e) {
            throw e;
          }
        break;

        case "videos":
        break
      }
    });

  };

Which call here is asynchronous? Is it `uiReference.set`? What are you returning a promise for? — , Jun 29 '17 at 08:11
No. Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! In fact you don't seem to need promises at all in here. — Bergi, Jun 29 '17 at 18:36
Why do you pass a function to `catch`, but not use a function in `then`? — Bergi, Jun 29 '17 at 18:36
after reading all the replies and comments i have refactored to not use promise at all since it was not needed. Thank you all for you time dedication it has helped understand promises better. — Mihai, Jun 30 '17 at 09:52

score 1 · Accepted Answer · 2017-06-30T10:00:37.450

If I understand your code correctly, you can just write:

privateProperties.setSearchedData = function(data, searchFor) {
  switch (searchFor) {

    case "photos":
      if (!Array.isArray(data.items)) 
        return Promise.reject("data.items is undefined or not an array");

      const photoItems = data.items.map(item => item.link);

      if (photoItems.some(item => !item)) {
        console.error('Link is undefined');
        return Promise.reject("Link is undefined");
     }

      return privateProperties._uiInstances['main'].uiReference
        .set("photos.photoItems", photoItems);

    case "videos":
      return Promise.reject("videos not supported");
  }
}

You are essentially trying to return the uiReference.set promise, so just return it. There is no need to catch a rejection from it--the rejection will be passed through for the consumer to handle. There is no need to create your own promise, and then laboriously resolve or reject your own new promise based on its result. Just return the promise you want. The error checking can be done up front and an explicit rejected promise returned in that case.

Using await, this could look like the following. You can throw instead of returning Promise.reject. The throw will result in the function returning a rejected promise with that as a reason.

privateProperties.setSearchedData = async function(data, searchFor) {
  switch (searchFor) {
    case "photos":
      return privateProperties._uiInstances['main'].uiReference
        .set("photos.photoItems", data.items.map(item => item.link));

    case "videos":
      throw new Error("videos not supported");
  }
}

I've omitted the check for an array, since a non-array will cause the .map to fail, which will result in the entire function returning a promise rejected for some reason such as "Cannot read property map of null".

I've also assumed a redesign of uiReference.set to do the check for missing or null links, and reject if that happens, so we don't have to check that the links are all there either. When all is said and done, this function really needs to do nothing more than switch on searchFor, extract the links from the data items, and then call and return uiReference.set. Personally, I would prefer to have separate functions for searching photos and videos, in which case it becomes very clean:

privateProperties.setSearchedPhotosData = async function(data) {
  return privateProperties._uiInstances['main'].uiReference
    .set("photos.photoItems", data.items.map(item => item.link));
}

The only remaining purpose of the async is to turn a failure of map (when data.items is not an array) into a rejected promise. However, if this situation represents a coding error, rather than a semi-expectable situation that you want to handle, it might be better to just let it throw (by removing async).

score 0 · Answer 2 · answered Jun 29 '17 at 08:09

No it is not. ;) Use reject without new Error and when you catch an error don't use throw but reject( e )

Then later on you can say

privateProperties.setSearchedData( ... )
  .then( callback )
  .catch( errorCallback );

Sidenote: Split your code into more functions for better readability. I guess all inside case "photos": (except break) can be its own function.

Alon · Answer 3 · 2017-06-30T07:20:07.150

You catch all errors but then just throw them. It's completely equal to just not catch them in the first place. Use reject instead of throw.

Regarding an easier and more readable way to use Promises, You can use async await which makes your code a lot more readable and concise, and use Promises under the hood. Then your function will look like that:

privateProperties.setSearchedData = async function (data, searchFor) {

switch (searchFor) {
    case "photos":
        var photoItems = [];

        if (data.items && data.items.constructor === Array) {
            data.items.forEach(function (value) {
                if (value.link) {
                    photoItems.push(value.link);
                } else {
                    console.error('Link is undefined');
                    throw new Error('Link is undefined');
                }
            });
            privateProperties._uiInstances['main'].uiReference.set("photos.photoItems",photoItems);
            return photoItems;
        } else {
            throw new Error('Data.items is undefined or not array');
        }
        break;

    case "videos":
        break
}

};

The snippet you posted doesn't appear to use async/await? – Bergi Jun 29 '17 at 18:37 — Bergi, Jun 29 '17 at 18:37

Is this a correct usage of error handling in a promise?

3 Answers3