Ensure every element in array passes asynchronous test

Question

I've written a function which ensures all objects in the items array belong to the user. This involves a database check, which results in the code being asynchronous. This code works, but feels sloppy. I'd appreciate a review and suggestions for how to improve it.

The main challenge I had with writing this was to use items.every() with Promises and I don't think the solution I came up with was very elegant. I spent quite a bit of time on it and couldn't think of a better way to do it.

Intuitively async/await feels like a good fit for this type of problem but I had difficulty applying it here.

function belongTo (user, items) {
  return new Promise(resolve => {
    resolve(items.every((item) => {
      return new Promise((resolve, reject) => {
        Item.findOne({ owner: user,
          assetID: item.assetid,
          appid: item.appid,
          contextid: item.contextid,
          marketHashName: item.marketHashName
        }).then((item) => {
          if (Object.keys(item).length !== 0) resolve(true)
          else resolve(false)
        }).catch(reject)
      })
    }))
  })
}

I'm using the JavaScript Standard style guide.

Answer 1

First of all, it appears that Item.findOne already returns a promise. You don't need to wrap it again in a promise. Simply use that same promise as the return value for your array.every.

Now as far as I understand your problem, you just need to hit the db for every element in item and resolve the promise belongTo returns if every element belongs to user. I don't think your code will work that way. What will happen is that belongTo return a promise will resolve immediately with the value returned by array.every.

Next, you don't need to pass true or false as promise resolution. The fact that it resolved/rejected is enough to tell you that. Also, you don't need to wrap an operation with a promise to create a promise. There's Promise.resolve() and Promise.reject() that create already resolved/rejected promises.

function belongTo (user, items) {
  const pingPromises = items.map(item => Item.findOne({ owner: user,
    assetID: item.assetid,
    appid: item.appid,
    contextid: item.contextid,
    marketHashName: item.marketHashName
  }).then((item) => {
    // Validate result if we should resolve/reject this promise.
    return Object.keys(item).length ? Promise.resolve() : Promise.reject();
  }));

  // Do not catch. If Item.findOne rejects, let's reuse that rejection.

  // Accepts an array of promises. Returns a promsie that resolves when all
  // promises resolve, rejects when at least one rejects.
  return Promise.all(pingPromises);
}

Now even with that code, the operation itself is very inefficient. You might want to consider creating some form of compound query instead and only hit the DB once. Not very familiar with MongoDB, but in MySQL this can easily be done by using multiple conditions.