Promises Code Review

Question

I have just started playing with nodeJS and I try to get familiar with promises.

I have the code bellow and for me it looks like it can be improved by moving the retry logic and put it inside getValue2.

Retry logic is different than the getValue2.

The problem is that as soon as I put the logic inside the method, getValue2 finishes before retryGetValue2's promise finishes.

Ideally, I want to remain just with line sendPriceResponse(res, res2); and get rid of the if-else

Any recommendations?

This is the code:

getValue1(link).then( function(res)
{
  getValue2(link2).then(function(res2)
  {
    if(res2==='') // retry logic <===---------------|
    {                                             //|
      retryGetValue2(link2).then(function(res2new)//|
      {                                           //|
        sendPriceResponse(res, res2new);          //|
      });                                         //|
    }                            //_________________|
    else
    {
        sendPriceResponse(res, res2);
    }
  });
 });

getValue2 looks like :

function getValue2(link)
{
 return getInfo(link); // returns a promise
}

Use async/await instead of promise https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function Promise is a hell https://medium.com/@pyrolistical/how-to-get-out-of-promise-hell-8c20e0ab0513 Use async/await https://blog.risingstack.com/node-js-async-best-practices-avoiding-callback-hell-node-js-at-scale/ — Subhojit Mondal, Jul 29 '18 at 06:22
Why is `The problem is that as soon as I put the logic inside the method, getValue2 finishes before retryGetValue2's promise finishes.` a problem? Isnt that exactly what the code is supposed to do? — Jonas Wilms, Jul 29 '18 at 06:30
@JaromandaX and Quentin, please take a look at the [Code Review help center](https://codereview.stackexchange.com/help/on-topic). This question would be **off-topic** at Code Review. — Mast, Jul 29 '18 at 07:33
Why? It's an MCVE and stripped of all context. At CR, we need context. — Mast, Jul 29 '18 at 07:34
@JaromandaX You recommended the site anyway. Don't redirect questions to sites if those sites don't want them. — Mast, Jul 29 '18 at 07:47

Jonas Wilms · Accepted Answer · 2018-07-29T10:16:15.833

2

Your retry logic can be as easy as:

 const getInfoRetry = link => getInfo(link).then(res => res ? res : getInfoRetry(link));

That will not only retry once but as lojg as it gets a valid response. Now just do:

getInfoRetry(link1).then(res =>
  getInfoRetry(link2).then(res2 =>
     sendPriceResponse(res, res2)
  )
);

You could also get them in parallel:

 Promise.all([getInfoRetry(link1), getInfoRetry(link2)])
  .then(([res, res2]) => sendPriceResponse(res, res2));

edited Jul 29 '18 at 10:16

answered Jul 29 '18 at 06:26

Jonas Wilms

132,000
20
149
151

After reconsidering, this is exactly what I was looking for. Thanks! What I don't get is here ".then(res => res ? res : getInfoRetry(link)" In case there is no res, call the retry method, but if the res exists, we return res (which is not a promise). So how can we use .then after it? – ClaudiuSocaci Jul 29 '18 at 10:06
@claudiuSocaci its promises flattening behaviour. Calling `.then` returns a promise itself, which will resolve to what you return from the inside, or if that is a promise, it will resolve to what that promise resolves to. So in conclusion: `getInfoRetry(link).then(res => ...)` – Jonas Wilms Jul 29 '18 at 10:14
YFYI: I could have written it as `.then(res => res || getInfoRetry(link))` but I thought that would just confuse anyone ... – Jonas Wilms Jul 29 '18 at 10:15
I actually used @CertainPerformance's solution on top of this solution and now it looks even better. thanks all! – ClaudiuSocaci Jul 29 '18 at 10:16
@claudiuSocaci well his solution is overly complicated ... Whatsoever: glad to help :) – Jonas Wilms Jul 29 '18 at 10:17
thanks for the last edit. I am really happy about the result after refactoring. His solution is more complex but I like the idea of getting rid of indentation. – ClaudiuSocaci Jul 29 '18 at 10:21
Nah if you replace simple code with complex code for the sake of getting less indentation ... – Jonas Wilms Jul 29 '18 at 10:30

CertainPerformance · Answer 2 · 2018-07-29T06:35:02.493

You should chain the promises together to avoid the promise-as-callback anti-pattern, which results in indentation hell; instead, return each Promise in the chain.

When you want to pass along a value (such as res) in addition to waiting for a Promise to resolve (such as your getValue2), use Promise.all to pass an array of both the value and the promise. Similarly, in the next function, you can use Promise.all to pass an array of both res and either the retryGetValue2 or the res2, depending on whether res2 is falsey:

getValue1(link)
  .then((res) => Promise.all([res, getValue2(link2)]))
  .then(([res, res2]) => Promise.all([
    res,
    res2 || retryGetValue2(link2)]
  ))
  .then(([res, verifiedRes2]) => {
    sendPriceResponse(res, verifiedRes2);
  });

If res2 can possibly be falsey but not the empty string, then you'll have to use the conditional operator instead of ||:

Promise.all([
  res,
  res2 === '' ? retryGetValue2(link2) : res2
])

I like this approach! Thanks! – ClaudiuSocaci Jul 29 '18 at 09:54 — ClaudiuSocaci, Jul 29 '18 at 09:54

cheekujha · Answer 3 · 2018-07-29T07:01:10.057

-1

const getValue2WithRetry = (link) => {
  return getValue2(link).then((res2) => {
    if(res2 === ''){
      return getValue2WithRetry(link);
    }else{
      return res2;
    }
  });
}

edited Jul 29 '18 at 07:01

answered Jul 29 '18 at 06:30

cheekujha

781
4
12

1

https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it – Jonas Wilms Jul 29 '18 at 06:31

Promises Code Review

3 Answers3