0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You will probably need to use Promise.all to wait for all the promises to resolve. \$\endgroup\$ – Marc Rohloff May 13 '17 at 18:37
  • \$\begingroup\$ Is there no way to query the database for all items at once? This would be the first thing I'd try. \$\endgroup\$ – le_m May 13 '17 at 21:57
  • \$\begingroup\$ If this code actually does what you want, please rephrase your text to ask for a review and possible better ways to do it. Currently it kind of seems like you're asking us to write code for you, which is off-topic for this site. \$\endgroup\$ – holroy May 14 '17 at 12:11
  • \$\begingroup\$ @holroy Thanks for the feedback, I've edited my question to explicitly ask for a review / critique and suggestions. \$\endgroup\$ – Sports May 15 '17 at 15:54
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed response. "need to hit the db for every element in item and resolve the promise belongTo returns if every element belongs to user" - that's exactly right. Most of your suggestions make sense right away, especially Promise.resolve/reject/all() (which I somehow hadn't heard of until now). However, I'm stuck on the way you're using items.every(). Array.every returns a boolean -- true if every callback on the respective items in the array returns true, false otherwise. It looks like you're calling Promise.all with a Boolean -- not an array of Promises. \$\endgroup\$ – Sports May 15 '17 at 16:02
  • \$\begingroup\$ @Sports oh... :facepalm: I meant array.map. \$\endgroup\$ – Joseph May 15 '17 at 16:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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