CODEWITHSUNDEEP
javascriptasynchronouscallbackpromise
bosticko
bosticko

Reputation: 833

Flow of logic when using promises

contrived example

Say we need to construct a Foo and Bar for a User in some Node application. We are expected to implement the constructFooForUser function, which must invoke some callback function with either

To do this, we must make use of some database functions which fetch/create our objects, returning a Promise.

var constructFooForUser = function(userId, data, callback) {

  db.users.find(userId)
    .then(function(user) {
      if (!user) { return callback(new Error('user not found')); }

      db.foos.create(user, data)
        .then(function(foo) {

          db.bars.create(user, foo, data)
            .then(function(bar) {

              callback(null, foo, bar);

            })
            .catch(function(err) {
              callback(err);
            });

        })
        .catch(function(err) {
          callback(err);
        });

    })
    .catch(function(err) {
      callback(err);
    });

};

Is this the correct way to structure this type of promise based code?

I've seen examples of Promise code which looks like

doSomething()
  .then(doSomethingElse)
  .then(doYetAnotherThing)
  .then(doLastThing)
  .catch(handleError);

but I don't think this works in this scenario, as I need user, foo, and bar at the same time, and scope isn't shared between the chained functions.

I ask because the code looks repetitive, so I am wondering if something is unusual.

EDIT: Forgot to include foo in the creation of bar

Upvotes: 3

Views: 77

Answers (3)

Thomas
Thomas

Reputation: 12677

foo and bar don't seem to depend on each other, so we could make this part paralell:

var constructFooForUser = function(userId, data, callback) {
    db.users.find(userId).then(function(user) {
        //because rejecting a promise means nothing else than throwing an Error
        if(!user) throw new Error('user not found');

        //resolves as soon as both requests have been resolved. 
        //rejects if _any_ of the requests is rejecting
        return Promise.all([
            db.foos.create(user, data),
            db.foos.create(user, data)
        ]);
    }).then(
        ([foo, bar]) => callback(null, foo, bar), //utilizing array destructuring
        (err) => callback(err)
    );
}

I'm using .then(a, b) instead of .then(a).catch(b), because the latter would also catch Errors that are thrown in your callback-function (and we don't want that); that's your responsibility to deal with them. It would hide bugs in your code, by error-messages simply not showing up in the console.

Even your attempt with the nested promises could be streamlined by returning the promises and concatenating at least the error-part

var constructFooForUser = function(userId, data, callback) {
  db.users.find(userId)
    .then(function(user) {
      if (!user) throw new Error('user not found');

      return db.foos.create(user, data)
        .then( foo => db.bars.create(user, data).then( bar => callback(null, foo, bar) ) );
    })
    .catch(function(err) {
      callback(err);
    });
};

Upvotes: 0

Jared Smith
Jared Smith

Reputation: 22040

I typically (and yes I consider this a hacky workaround) accumulate the results in an array as I go. 

firstThing()
  .then(first => Promise.all([first, secondThing]))
  .then(([first, second]) => Promise.all([first, second, thirdThing()]))
  .catch(e => handleErr(e));

This accumulation pattern works, and with ES 2015 destructuring its not too clunky.

Upvotes: 1

Sirko
Sirko

Reputation: 74106

How about storing foo in some variable until we need it?

var constructFooForUser = function(userId, data, callback) {

  var gFoo;

  db.users.find(userId)
    .then(function(user) {
      if (!user) { return callback(new Error('user not found')); }

      return db.foos.create(user, data)
    })
    .then(function(foo) {
      gFoo = foo;

      return db.bars.create(user, data)
    })
    .then(function(bar) {
      callback(null, gFoo, bar);
    })
    .catch(function(err) {
      callback(err);
    });

};

Upvotes: 1

Related Questions