How to optimize this block of code since its async










1














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!










share|improve this question

















  • 1




    "what if both files are read at same time" - isn't that exactly what you want?
    – Bergi
    Nov 11 '18 at 19:23






  • 1




    Basically, yes that's the idea - counting how many of the tasks are done, and then continue. There are many pitfalls though, like taking errors into account correctly or the edge case of 0 iterations (like on an empty input), so you better use a some proven existing code instead of reinventing the wheel. Learn about promises and Promise.all.
    – Bergi
    Nov 11 '18 at 19:25










  • Yes they can all read at the same time, but I want the flux to continue ONLY when all have been read, so I have the right and exact amount of totalPrice variable. Yes I have heard about Promise, but don't know how to use it, will do some search on this, thanks @Bergi
    – TiagoM
    Nov 11 '18 at 19:34
















1














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!










share|improve this question

















  • 1




    "what if both files are read at same time" - isn't that exactly what you want?
    – Bergi
    Nov 11 '18 at 19:23






  • 1




    Basically, yes that's the idea - counting how many of the tasks are done, and then continue. There are many pitfalls though, like taking errors into account correctly or the edge case of 0 iterations (like on an empty input), so you better use a some proven existing code instead of reinventing the wheel. Learn about promises and Promise.all.
    – Bergi
    Nov 11 '18 at 19:25










  • Yes they can all read at the same time, but I want the flux to continue ONLY when all have been read, so I have the right and exact amount of totalPrice variable. Yes I have heard about Promise, but don't know how to use it, will do some search on this, thanks @Bergi
    – TiagoM
    Nov 11 '18 at 19:34














1












1








1







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!










share|improve this question













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!







javascript node.js asynchronous conditional






share|improve this question













share|improve this question











share|improve this question




share|improve this question










asked Nov 11 '18 at 19:08









TiagoM

1,48542560




1,48542560







  • 1




    "what if both files are read at same time" - isn't that exactly what you want?
    – Bergi
    Nov 11 '18 at 19:23






  • 1




    Basically, yes that's the idea - counting how many of the tasks are done, and then continue. There are many pitfalls though, like taking errors into account correctly or the edge case of 0 iterations (like on an empty input), so you better use a some proven existing code instead of reinventing the wheel. Learn about promises and Promise.all.
    – Bergi
    Nov 11 '18 at 19:25










  • Yes they can all read at the same time, but I want the flux to continue ONLY when all have been read, so I have the right and exact amount of totalPrice variable. Yes I have heard about Promise, but don't know how to use it, will do some search on this, thanks @Bergi
    – TiagoM
    Nov 11 '18 at 19:34













  • 1




    "what if both files are read at same time" - isn't that exactly what you want?
    – Bergi
    Nov 11 '18 at 19:23






  • 1




    Basically, yes that's the idea - counting how many of the tasks are done, and then continue. There are many pitfalls though, like taking errors into account correctly or the edge case of 0 iterations (like on an empty input), so you better use a some proven existing code instead of reinventing the wheel. Learn about promises and Promise.all.
    – Bergi
    Nov 11 '18 at 19:25










  • Yes they can all read at the same time, but I want the flux to continue ONLY when all have been read, so I have the right and exact amount of totalPrice variable. Yes I have heard about Promise, but don't know how to use it, will do some search on this, thanks @Bergi
    – TiagoM
    Nov 11 '18 at 19:34








1




1




"what if both files are read at same time" - isn't that exactly what you want?
– Bergi
Nov 11 '18 at 19:23




"what if both files are read at same time" - isn't that exactly what you want?
– Bergi
Nov 11 '18 at 19:23




1




1




Basically, yes that's the idea - counting how many of the tasks are done, and then continue. There are many pitfalls though, like taking errors into account correctly or the edge case of 0 iterations (like on an empty input), so you better use a some proven existing code instead of reinventing the wheel. Learn about promises and Promise.all.
– Bergi
Nov 11 '18 at 19:25




Basically, yes that's the idea - counting how many of the tasks are done, and then continue. There are many pitfalls though, like taking errors into account correctly or the edge case of 0 iterations (like on an empty input), so you better use a some proven existing code instead of reinventing the wheel. Learn about promises and Promise.all.
– Bergi
Nov 11 '18 at 19:25












Yes they can all read at the same time, but I want the flux to continue ONLY when all have been read, so I have the right and exact amount of totalPrice variable. Yes I have heard about Promise, but don't know how to use it, will do some search on this, thanks @Bergi
– TiagoM
Nov 11 '18 at 19:34





Yes they can all read at the same time, but I want the flux to continue ONLY when all have been read, so I have the right and exact amount of totalPrice variable. Yes I have heard about Promise, but don't know how to use it, will do some search on this, thanks @Bergi
– TiagoM
Nov 11 '18 at 19:34













2 Answers
2






active

oldest

votes


















2














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.






share|improve this answer






















  • Hi @YonaAppletree thank you so much for your detailed answer, never used promises, it's a good time to learn them! I believe I got the logic and understood in general your code. I just got a question, promiseOfPrices will be an array of all the prices right? In this case, if there was a single reject, I will know because the length of the array 'promiseOfPrices' will be different than the one I expect it to be, is that right? Just to clarify, I am accepting your detailed answer now :)
    – TiagoM
    Nov 13 '18 at 10:38










  • @TiagoM Promises are a really great abstraction, and I do suggest reading the docs, since there is more than I can say in one SO post. Answering your question: no. Promise.all produces a new promise which will resolve when all promises passed to it resolve (success) or reject when any one of them reject (fail). So, promiseOfPrices will be a Promise that resolves with an array of all prices. It will never resolve with an array of a different length. If any single one fails, the promiseOfPrices promise will reject (fail) with that first error.
    – Yona Appletree
    Nov 13 '18 at 20:39











  • The google introductory tutorial on Promises seems pretty good: developers.google.com/web/fundamentals/primers/promises
    – Yona Appletree
    Nov 13 '18 at 20:43










  • Put another way, if you want to know if anything went wrong, look at the example I gave about doing more work, with two arguments to .then() or you could use .catch() to catch errors.
    – Yona Appletree
    Nov 13 '18 at 20:47










  • Hello there @YonaAppletree just wanted to pass by and say thank you for the docs and explanation! I got it working finally the way I need, using promises.All and Then clause, also I will read the doc you sent me because it sounds very well documented! Thank you again for all the help and your time! All the best ;)
    – TiagoM
    Nov 14 '18 at 1:19


















1














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






share|improve this answer






















  • …assuming that _data.read supports promises.
    – Bergi
    Nov 11 '18 at 21:55










  • Yeah, and this runs in sequence, rather than parallel, which isn't likely desirable. One of my biggest issues with async/await is that it seems to encourage less parallel code.
    – Yona Appletree
    Nov 12 '18 at 19:31










  • @bergi yes, async/await is assumed in this case, will add it to the answer.
    – Christian
    Nov 12 '18 at 19:36











  • @YonaAppletree I find it more often than not that it's desirable to optimise for readability rather than performance.
    – Christian
    Nov 12 '18 at 19:39






  • 1




    @Christian Of course. But there's performance and there's performance. In this case, you're taking something that might be O(1) and turning it into O(N). That's just not the same as something like the intermediate arrays created when using comprehensions. O(1) vs O(N) is huge, especially if there's network or file reads or whatever. It's at least worth considering carefully.
    – Yona Appletree
    Nov 13 '18 at 2:06










Your Answer






StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "1"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: true,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: 10,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);













draft saved

draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53252200%2fhow-to-optimize-this-block-of-code-since-its-async%23new-answer', 'question_page');

);

Post as a guest















Required, but never shown

























2 Answers
2






active

oldest

votes








2 Answers
2






active

oldest

votes









active

oldest

votes






active

oldest

votes









2














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.






share|improve this answer






















  • Hi @YonaAppletree thank you so much for your detailed answer, never used promises, it's a good time to learn them! I believe I got the logic and understood in general your code. I just got a question, promiseOfPrices will be an array of all the prices right? In this case, if there was a single reject, I will know because the length of the array 'promiseOfPrices' will be different than the one I expect it to be, is that right? Just to clarify, I am accepting your detailed answer now :)
    – TiagoM
    Nov 13 '18 at 10:38










  • @TiagoM Promises are a really great abstraction, and I do suggest reading the docs, since there is more than I can say in one SO post. Answering your question: no. Promise.all produces a new promise which will resolve when all promises passed to it resolve (success) or reject when any one of them reject (fail). So, promiseOfPrices will be a Promise that resolves with an array of all prices. It will never resolve with an array of a different length. If any single one fails, the promiseOfPrices promise will reject (fail) with that first error.
    – Yona Appletree
    Nov 13 '18 at 20:39











  • The google introductory tutorial on Promises seems pretty good: developers.google.com/web/fundamentals/primers/promises
    – Yona Appletree
    Nov 13 '18 at 20:43










  • Put another way, if you want to know if anything went wrong, look at the example I gave about doing more work, with two arguments to .then() or you could use .catch() to catch errors.
    – Yona Appletree
    Nov 13 '18 at 20:47










  • Hello there @YonaAppletree just wanted to pass by and say thank you for the docs and explanation! I got it working finally the way I need, using promises.All and Then clause, also I will read the doc you sent me because it sounds very well documented! Thank you again for all the help and your time! All the best ;)
    – TiagoM
    Nov 14 '18 at 1:19















2














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.






share|improve this answer






















  • Hi @YonaAppletree thank you so much for your detailed answer, never used promises, it's a good time to learn them! I believe I got the logic and understood in general your code. I just got a question, promiseOfPrices will be an array of all the prices right? In this case, if there was a single reject, I will know because the length of the array 'promiseOfPrices' will be different than the one I expect it to be, is that right? Just to clarify, I am accepting your detailed answer now :)
    – TiagoM
    Nov 13 '18 at 10:38










  • @TiagoM Promises are a really great abstraction, and I do suggest reading the docs, since there is more than I can say in one SO post. Answering your question: no. Promise.all produces a new promise which will resolve when all promises passed to it resolve (success) or reject when any one of them reject (fail). So, promiseOfPrices will be a Promise that resolves with an array of all prices. It will never resolve with an array of a different length. If any single one fails, the promiseOfPrices promise will reject (fail) with that first error.
    – Yona Appletree
    Nov 13 '18 at 20:39











  • The google introductory tutorial on Promises seems pretty good: developers.google.com/web/fundamentals/primers/promises
    – Yona Appletree
    Nov 13 '18 at 20:43










  • Put another way, if you want to know if anything went wrong, look at the example I gave about doing more work, with two arguments to .then() or you could use .catch() to catch errors.
    – Yona Appletree
    Nov 13 '18 at 20:47










  • Hello there @YonaAppletree just wanted to pass by and say thank you for the docs and explanation! I got it working finally the way I need, using promises.All and Then clause, also I will read the doc you sent me because it sounds very well documented! Thank you again for all the help and your time! All the best ;)
    – TiagoM
    Nov 14 '18 at 1:19













2












2








2






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.






share|improve this answer














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.







share|improve this answer














share|improve this answer



share|improve this answer








edited Nov 11 '18 at 21:53

























answered Nov 11 '18 at 21:48









Yona Appletree

3,50552635




3,50552635











  • Hi @YonaAppletree thank you so much for your detailed answer, never used promises, it's a good time to learn them! I believe I got the logic and understood in general your code. I just got a question, promiseOfPrices will be an array of all the prices right? In this case, if there was a single reject, I will know because the length of the array 'promiseOfPrices' will be different than the one I expect it to be, is that right? Just to clarify, I am accepting your detailed answer now :)
    – TiagoM
    Nov 13 '18 at 10:38










  • @TiagoM Promises are a really great abstraction, and I do suggest reading the docs, since there is more than I can say in one SO post. Answering your question: no. Promise.all produces a new promise which will resolve when all promises passed to it resolve (success) or reject when any one of them reject (fail). So, promiseOfPrices will be a Promise that resolves with an array of all prices. It will never resolve with an array of a different length. If any single one fails, the promiseOfPrices promise will reject (fail) with that first error.
    – Yona Appletree
    Nov 13 '18 at 20:39











  • The google introductory tutorial on Promises seems pretty good: developers.google.com/web/fundamentals/primers/promises
    – Yona Appletree
    Nov 13 '18 at 20:43










  • Put another way, if you want to know if anything went wrong, look at the example I gave about doing more work, with two arguments to .then() or you could use .catch() to catch errors.
    – Yona Appletree
    Nov 13 '18 at 20:47










  • Hello there @YonaAppletree just wanted to pass by and say thank you for the docs and explanation! I got it working finally the way I need, using promises.All and Then clause, also I will read the doc you sent me because it sounds very well documented! Thank you again for all the help and your time! All the best ;)
    – TiagoM
    Nov 14 '18 at 1:19
















  • Hi @YonaAppletree thank you so much for your detailed answer, never used promises, it's a good time to learn them! I believe I got the logic and understood in general your code. I just got a question, promiseOfPrices will be an array of all the prices right? In this case, if there was a single reject, I will know because the length of the array 'promiseOfPrices' will be different than the one I expect it to be, is that right? Just to clarify, I am accepting your detailed answer now :)
    – TiagoM
    Nov 13 '18 at 10:38










  • @TiagoM Promises are a really great abstraction, and I do suggest reading the docs, since there is more than I can say in one SO post. Answering your question: no. Promise.all produces a new promise which will resolve when all promises passed to it resolve (success) or reject when any one of them reject (fail). So, promiseOfPrices will be a Promise that resolves with an array of all prices. It will never resolve with an array of a different length. If any single one fails, the promiseOfPrices promise will reject (fail) with that first error.
    – Yona Appletree
    Nov 13 '18 at 20:39











  • The google introductory tutorial on Promises seems pretty good: developers.google.com/web/fundamentals/primers/promises
    – Yona Appletree
    Nov 13 '18 at 20:43










  • Put another way, if you want to know if anything went wrong, look at the example I gave about doing more work, with two arguments to .then() or you could use .catch() to catch errors.
    – Yona Appletree
    Nov 13 '18 at 20:47










  • Hello there @YonaAppletree just wanted to pass by and say thank you for the docs and explanation! I got it working finally the way I need, using promises.All and Then clause, also I will read the doc you sent me because it sounds very well documented! Thank you again for all the help and your time! All the best ;)
    – TiagoM
    Nov 14 '18 at 1:19















Hi @YonaAppletree thank you so much for your detailed answer, never used promises, it's a good time to learn them! I believe I got the logic and understood in general your code. I just got a question, promiseOfPrices will be an array of all the prices right? In this case, if there was a single reject, I will know because the length of the array 'promiseOfPrices' will be different than the one I expect it to be, is that right? Just to clarify, I am accepting your detailed answer now :)
– TiagoM
Nov 13 '18 at 10:38




Hi @YonaAppletree thank you so much for your detailed answer, never used promises, it's a good time to learn them! I believe I got the logic and understood in general your code. I just got a question, promiseOfPrices will be an array of all the prices right? In this case, if there was a single reject, I will know because the length of the array 'promiseOfPrices' will be different than the one I expect it to be, is that right? Just to clarify, I am accepting your detailed answer now :)
– TiagoM
Nov 13 '18 at 10:38












@TiagoM Promises are a really great abstraction, and I do suggest reading the docs, since there is more than I can say in one SO post. Answering your question: no. Promise.all produces a new promise which will resolve when all promises passed to it resolve (success) or reject when any one of them reject (fail). So, promiseOfPrices will be a Promise that resolves with an array of all prices. It will never resolve with an array of a different length. If any single one fails, the promiseOfPrices promise will reject (fail) with that first error.
– Yona Appletree
Nov 13 '18 at 20:39





@TiagoM Promises are a really great abstraction, and I do suggest reading the docs, since there is more than I can say in one SO post. Answering your question: no. Promise.all produces a new promise which will resolve when all promises passed to it resolve (success) or reject when any one of them reject (fail). So, promiseOfPrices will be a Promise that resolves with an array of all prices. It will never resolve with an array of a different length. If any single one fails, the promiseOfPrices promise will reject (fail) with that first error.
– Yona Appletree
Nov 13 '18 at 20:39













The google introductory tutorial on Promises seems pretty good: developers.google.com/web/fundamentals/primers/promises
– Yona Appletree
Nov 13 '18 at 20:43




The google introductory tutorial on Promises seems pretty good: developers.google.com/web/fundamentals/primers/promises
– Yona Appletree
Nov 13 '18 at 20:43












Put another way, if you want to know if anything went wrong, look at the example I gave about doing more work, with two arguments to .then() or you could use .catch() to catch errors.
– Yona Appletree
Nov 13 '18 at 20:47




Put another way, if you want to know if anything went wrong, look at the example I gave about doing more work, with two arguments to .then() or you could use .catch() to catch errors.
– Yona Appletree
Nov 13 '18 at 20:47












Hello there @YonaAppletree just wanted to pass by and say thank you for the docs and explanation! I got it working finally the way I need, using promises.All and Then clause, also I will read the doc you sent me because it sounds very well documented! Thank you again for all the help and your time! All the best ;)
– TiagoM
Nov 14 '18 at 1:19




Hello there @YonaAppletree just wanted to pass by and say thank you for the docs and explanation! I got it working finally the way I need, using promises.All and Then clause, also I will read the doc you sent me because it sounds very well documented! Thank you again for all the help and your time! All the best ;)
– TiagoM
Nov 14 '18 at 1:19













1














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






share|improve this answer






















  • …assuming that _data.read supports promises.
    – Bergi
    Nov 11 '18 at 21:55










  • Yeah, and this runs in sequence, rather than parallel, which isn't likely desirable. One of my biggest issues with async/await is that it seems to encourage less parallel code.
    – Yona Appletree
    Nov 12 '18 at 19:31










  • @bergi yes, async/await is assumed in this case, will add it to the answer.
    – Christian
    Nov 12 '18 at 19:36











  • @YonaAppletree I find it more often than not that it's desirable to optimise for readability rather than performance.
    – Christian
    Nov 12 '18 at 19:39






  • 1




    @Christian Of course. But there's performance and there's performance. In this case, you're taking something that might be O(1) and turning it into O(N). That's just not the same as something like the intermediate arrays created when using comprehensions. O(1) vs O(N) is huge, especially if there's network or file reads or whatever. It's at least worth considering carefully.
    – Yona Appletree
    Nov 13 '18 at 2:06















1














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






share|improve this answer






















  • …assuming that _data.read supports promises.
    – Bergi
    Nov 11 '18 at 21:55










  • Yeah, and this runs in sequence, rather than parallel, which isn't likely desirable. One of my biggest issues with async/await is that it seems to encourage less parallel code.
    – Yona Appletree
    Nov 12 '18 at 19:31










  • @bergi yes, async/await is assumed in this case, will add it to the answer.
    – Christian
    Nov 12 '18 at 19:36











  • @YonaAppletree I find it more often than not that it's desirable to optimise for readability rather than performance.
    – Christian
    Nov 12 '18 at 19:39






  • 1




    @Christian Of course. But there's performance and there's performance. In this case, you're taking something that might be O(1) and turning it into O(N). That's just not the same as something like the intermediate arrays created when using comprehensions. O(1) vs O(N) is huge, especially if there's network or file reads or whatever. It's at least worth considering carefully.
    – Yona Appletree
    Nov 13 '18 at 2:06













1












1








1






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






share|improve this answer














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







share|improve this answer














share|improve this answer



share|improve this answer








edited Nov 12 '18 at 19:45

























answered Nov 11 '18 at 21:09









Christian

3,00622544




3,00622544











  • …assuming that _data.read supports promises.
    – Bergi
    Nov 11 '18 at 21:55










  • Yeah, and this runs in sequence, rather than parallel, which isn't likely desirable. One of my biggest issues with async/await is that it seems to encourage less parallel code.
    – Yona Appletree
    Nov 12 '18 at 19:31










  • @bergi yes, async/await is assumed in this case, will add it to the answer.
    – Christian
    Nov 12 '18 at 19:36











  • @YonaAppletree I find it more often than not that it's desirable to optimise for readability rather than performance.
    – Christian
    Nov 12 '18 at 19:39






  • 1




    @Christian Of course. But there's performance and there's performance. In this case, you're taking something that might be O(1) and turning it into O(N). That's just not the same as something like the intermediate arrays created when using comprehensions. O(1) vs O(N) is huge, especially if there's network or file reads or whatever. It's at least worth considering carefully.
    – Yona Appletree
    Nov 13 '18 at 2:06
















  • …assuming that _data.read supports promises.
    – Bergi
    Nov 11 '18 at 21:55










  • Yeah, and this runs in sequence, rather than parallel, which isn't likely desirable. One of my biggest issues with async/await is that it seems to encourage less parallel code.
    – Yona Appletree
    Nov 12 '18 at 19:31










  • @bergi yes, async/await is assumed in this case, will add it to the answer.
    – Christian
    Nov 12 '18 at 19:36











  • @YonaAppletree I find it more often than not that it's desirable to optimise for readability rather than performance.
    – Christian
    Nov 12 '18 at 19:39






  • 1




    @Christian Of course. But there's performance and there's performance. In this case, you're taking something that might be O(1) and turning it into O(N). That's just not the same as something like the intermediate arrays created when using comprehensions. O(1) vs O(N) is huge, especially if there's network or file reads or whatever. It's at least worth considering carefully.
    – Yona Appletree
    Nov 13 '18 at 2:06















…assuming that _data.read supports promises.
– Bergi
Nov 11 '18 at 21:55




…assuming that _data.read supports promises.
– Bergi
Nov 11 '18 at 21:55












Yeah, and this runs in sequence, rather than parallel, which isn't likely desirable. One of my biggest issues with async/await is that it seems to encourage less parallel code.
– Yona Appletree
Nov 12 '18 at 19:31




Yeah, and this runs in sequence, rather than parallel, which isn't likely desirable. One of my biggest issues with async/await is that it seems to encourage less parallel code.
– Yona Appletree
Nov 12 '18 at 19:31












@bergi yes, async/await is assumed in this case, will add it to the answer.
– Christian
Nov 12 '18 at 19:36





@bergi yes, async/await is assumed in this case, will add it to the answer.
– Christian
Nov 12 '18 at 19:36













@YonaAppletree I find it more often than not that it's desirable to optimise for readability rather than performance.
– Christian
Nov 12 '18 at 19:39




@YonaAppletree I find it more often than not that it's desirable to optimise for readability rather than performance.
– Christian
Nov 12 '18 at 19:39




1




1




@Christian Of course. But there's performance and there's performance. In this case, you're taking something that might be O(1) and turning it into O(N). That's just not the same as something like the intermediate arrays created when using comprehensions. O(1) vs O(N) is huge, especially if there's network or file reads or whatever. It's at least worth considering carefully.
– Yona Appletree
Nov 13 '18 at 2:06




@Christian Of course. But there's performance and there's performance. In this case, you're taking something that might be O(1) and turning it into O(N). That's just not the same as something like the intermediate arrays created when using comprehensions. O(1) vs O(N) is huge, especially if there's network or file reads or whatever. It's at least worth considering carefully.
– Yona Appletree
Nov 13 '18 at 2:06

















draft saved

draft discarded
















































Thanks for contributing an answer to Stack Overflow!


  • Please be sure to answer the question. Provide details and share your research!

But avoid


  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.

To learn more, see our tips on writing great answers.





Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


Please pay close attention to the following guidance:


  • Please be sure to answer the question. Provide details and share your research!

But avoid


  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.

To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53252200%2fhow-to-optimize-this-block-of-code-since-its-async%23new-answer', 'question_page');

);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

How to how show current date and time by default on contact form 7 in WordPress without taking input from user in datetimepicker

Syphilis

Darth Vader #20