Deleting rooms using nested promises

Question

I have a piece of code that has to deal with some nested promises and shared variables between them. This code is complicated by itself and because of promises syntax is even more difficult to read for those that haven't written it.

The code right now is something like this:

const _deleteOneToOneRooms = function(userId, teamMembers) {
  return function() {
    return User.findOne({ _id: userId })
    .then((userDeleted) => {
      return Promise.all(teamMembers.map((member) => {
        return User.findOne({ _id: member }).then((user) => {
          return {
            id: user._id,
            commonTeams: Utils.arrayIntersect(user.teams, userDeleted.teams).length !== 0
          };
        });
      }))
      .then((usersNoCommonTeam) => {
        return Promise.all(usersNoCommonTeam.map((user) => {
          if(user.commonTeams) {
            return Promise.resolve();
          }
          return Room.findOneAndRemove(
            {
              $and: [
                { type: 'one-to-one' },
                { owners: { $all: [user.id, userDeleted] } }
              ]
            });
        }));
      });
    })
    .then((roomsToDelete) => {
      return Promise.all(roomsToDelete.map((room) => {
        if(room) {
          return Membership.remove({ room: room._id });
        }
        return Promise.resolve();
      }));
    });
  };
};

someDatabaseLookups.then(_deleteOneToOneRooms(userId, teamMembers))

I have considered to split each then into functions so the overall code is simpler to read:

const _deleteOneToOneRooms = function(userId, teamMembers) {
  return function() {
     return User.findOne({ _id: userId })
     .then((deletedUser) => {
       return _getUsersWithNoCommonTeams(deletedUser, teamMembers);
     })
     .then((usersNoCommonTeam) => {
       return _getOneToOneRooms(userId, usersNoCommonTeam);
     })
     .then(_removeRooms);
  };
};

But I don't like having one-line functions only to give them names and at the end if you are reading the code for the first time, you will have to scroll up and down to know what is being done, which from my point of view makes it harder to understand.

So my question is, is there a convention on how to write the cleanest code when dealing with complicated nested promises that use shared variables?

Answer 1

If you have the possibility to use it, you could try the async / await pattern. It is however only part of the standard since ecmascript 2017.

The pattern allows you to wait for promise before continuing the execution path. This means you could rewrite your current code in the following syntax

// mock
const User = {
  findOne: async function( param ) {
    return new Promise(resolve => setTimeout(() => resolve( param ), 500));
  }
};

const _deleteOneToOneRooms = function(userId, teamMembers) {
  return async function() {
    let user = await User.findOne({ _id: userId });
    for (const member of teamMembers) {
      let foundMember = await User.findOne({ _id: member });
      // do something with that member
    }
  };
};

const removeUser = async function() {
  return _deleteOneToOneRooms( 5, [10, 15, 16] )().then( () => console.log('done') );
};

// an async function returns a promise in itself, so from outside an async function, you can use the `then` syntax
removeUser().then(() => console.log('async also done'));
// this will be logged first
console.log('remove user was called, rest is async');

This only gives you an idea from how you could reformat your code using this pattern.

The last piece of code can then also be rewritten like:

const _deleteOneToOneRooms = function(userId, teamMembers) {
  return async function() {
     let deletedUser = await User.findOne({ _id: userId });
     let usersNoCommonTeam = await _getUsersWithNoCommonTeams(deletedUser, teamMembers);
     return await _getOneToOneRooms(userId, usersNoCommonTeam);
  };
};

Answer 2

You should also avoid multiple hits to the DB. If possible, alter your query so that you only issue one operation to delete multiple things. Let the DB do that work, not your code. This way, you also remove the Promise.all portions of the code. For instance, the following pieces are candidates for a bulk query operation:

return Promise.all(teamMemberIds.map(teamMemberId => {
  return User.findOne({ _id: teamMemberId }).then(user => ({
    id: user._id,
    commonTeams: Utils.arrayIntersect(user.teams, deletedUser.teams).length !== 0
  }))
})

return Promise.all(teamMembers.map(teamMember => {
  return teamMember.commonTeams ? Promise.resolve() : Room.findOneAndRemove({
    $and: [
      { type: 'one-to-one' },
      { owners: { $all: [teamMember.id, deletedUser] } }
    ]
  })
})

return Promise.all(roomsToDelete.map(room => {
  return room ? Membership.remove({ room: room._id }) : Promise.resolve()
}))

I'm no mongo expert so you'll have to do the research.

Now to nitpick your code even more, arrays should be in plural form. When you do array operations (i.e. map, etc.), the array item reference should be named the same except in singular form. Not doing it this way introduces ambiguity, especially when you call things different names in different contexts (i.e. a "team member" is also a "member", a "user").

Also, you could avoid visual noise to make your code a bit readable. JS is fine without semi-colons. Parens are optional for single-arg arrow functions. Arrow functions without the block implicitly return the expression value. Ternaries for short, binary conditions.

const _deleteOneToOneRooms = function (userId, teamMemberIds) {
  return User.findOne({ _id: userId }).then(deletedUser => {
    return Promise.all(teamMemberIds.map(teamMemberId => {
      return User.findOne({ _id: teamMemberId }).then(user => ({
        id: user._id,
        commonTeams: Utils.arrayIntersect(user.teams, deletedUser.teams).length !== 0
      }))
    })).then((teamMembers) => {
      return Promise.all(teamMembers.map(teamMember => {
        return teamMember.commonTeams ? Promise.resolve() : Room.findOneAndRemove({
          $and: [
            { type: 'one-to-one' },
            { owners: { $all: [teamMember.id, deletedUser] } }
          ]
        })
      }))
    })
  }).then(roomsToDelete => {
    return Promise.all(roomsToDelete.map(roomToDelete => {
      return roomToDelete ? Membership.remove({ room: roomToDelete._id }) : Promise.resolve()
    }))
  })
}