2
\$\begingroup\$

A few years back I interviewed with a company for a Javascript position. After a couple of warm-up challenges I was presented with this:

Please write a function that calls back with true if all promises have resolved successfully, or false if at least one promise has rejected.

Given 'Promise' API:

promise.then(
  function resolve() { /* called when some async thing was successful */},
  function reject() { /* the async thing failed */ }
);

and also was given the following function structure and mocking code:

function all (promises, callback) {
    // TODO call back with `true` if all promises resolve(), or `false` if a promise has reject()ed
    promises.forEach(function (promise) {
        promise.then(
          function resolve() {},
          function reject() {}
        );
    });
}

// Some mocking code (NO NEED TO READ THIS):
function P() { return { then: function (resolve, reject) {
  setTimeout(function() { (5*Math.random()|0) ? resolve() : reject() }, Math.random()*1000);
}}}
var promises = [1,2,3,4,5].map(P);

all(promises, function (success) {
  console.log('The promises have ' + ( success ? '' : 'not ' ) + 'all resolved!' )
});

The gist of it: I had to write an almost Promise.all() method that would check if all async functions finished and how they finished (resolve/reject).

I didn't finish the challenge in the allocated time frame, so I failed (and years later, looking at the code I've written to finally solve it, if was the interviewer, I would failed me even if I was done in time...)

My current implementation

A few days ago I found the challenged buried on the hard drive and decided to give it a go (I timed myself to finish it in time):

Does not re-include the mocking code from above, but it is included in the jsbin below

function all (promises, callback) {
    const promisesStatus = [];
    const allPromisesChecked = (promisesArray = promises, promisesStatusArray = promisesStatus) => promisesStatusArray.length === promisesArray.length;
    const allPromisesPassed = (promisesArray = promisesStatus) => {
        if (promisesArray.filter(value => !value).length === 0) { return true; }
        return false;
    };
    promises.forEach(function (promise) {
        promise.then(
          function resolve() {
              promisesStatus.push(true);
              if (allPromisesChecked() && allPromisesPassed()) { callback(true); }
          },
          function reject() {
              promisesStatus.push(false);
              if (allPromisesChecked() && !allPromisesPassed()) { callback(false); }
          }
        );
    });
};

JSBin: https://jsbin.com/dajini/edit?js,console

Questions

Keeping in mind that this is to be done under the clock and under the interviewer's eyes (pair programming), hence under stress...

  1. Implementation - Leaving aside minor performance optimizations, could I have done it better? Another way that I am unaware of ?
  2. Time - How long does it take you ? (ballpark it) - Originally, I had to code two functions that dealt with string manipulations + this one in under 1 hour
\$\endgroup\$
3
\$\begingroup\$

Implementation

  1. The biggest thing that jumped out at me is your all function takes a callback instead of returning a Promise like Promise.all would do. (EDIT: it looks like the interview asked that of you, so that makes sense then. See my edit below)
  2. When your handling the reject branch of each promise (in the .then call), there's no reason to do anything fancy. As soon as you encounter an error, you can immediately reject the outer promise (or in your case, callback with an Error
  3. allPromisesChecked and allPromisesPassed gets a little verbose but as long as it works it kinda doesn't matter

This interested me so I took a shot at implementing it. Here's my code

// Promise.all polyfill
function all(promises) {
  return new Promise(function(resolve,reject) {
    var count = promises.length
    var result = []
    var checkDone = function() { if (--count === 0) resolve(result) }
    promises.forEach(function(p, i) {
      p.then(function(x) { result[i] = x }, reject).then(checkDone)
    })
  })
}

// delay helper for creating promises that resolve after ms milliseconds
function delay(ms, value) {
  return new Promise(function(pass) {
    setTimeout(pass, ms, value)
  })
}

// basic boilerplate to check an answer
function checkAnswer(promises) {
  all(promises).then(
    function(xs) { console.log(xs) },
    function(err) { console.log(err.message) }
  )
}

// resolved promises wait for one another but ensure order is kept
checkAnswer([
  delay(100, 'a'),
  delay(200, 'b'),
  delay(50, 'c'),
  delay(1000, 'd')
])

// check that error rejects asap
checkAnswer([
  delay(100, 'a'),
  delay(200, 'b'),
  Promise.reject(Error('bad things happened')),
  delay(50, 'c'),
  delay(1000, 'd')
])

Time

This took me about 10 minutes. If someone already has experience with Promises, I would expect someone could come up with a working solution in less than 30 minutes. If you've never seen Promises before, maybe 60 minutes?


EDIT

Re-reading the question, I see that my original answer is an actual polyfill of Promise.all, not what the interview asked of you.

Here's a dramatically simplified function that is essentially useless except for answering the interview question.

// Promise.all wannabe
// ([Promise], (bool-> void)) -> void
function all(promises, callback) {
  var count = promises.length
  promises.forEach(function(p, i) {
    p.then(
      function() { if (--count === 0) callback(true) },
      function() { callback(false) }
    )
  })
}

// delay helper for creating promises that resolve after ms milliseconds
function delay(ms, value) {
  return new Promise(function(pass) {
    setTimeout(pass, ms, value)
  })
}

// basic boilerplate to check an answer
function checkAnswer(label, promises) {
  all(promises, function(result) {
    console.log(label, result)
  })
}

// resolved promises wait for one another but ensure order is kept
checkAnswer('example1', [
  delay(100, 'a'),
  delay(200, 'b'),
  delay(50, 'c'),
  delay(1000, 'd')
])

// check that error rejects asap
checkAnswer('example2', [
  delay(100, 'a'),
  delay(200, 'b'),
  Promise.reject(Error('bad things happened')),
  delay(50, 'c'),
  delay(1000, 'd')
])

Implementation

So in hindsight, I do have some more critique to offer. Considering the function only has to return true or false, there's no reason to make it complex. Basically you just have to count the resolve branches until it reaches the count of promises provided as input. If a reject happens, you can immediately return false. There's no need for any other code.

\$\endgroup\$
  • \$\begingroup\$ Now that I see your code, It's so obvious that I got a little carried away when coding it. The biggest thing I missed (in my opinion) is that I am handling reject() when I should of just stopped there. \$\endgroup\$ – 481b8423202598ecfb233c5fa68caf Jul 8 '16 at 10:25
2
\$\begingroup\$

1 thing really caught my eyes:

if (promisesArray.filter(value => !value).length === 0) { return true; }
return false;

Returning a boolean based on a if statement is just a no-no, especially in an interview question.

Any of these is better:

    var done = promisesArray.filter(value => !value).length === 0;
    return done;

or

    return (promisesArray.filter(value => !value).length === 0);

or

    return !promisesArray.filter(value => !value).length
\$\endgroup\$
  • \$\begingroup\$ I agree with you that I should of just returned the value of the comparison (like I did on the previous function (allPromisesChecked()), can't remember why I did that. But can you expand a little on why is this frowned upon at an interview ? Code length? Readability? Shows a deeper understanding of the language? \$\endgroup\$ – 481b8423202598ecfb233c5fa68caf Jul 8 '16 at 16:58
  • 1
    \$\begingroup\$ It shows a possible lack of understanding what booleans are and what return does \$\endgroup\$ – konijn Jul 8 '16 at 18:06

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.