4
\$\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\$
6
\$\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. A section has been added 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)
  })
}

// resolved promises wait for one another but ensure order is kept
all([
  delay(100, 'a'),
  delay(200, 'b'),
  delay(50, 'c'),
  delay(1000, 'd')
])
.then(console.log, console.error) // [ a, b, c, d ]

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

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?


using a callback

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')
]) // [ a, b, c , 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')
]) // Error: bad things happened

remarks

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.


back from the future (4 years later)

Functions have taught me a lot over the recent years. Small functions that do one thing are always better than big functions that do many. The design for this implementation starts with the simple idea of combining two promises into an array of two values. We'll call it and -

const and = (px, py) => // <-- two promises: "p of x" and "p of y"
  px.then(x => py.then(y => [ x, y ])) // two values: "x" and "y"

const all = (promises = []) =>
  promises.reduce
    ( (pr, px) =>
        and(pr, px) // <-- two promises: "p of r" and "p of x"
          .then(([ r, x ]) => // <-- two values: "r" and "x"
            [ ...r, x ]) // <-- result: append "x" to "r"
    , Promise.resolve([]) // <-- initial result, "p of empty"
    )

Already we're done implementing all and behaviour is the same -

all([
  delay(100, 'a'),
  delay(200, 'b'),
  delay(50, 'c'),
  delay(1000, 'd'),
])
.then(console.log, console.error)
// [ a, b, c, d ]

all([
  delay(100, 'a'),
  delay(200, 'b'),
  Promise.reject(Error('bad things happened')),
  delay(50, 'c'),
  delay(1000, 'd'),
])
.then(console.log, console.error)
// Error: bad things happened

Expand the snippet below to verify the result in your browser -

const and = (px, py) =>
  px.then(x => py.then(y => [ x, y ]))

const all = (promises = []) =>
  promises.reduce
    ( (pr, px) =>
        and(pr, px).then(([ r, x ]) => [ ...r, x ])
    , Promise.resolve([])
    )

const delay = (ms, x) =>
  new Promise(r => setTimeout(r, ms, x))

all([
  delay(100, 'a'),
  delay(200, 'b'),
  delay(50, 'c'),
  delay(1000, 'd'),
])
.then(console.log, console.error) // [ a, b, c, d ]

all([
  delay(100, 'a'),
  delay(200, 'b'),
  Promise.reject(Error('bad things happened')),
  delay(50, 'c'),
  delay(1000, 'd'),
])
.then(console.log, console.error) // Error: bad things happened

But we can do better. Putting x and y in an array [] is an extra "thing". And the operation we are using in reduce, [ ...r, x ] is also a well-known function -

const and = (px, py) =>
  px.then(x => py.then(y => [ x, y ])) // hard-coded []

const all = (promises = []) =>
  promises.reduce
    ( (pr, px) =>
        and(pr, px)
          .then(([ r, x ]) =>
            [ ...r, x ]) // <-- is known as "append"
    , Promise.resolve([])
    )

Instead of locking x and y in an array, [ x, y ], we could specify any behaviour we want, ie doSomething(x, y) -

const and = (doSomething, px, py) =>
  px.then(x => py.then(y => doSomething(x, y)))

const myFunc = (a, b) =>
  (a + b) * 100

and(myFunc, Promise.resolve(1), Promise.resolve(2))
  .then(console.log, console.error) // 300

This implementation of and is well known by another name. lift2 allows us to take an ordinary function (eg append) and use it in (ie "lift it into") the Promise context -

// complete polyfill for Promise.all

const lift2 = (f, px, py) =>
  px.then(x => py.then(y => f(x, y)))

const append = (a = [], x = null) =>
  [...a, x]

const all = (promises = []) =>
  promises.reduce
    ( (pr, px) => lift2(append, pr, px)
    , Promise.resolve([])
    )

Expand the snippet to verify the results in your browser -

const lift2 = (f, px, py) =>
  px.then(x => py.then(y => f(x, y)))

const append = (a = [], x = null) =>
  [...a, x]

const all = (promises = []) =>
  promises.reduce
    ( (pr, px) => lift2(append, pr, px)
    , Promise.resolve([])
    )

const delay = (ms, x) =>
  new Promise(r => setTimeout(r, ms, x))

all([
  delay(100, 'a'),
  delay(200, 'b'),
  delay(50, 'c'),
  delay(1000, 'd'),
])
.then(console.log, console.error) // [ a, b, c, d ]

all([
  delay(100, 'a'),
  delay(200, 'b'),
  Promise.reject(Error('bad things happened')),
  delay(50, 'c'),
  delay(1000, 'd'),
])
.then(console.log, console.error) // Error: bad things happened


why is it named lift2?

It's called lift2 because the function we lifted (append in this example) expects two arguments. In typed languages, it's common to see lift3 and lift4 variants -

const lift3 = (f, px, py, pz) => // <-- 3 promises
  px.then(x => py.then(y => pz.then(z => 
    f(x,y,z) // <-- 3 values
  )))

const lift4 = (f, pw, px, py, pz) => // <-- 4 promises
  pw.then(w => px.then(x => py.then(y => pz.then(z => 
    f(w,x,y,z) // <-- 4 values
  ))))
  
const p = x => // <-- helper for demo
  Promise.resolve(x) 

const add = (...numbers) =>
  numbers.reduce((a, b) => a + b, 0)

lift3(add, p(1), p(2), p(3))
  .then(console.log, console.error) // 6

lift4(add, p(1), p(2), p(3), p(4))
  .then(console.log, console.error) // 10

But JavaScript is a dynamic language and so we do not need such strict implementations. We can implement liftN which accepts a variadic function -

const append = (a = [], x = null) =>
  [...a, x]

const lift2 = f =>
  (px, py) => px.then(x => py.then(y => f(x, y)))

const liftN = (f, ...promises) =>
  promises
    .reduce(lift2(append), Promise.resolve([])) // <-- !
    .then(values => f(...values))

const p = x => // <-- helper for demo
  Promise.resolve(x) 

const add = (...numbers) =>
  numbers.reduce((a, b) => a + b, 0)

liftN(add, p(1))
  .then(console.log, console.error) // 1

liftN(add, p(1), p(2))
  .then(console.log, console.error) // 3

liftN(add, p(1), p(2), p(3))
  .then(console.log, console.error) // 6

liftN(add, p(1), p(2), p(3), p(4))
  .then(console.log, console.error) // 10

Notice ! above. liftN is essentially the variadic version of all. The two can be used interchangeably -

const all = (promises = []) =>
  promises.reduce(lift2(append), Promise.resolve([]))

const liftN = (f, ...promises) =>
  all(promises).then(values => f(...values))
all([ p(1), p(2), p(3), p(4) ])
  .then(nums => add(...nums))
  .then(console.log, console.error) // 10

liftN(add, p(1), p(2), p(3), p(4))
  .then(console.log, console.error) // 10
liftN
  ( console.log
  , delay(100, 'a')
  , delay(200, 'b')
  , delay(50, 'c')
  , delay(1000, 'd')
  )
  .catch(console.error) // [ a, b, c, d ]

liftN
  ( console.log
  , delay(100, 'a')
  , delay(200, 'b')
  , Promise.reject(Error('bad things happened'))
  , delay(50, 'c')
  , delay(1000, 'd')
  )
  .catch(console.error) // Error: bad things happened

Expand the snippet below to verify the results in your browser -

const append = (a = [], x = null) =>
  [...a, x]

const lift2 = f =>
  (px, py) => px.then(x => py.then(y => f(x, y)))

const all = (promises = []) =>
  promises.reduce(lift2(append), Promise.resolve([]))

const liftN = (f, ...promises) =>
  all(promises).then(values => f(...values))

const delay = (ms, x) =>
  new Promise(r => setTimeout(r, ms, x))

liftN
  ( console.log
  , delay(100, 'a')
  , delay(200, 'b')
  , delay(50, 'c')
  , delay(1000, 'd')
  )
  .catch(console.error) // [ a, b, c, d ]

liftN
( console.log
, delay(100, 'a')
, delay(200, 'b')
, Promise.reject(Error('bad things happened'))
, delay(50, 'c')
, delay(1000, 'd')
)
.catch(console.error) // Error: bad things happened

| improve this answer | |
\$\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
3
\$\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
| improve this answer | |
\$\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

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