Problem
I am new to node, after I finished my APIs I realized that all of them are a mess, and are a callback hell, this forced me to learn about promises, now it was all good until I faced an API that has condition with more than possible function, my question is how to do nested promises, my code is about a parent object who has a user attached to it, when delete route is called there are many conditions:
- If this parent has children attached to it it shouldn’t be deleted
- If there are no children,
- This parent is in more than one school, the
school_idis removed from the school id of the parent object and the user object attached to it - If this parent has no children and only one school_id it should be removed and the user attached also should be removed
- This parent is in more than one school, the
The code looks like this and it’s 100% functional. I am trying to apply promises and make my code better.
router.post('/delete',Validation, function (req, res) {
var school_id = req.body.schoolId;
var parent_id = req.body.selected[0];
Student.findOne({parent_ids:parent_id},function(err,parentF){
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return
}else if(parentF){
res.json({status:"error", message:"you can not delete a parent who has students associated with it"});
return;
}else{
Parent.findOne({_id:parent_id},function(err,parent){
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return;
}else{
if(parent.school_id.length>1){
var a = parent.school_id.indexOf(school_id);
parent.school_id.pop(a);
parent.save(function(err,parent){
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return;
}else{
User.findOne({refid:parent_id},function(err,user){
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return;
}else {
user.school_id.pop(a);
user.save(function(err) {
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return;
}else{
res.json({status: "success", message: "parent removed"});
return;
}
});
}
});
}
});
}else{
Parent.remove({_id: parent_id}, function (err) {
if (err) {
res.json({status: "error", message: err.message});
} else {
User.remove({refid:parent_id},function(err){
if (err) {
res.json({status: "error", message: "parent user wasn't deleted"});
return;
}else{
res.json({status: "success", message: "parent data successfully deleted"});
return;
}
});
}
});
}
}
});
}
});
});
Solution
One of the biggest things you can do to clean up you code is to simply remove the unnecessary if-else blocks. In many cases you have a return inside the first conditional, which means there is no reason for an else block at all.
For example, you code rewrite like this:
Student.findOne({parent_ids:parent_id},function(err,parentF){
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return
}
if(parentF){
res.json({status:"error", message:"you can not delete a parent who has students associated with it"});
return;
}
Parent.findOne({_id:parent_id},function(err,parent){
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return;
}
if(parent.school_id.length>1){
var a = parent.school_id.indexOf(school_id);
parent.school_id.pop(a);
parent.save(function(err,parent){
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return;
}
User.findOne({refid:parent_id},function(err,user){
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return;
}
user.school_id.pop(a);
user.save(function(err) {
if(err){
console.log(err);
res.json({status:"error",message:"an error occurred"});
return;
}
res.json({status: "success", message: "parent removed"});
return;
});
});
});
} else {
Parent.remove({_id: parent_id}, function (err) {
if (err) {
res.json({status: "error", message: err.message});
} else {
User.remove({refid:parent_id},function(err){
if (err) {
res.json({status: "error", message: "parent user wasn't deleted"});
return;
}
res.json({status: "success", message: "parent data successfully deleted"});
return;
});
}
});
}
});
});
Hmm… half my post went away 🙁
I had also mentioned that you should look into naming your functions as a way to get away from the nesting that can be proliferated by anonymous functions. You could add named functions for handling you error and success logging/response formation. You could also add named functions to perform the callbacks, so as to flatten these callbacks out as well as provide re-use.
Finally, I think you have some style problems:
- inconsistent use of semicolons at end of line
- inconsistent spacing around flow control structures
- inconsistent spacing around commas in function arguments and object literals
- inconsistent use of camelCase vs. snake_case (you should probably just stick to camel casing in JS)
You should consider defining a style and enforcing it.
Rewrite
Despite you explicitly asked for Promises, I’ll show you my solution without nested callbacks. It was made simply by extracting those callbacks to standalone functions and applying few “best practices” to the code.
Before the code itself, few remarks about it:
- Just like your code, it assumes availability of these 5 variables or objects:
Parent,User,router,ValidationandStudent. - It uses minimum of ES6, but it can be easily translated back to the previous version of specification. It can be even done automatically by transpiler such as Babel.
- Its scope should be limited. In ES6 it could be done by creating the block scope by simply enclosing the entire code with curly brackets. More about it in ES6 Block Scope is The new IIFE by Wes Bos.
Quoting Linus Torvalds:
Talk is cheap. Show me the code.
let parentId, schoolId, res, schoolIndex;
const messages = {
generalError: 'an error occurred',
studentFindOne: 'you can not delete a parent who has students associated with it',
userSave: 'parent removed',
userRemoveError: "parent user wasn't deleted",
userRemove: 'parent data successfully deleted'
};
const logError = err => {
console.log(err);
res.json({status: 'error', message: messages.generalError});
};
const studentFindOneCallback = (err, parentF) => {
if (err) {
logError(err);
}
else if (parentF) {
res.json({status: 'error', message: messages.studentFindOne});
}
else {
Parent.findOne({_id: parentId}, parentFindOneCallback);
}
};
const parentFindOneCallback = (err, parent) => {
if (err) {
logError(err);
}
else if (parent.schoolId.length > 1) {
schoolIndex = parent.schoolId.indexOf(schoolId);
parent.schoolId.pop(schoolIndex);
parent.save(parentSaveCallback);
}
else {
Parent.remove({_id: parentId}, parentRemoveCallback);
}
};
const parentSaveCallback = err => {
if (err) {
return logError(err);
}
User.findOne({refid: parentId}, userFindOneCallback);
};
const userFindOneCallback = (err, user) => {
if (err) {
return logError(err);
}
user.schoolId.pop(schoolIndex);
user.save(userSaveCallback);
};
const userSaveCallback = err => {
if (err) {
return logError(err);
}
res.json({status: 'success', message: messages.userSave});
};
const parentRemoveCallback = err => {
if (err) {
return res.json({status: 'error', message: err.message});
}
User.remove({refid: parentId}, userRemoveCallback);
};
const userRemoveCallback = err => {
return res.json({
status: err ? 'error' : 'success',
message: err ? messages.userRemove : messages.userRemoveError
});
};
router.post('/delete', Validation, (req, response) => {
schoolId = req.body.schoolId;
parentId = req.body.selected[0];
res = response;
Student.findOne({parentIds: parentId}, studentFindOneCallback);
});
Remarks about your code
Firstly, it’s spaghetti code that is hard to read. The rest of the remarks seem to be each irrelevant, but they altogether make for why your code is so untidy and unpleasant. Just like in Lingchi, each of the cuts doesn’t contribute much to the final result, but altogether they are game-changing.
-
Indentation problems:
} }); }); -
Inconsistent quotation method choice:
'/delete'vs."error". I myself prefer to use single quotes unless that would force me to perform escaping, because they introduce lesser cognitive load than the double quotes. -
Inconsistent and wrong spacing:
'/delete',Validationinstead of'/delete', Validationfunction (instead offunction(){instead of) {}else{instead of} else {
- Inconsistent character case:
school_idvs.findOne. In JS generally camelCase should be used, rather than snake_case. - Inconsistent sticking to selected philosophy of ending lines with a semicolon or not.
Most of those problems could be removed with static analysis tools and beautifiers.
Personally, I decided to use the magic of shadow variables.
When you use the pattern:
module.exports = (args, callback) => {
var uri = `${baseUrl}/something/${args.somethingElse}/more?access_token=${args.even_more_args}`;
let options = {
uri: uri,
method: 'POST',
json: {
param_a : args.this_function_takes_tons,
param_b : args.and_tons,
param_c: args.of_arguments,
param_d: args.and_i_need_something_it_gives_back
}
};
request(options, (err, response) => {
args.i_need_this_back = response.body;
return callback(args);
}); };
and place your return values inside of your input argument, the callback hell chain goes from a zig-zag dependency chain pile of “Oh god, how will I ever pass that one argument in to that 5 tiers deep callback” to
args = {};
args['client_id'] ='something';
args['username'] = config.get('something');
args['password'] = config.get('something_else');
getSomethingA(args, (result) => {
getSomethingB(args, (result) => {
getSomethingC(args, (result) => {
getSomethingD(args, (result) => {
getSomethingE(args, (result) => {
getSomethingF(args, (result) => {
getSomethingG(args, (result) => {
getSomethingH(args, (result) => {
createSomething(args, (result) => {
console.log(args.that_one_thing_i_needed);
});
});
});
});
});
});
});
});
});