1

I have three resources Hotel, Address and Image. A hotel can have may images but only one address. While showing hotel details I need to fetch its address as well as all images of it so, in my controller the following code goes:

function getHotel(req, res) {
  const { hotelId, merchantId } = req.params;
  Hotel.search(req, [['id', '=', hotelId], ['merchant_id', '=', merchantId]])
      .then(function then_(hotel) {
        Hotel.searchAddress(req, hotelId).then(function then_(address) {
          hotel.address = address;
          Hotel.searchImages(req, hotelId).then(function (images) {
            hotel.images = images;
            res.status(200).json({ hotel });
          });
        });
      }).catch(function catch_(err) {
        console.log(err);
        res.status(500).send();
      });
}

It can be seen that the SQL queries are inside the static class methods but the more painful concern is that the code seems procedural to me. I know there is a Repository pattern but I haven't used it much yet. How can I make the given code more OOP and flexible?

  • 1
    I'd consider moving the Hotel.searchAddress and Hotel.searchImages calls out to the scope of getHotel, and have them write to a local. Then all the promises can be passed to Promise.all(), which assembles the response. At the moment, you just have a linear sequence of steps, wrapped up in an arrowhead – Caleth Jul 11 '17 at 11:34
  • @Caleth: sounds fine, why not make an answer from your comment? – Doc Brown Jul 11 '17 at 12:20
2

I'd consider moving the Hotel.searchAddress and Hotel.searchImages calls to the same level. Then all the promises can be passed to Promise.all(), which assembles the response.

This is possible because none of your steps depends on data from the outer promises. Where there is a data dependancy, you can nest only the dependant calls as in your example, which will minimise the depth of callbacks.

e.g.

function getHotel(req, res) {
  const { hotelId, merchantId } = req.params;
  Promise.all([
      Hotel.search(req, [['id', '=', hotelId], ['merchant_id', '=', merchantId]]), 
      Hotel.searchAddress(req, hotelId),
      Hotel.searchImages(req, hotelId)
    ]).then(function (values) {
        const { hotel, address, images } = values; 
        hotel.address = address;
        hotel.images = images;
        res.status(200).json({ hotel });
    }).catch(function (err) {
        console.log(err);
        res.status(500).send();
    });
}
  • There may be the case that the hotelId is invalid. I think fist we need to fetch hotel and then address and images in parallel. – CodeYogi Jul 11 '17 at 13:11
  • If hotelId is invalid, then at least one of the promises will return an error, and the error status will be returned. We can then ignore the other two values. The point is not to minimise how much we search for, but to minimise how long we have to wait for our searching to finish – Caleth Jul 11 '17 at 13:51
  • Yes, but the issue if Hotel.search returns null i.e. no record found then also other two searches will be performed which is not wanted. wdyt? – CodeYogi Jul 11 '17 at 17:44
  • 1
    from the docs : "The Promise.all() method returns a single Promise that resolves when all of the promises in the iterable argument have resolved [...]. It rejects with the reason of the first promise that rejects." – Caleth Jul 13 '17 at 13:57
  • 1
    Promises run asynchronously. That's not required to be parallel, but it is likely to be, for things like database accesses. – Caleth Jul 13 '17 at 14:32

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.