CODEWITHSUNDEEP
javascriptnode.jsmongoosepromise
Nite
Nite

Reputation: 373

NodeJS+Mongoose - Need help Refactor code using Promises

Consider this example :

router.post('/', function (req, res, next) {

var order = new Order({

    customer_id: req.body.customer_id,
    order_elements: []
});

order.save(function(err, saved_order) {

    var elementsSaveCount = 0;

    req.body.order_elements.forEach( function(order_element, index) {

        orderElement = new OrderElement({

            order_id: saved_order._id,
            name: order_element.name
        });

        orderElement.save(function(err, result) {

            elementsSaveCount++;
            order.order_elements.push(result._id);

            if(elementsSaveCount >= req.body.order_elements.length) {

                saved_order.save();
                res.status(201).json({
                message: 'order saved',
                order: order
                });
            }
        });
    });
});
});

The 'Order' model has an array of id of 'OrderElements'. First I'm saving the Order, then inside the callback of the save() I loop through the OrderElements of the request to save them as well and push each id inside the Order. I use the variable 'elementsSaveCount' to check when all the save() are executed.

I'd like to refactor this code using promises.

Upvotes: 1

Views: 141

Answers (2)

TylerFowler
TylerFowler

Reputation: 568

This isn't strictly what I would do if I were actually refactoring the given code sample but I'll attempt to illustrate a couple different ways Promises will help us refactor the handler:

router.post('/', (req, res, next) => {
  const saveOrdersWithParent = parentOrderId => req.body.order_elements
  .map(order_element =>
    new OrderElement({
      order_id: parentOrderId, name: order_element.name
    }).save()
  );

  return new Order({
    customer_id: req.body.customer_id,
    order_elements: []
  }).save()
  .then(saved_order =>
    Promise.all(saveOrdersWithParent(saved_order._id))
    // if you want to catch invidiually failed items separately,
    // otherwise omit this and the next catch will get it
    .catch(err => {
      console.error('Some individual order element could not be saved', err);

      // Important: we need to "bubble up" the error if we want to handle it
      // specially or else the next 'then' will go ahead and run but of
      // course it's `orders` param will be undefined.
      // Usually avoid this situation but it's good to know
      throw err;
    }).then(orders => {
      // assuming you wanted this to be persisted to the database
      // and not just the local representation
      saved_order.order_elements = orders.map(o => o._id);
      return saved_order.save();
    })
  ).then(order => res.status(201).json({ message: 'order saved', order }))
  .catch(err => console.error('Original order could not be saved', err));
});

First of all, the nice thing about Promises is that they're super easy to compose since you can easily treat them as values. So you'll notice I broke out a somewhat messy bit of logic (creating and saving order elements) into a helper function that returns an array of Promises. Not super necessary but it's a good tool to help clean up promise chains. Something that's really key when using Promises to make complex async code clearer is making the chains themselves as bare as possible, strive for something like this: createPost().then(persistPost).then(notifyUsersOfPost).then(etc) where it's really clear what's going on at a high level and hides the details. You can also do this with callbacks but it's significantly harder.

Promise.all can be used to wait until all of the order elements have been successfully saved, removing the need for a counter. If any of the order elements were not successfully saved it will skip directly to that catch block where you could potentially handle the case where some order elements didn't save. Only really do this if you can recover somehow, maybe by retrying (see the note in the code sample).

So we do have some nesting here, like with callbacks, but much less of it. Generally you want to operate in nested promises as a way of showing "lifecycles" of objects. In this example the nested promises (saving order elements) require a reference of the originally saved order so we can pretty clearly see what parts of code depend on that (intermediate) value. The moment we don't need that saved_order value we kick back up to the top level promise chain. An equally valid thing to do might've been this:

.then(saved_order => {
  const orderSaves = saveOrdersWithParent(saved_order._id)
    .catch(err => {
      console.error('Some individual ...', err);
      throw err;
    })
  return Promise.all([ saved_order, orderSaves... ]);
}).then(([ saved_order, ...orders ]) => {
  saved_oder.order_elements = orders.map(o => o._id);
  return saved_order.save()
})
...

This avoids the nesting by including value we'll need (persisted Order object) alongside the saved order elements but IMO is usually less clear.

I think this particular example does a good job of showing how to create "pipelines" of operations that are blocked out by what inputs a thing needs and what it produces. The win there being clarity on the steps of the logic and what depends on what.

Upvotes: 1

David Vicente
David Vicente

Reputation: 3111

This approach could be a beggining:

router.post('/', function (req, res, next) {

    var order = new Order({
        customer_id: req.body.customer_id,
        order_elements: []
    });

    order.save()
    .then(saved_order => {
        var elementsSaveCount = 0;
        var listPromises = [];
        for(let order_element of req.body.order_elements) {
            let orderElement = new OrderElement({
                order_id: saved_order._id,
                name: order_element.name
            });

            listPromises.push(orderElement.save());
        }

        return Promise.all(listPromises);
    })
    .then(allOrderElementsSaved => {
        allOrderElementsSaved.map(orderElementSaved => {
            saved_order.order_elements.push(orderElementSaved._id);  
        });

        return saved_order.save();
    })
    .then(saved_order => {
        res.status(201).json({
            message: 'order saved',
            order: saved_order.toObject()
        });
    });
});

Upvotes: 0

Related Questions