3
\$\begingroup\$

I have a MEAN Stack app in which I have been using mongoose routes to update my data. Some of those routes have a lot of back and forth in terms of updating data across multiple users and collections. So I used the standard mongoose callbacks to handle them. It all worked perfectly but it turned into Callback Hell! So I turned towards using promises.

My Question:

Although my new code is still quite long but I think its much easier to manage and maintain. What do you think about my move from callback to promises? Have I implemented it right? Is there anything else I can do to improve it?

Old CallBack Code:

(as you can see its callback hell and very confusing)

app.post('/bios/approve', isLoggedIn, function(req, res) {

if (req.user.accessLevel.indexOf('Bio Officer UK') > -1) {

Bio.findOneAndUpdate({userID: req.body.userID, bioForSector: req.body.bioForSector }, {
    background: req.body.background,
    experience: req.body.experience,
    skills: req.body.skills,
    otherSkills: req.body.otherSkills,
    bioStatus: req.body.bioStatus,
    approvalStage: "Bio Designer"

}, function(err, editedBio) {

    if (err) {console.log("Error while editing Pending Bio is " + err);} 

    else if (editedBio) {
        User.findOneAndUpdate({
            accessLevel: "Bio Designer"
        }, {
            $push: {
                biosPending: editedBio._id
            }
        }, function(err, user) {
            if (err) {
                console.log("The error while finding lineManager is " + err);
            } else if (user) {
                User.findOneAndUpdate({
                    accessLevel: "Bio Officer UK"
                }, {
                    $pull: {
                        biosPending: editedBio._id
                    }
                }, function(err, bioOfficer) {
                    if (err) {
                        console.log("The error while finding lineManager is " + err);
                    } else if (!bioOfficer) {
                        req.flash('addBioMessage', 'An error occured while trying to approve this bio, please contact the system admin and provide them this error code: ER2503J');
                        return (res.json({
                            message: "An error occured while trying to approve this bio, please contact the system admin and provide them this error code: ER2503J"
                        }));
                    } else if (bioOfficer) {

                        var data = {
                            from: '',
                            to: user.email,
                            subject: 'KB - Bio Pending',
                            text: 'Hi ' + user.firstName + '\n This email is to inform you that a user has added their bio to the knowledge Base \n\n' +
                                'Please login to the system & view and approve the bio so it can be finalised.\n\n' +
                                'Thank you!\n'
                        };

                        mailgun.messages().send(data, function(error, body) {
                            console.log(body);
                            console.log("sending bio added email to bio officer");
                            req.flash('successMessage', 'Bio approved & moved on to the next stage to the Bio Designer.');
                            return (res.json({
                                message: "Bio approved & moved on to the next stage."
                            }));
                        });

                    }
                });
            } else if (!user) {
                req.flash('addBioMessage', 'An error occured while trying to approve this bio, please contact the system admin and provide them this error code: ER2503I');
                return (res.json({
                    message: "An error occured while trying to approve this bio, please contact the system admin and provide them this error code: ER2503I"
                }));
            }
        });

    }
});

 }

New Promises Code:

if(req.user.accessLevel.indexOf('Bio Officer UK') > -1){

            return _findBio(req.body)
                .then(function(bio) {
                    return _findBioDesigner(bio._id)
                })
                .then(function(designer){
                    _sendEmail(designer, req.body);
                    return _findBio(req.body)
                })
                .then(function(bio){
                    return _findBioOfficer(bio._id)
                })
                .then(function(officer){
                    req.flash('successMessage', 'Bio approved & moved on to the next stage to the Bio Designer.');
                    return res.status(200).json()
                 })
                .catch(function(err){
                    if(err)
                    req.flash('addBioMessage', 'An error occured while trying to approve this bio, please contact the system admin and provide them this error code: ER2503I');
                    return res.status(500).json(null)
                })
        }

Functions returning Promises:

// Find and Update
function _findBio(bio){
    return Bio.findOneAndUpdate({userID: bio.userID, bioForSector: bio.bioForSector},{
                background: bio.background,
                experience: bio.experience,
                skills: bio.skills,
                otherSkills: bio.otherSkills,
                bioStatus: bio.bioStatus,
                approvalStage: "Bio Designer"
            }).exec()
};

// Find and Update
function _findBioDesigner(bio){
    return User.findOneAndUpdate({accessLevel: "Bio Designer"},{$push:{biosPending: bio}}).exec()
};

// Find and Update
function _findBioOfficer(bio){
    return User.findOneAndUpdate({accessLevel: "Bio Officer UK"},{$pull:{biosPending: bio}}).exec()
}

// Inform User/Send Email
function _sendEmail(designer, bio){
    var data = {    from: '',
                    to: designer.email,
                    subject: 'KB - Bio Pending',
                    text: 'Hi ' + designer.firstName + '\n This email is to inform you that ' + bio.firstName + ' ' + bio.lastName + ' has added their bio to the knowledge Base \n\n' +
                                'Please login to the System & view and approve the bio so it can be finalised.\n\n' +
                                'Thank you!\n'
                    };

                    mailgun.messages().send(data, function(error, body) {

                    });
}
\$\endgroup\$
2
\$\begingroup\$

Assuming User and Bio functions all return promises, yes, looks about right for the most part.


_sendEmail looks like a one-off operation that doesn't appear to be part of the chain. Its errors won't get caught and will make it hard to debug. To make it part of the promise chain while still calling it parallel, use Promise.all together with _findBio. Promise.all accepts an array of promises, and resolves with an array of resolved values in the same order.

.then(function(designer){
  return Promise.all([_sendEmail(designer, req.body), _findBio(req.body)]);
})
.then(function(values){
  const email = value[0];
  const bio = value[1];
  return _findBioOfficer(bio._id)
})

_sendEmail also doesn't return a promise. You can easily wrap it in one.

return new Promise((resolve, reject) => {
  mailgun.messages().send(data, function(error, body) {
    if(error) reject(error);
    else resolve(body);
  });
});

text: 'Hi ' + designer.firstName + '\n This email is to inform you that ' + bio.firstName + ' ' + bio.lastName + ' has added their bio to the knowledge Base \n\n' +
  'Please login to the System & view and approve the bio so it can be finalised.\n\n' +
  'Thank you!\n'

If you're using Node 6+, you can use template literals.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for the answer. Yes both User & Bio return promises. Mongoose allows us to plugin different promise libraries see here. In my code after require('mongoose) I have the following line mongoose.Promise = Promise, this plugs the native ES6 promises into mongoose and exec() returns a promise. I will be making the improvements you have pointed out in my code. Thank you so much. In your opinion the way this code is structured would it suitable for a Production environment? \$\endgroup\$ – Lorenzo von Matterhorn Aug 10 '16 at 16:50
  • 1
    \$\begingroup\$ if OP is using a newer version of Node, he can not only use template literals but also array destructuring; .then(function (values) can become .then(([email, bio]) => _findBioOfficer(bio._id)) or even .then(([_, { id }]) => _findBioOfficer(id)) if you want to be really terse \$\endgroup\$ – Dan Pantry Sep 9 '16 at 18:10

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.