current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

Sometimes I need to repeat an execution of rejected promises several times, for example, to fetch some data over internet. There is a wrapper, which accepts Promise and tryCount:

function tryPromise(f, tryCount) {
  return f().then(null, function(v) {
    tryCount--;
    if (tryCount > 0) {
      return $q.reject(v);
    }
    return tryPromise(f, tryCount);
  });
}

Is there some hidden troubles?

share|improve this question
    
What is $q in return $q.reject(v);? – Sergio Jul 26 at 10:15
    
@Sergio Angular's Promise docs.angularjs.org/api/ng/service/$q – Vladimir Gamalian Jul 26 at 10:25
2  
if tryCount is <= 0 that code will enter a loop and kill your browser if there is no waiting moment in f.. What numbers to you give tryCount when you invoque that function? will it ever be <=0? – Sergio Jul 26 at 10:34
    
@Sergio you are right, will be better to place additional conditional for that case. – Vladimir Gamalian 2 days ago
up vote 3 down vote

Well, first of all you aren't trying a promise. What you're really doing is trying out a function that returns a promise. So the name is already misleading. Try something better.

Next, f and v doesn't really tell me anything. Only when I read through the code did I realize f was the function to try out and v is supposed to be a value. Additionally, even if you name v a value, it's still not correct. Most reject handlers often pass an error object.

Code that requires one to actually read to understand is a bad practice. In this case it's negligible due to the size of the function. But if you're in larger codebases, it's a nightmare to maintain. Name your functions and variables meaningfully.

Lastly, your function doesn't allow for function arguments. It would be nice to at least accept argument 3 onwards as the arguments, or accept an array that would be the arguments.

My take on it would be:

function tryPromiseFunction(functionToTry, retries, ...args){
  return functionToTry(...args).then(null, error => {
    return retries > 0 ? tryPromiseFunction(functionToTry, retries - 1, ...args)
                       : Promise.reject(error);
  });
}
share|improve this answer
    
Looks good, many thanks. Not sure, should we compare retries with > 1instead > 0 ? – Vladimir Gamalian 2 days ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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