CODEWITHSUNDEEP
javascriptnode.jsasynchronousconditional-statements
TiagoM
TiagoM

Reputation: 3526

How to optimize this block of code since its async

var orderItems = userData.shoppingcart;

var totalPrice = 0;
userData.shoppingcart.forEach(function(itemName, i){
   _data.read('menuitems', itemName, function(err, itemData){
       if(!err && itemData)
       {
          totalPrice += itemData.price;
          if(++i == userData.shoppingcart.length){

              // Only here when all the itemNames have been read I should continue

          }
       }
    });
});

As you can see, the call to _data.read is async, because I am reading from a file.

But I need to wait to have all the files read, so I can have the totalPrice calculated. That's why I place that condition [ ++i == userData.shoppingcart.length ].

I am new to javascript and nodejs in general, never was very good programmar at it, but anyway my point is that I am sure this isn't a good approach, what if both files are readed at same time, and then that condition is never ran or the calculation of totalPrice is badly done?

Can someone give me please some guidance on this? Thank you in advance!

Upvotes: 1

Views: 616

Answers (2)

Christian
Christian

Reputation: 7852

This is how you can read the items in sequence using promises (async/await flavour):

var orderItems = userData.shoppingcart;

let totalPrice = 0;
for (let itemName of userData.shoppingcart) {
  const itemData = await _data.read('menuitems', itemName);
  totalPrice += itemData.price;
}

This example assumes that _data.read supports async/await. If it doesn't, however, it can "promisified" using the promisify function in nodejs' util module

Upvotes: 1

Yona Appletree
Yona Appletree

Reputation: 9142

Given that you don't specify what context this is in, I'm going to make a few assumptions:

  1. I assume that _data.read() doesn't already support returning a promise.
  2. I assume that this code needs to either call a callback function or return a promise.

My (somewhat naive) approach to this would be to:

  1. Map the orderItems into Promises for each price of that item.
  2. Map the result into a total
  3. Return that resulting promise OR call the callback.

Here's an annotated example of how you might do it:

// Promise.all takes an array of promises and returns
// a new promise that completes when all the promises in the array are complete.
const promiseOfPrices = Promise.all(
    // Here we map all the items in the shopping cart into promises for each of their prices
    userData.shoppingcart.map(

        // The Promise object takes a single function that it will immediatly call with functions to resolve or
        // reject the promise. I suggest reading up on Promises if you're not familiar with them.
        itemName => new Promise((resolve, reject) => {
            // Here, we have a `reject` and `resolve` function that will each complete the new promise,
            // either in success or error respectfully.

            // Do the actual read of your file or database or whatever
            _data.read('menuitems', itemName, (err, itemData) => {
                // If there was an error, reject this promise.
                if (err) reject(err);

                // Otherwise, we're successful and we resolve with the price of the item
                else resolve(itemData.price);
            });
        })
    )
);

// Now, we have a promise (promiseOfPrices) for all the prices of the items in the cart. We use `then` which will
// perform a transform on the result, much like the `map` function on an Array.

const promiseOfTotal = promiseOfPrices.then(
    // Here we use the `Array.reduce` function to succinctly sum the values in the array.
    arrayOfCartItemPrices => arrayOfCartItemPrices.reduce(
        // For each item, reduce calls our function with the current sum and an item in the array. We produce a new
        // sum by adding the sum to the item price.
        (sum, itemPrice) => sum + itemPrice,

        // This is the initial value for sum, 0.
        0
    )
);

If you can return a promise, and you just want to return the total, then

return promiseOfTotal;

If you have a callback that expects (err, result), then do something like this:

promiseOfTotal.then(
    result => callback(null, result),
    error => callback(error, null),
)

If you need to do more work on the result, you can do so with another then:

promiseOfTotal.then(
    priceSum => {
        // Do work here
    },
    // Optionally handle errors here:
    error => {
        // Do error handling here.
    }
)

Note that by using promises, arrow functions, and array comprehensions (map and reduce) we avoid complex and hard to follow mutation of variables and loops. This is a "functional" style of programming, and while somewhat harder to learn, is generally safer and cleaner than the alternatives. I suggest taking the time to understand how this works, as it'll help you write code that's less likely to have bugs when you're dealing with complex things like asynchronicity.

Finally, I haven't run this code. It may well have a bug or two. Feel free to ask for clarification or if it doesn't work.

Best of luck!

P.S. I didn't go in to using async/await because I think it would be less clear than using the Promises directly, and the use of Promise.all is needed for the parallelism anyways. It's absolutely possible to use them to good effect here, but I'll leave that as an exercise to the OP.

Upvotes: 2

Related Questions