patternjavascriptMinor
Verifying requirements before deleting a user-parent-student-school relationship
Viewed 0 times
deletingverifyingstudentuserparentschoolbeforerequirementsrelationship
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:
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;
- 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
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;
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:
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:
You should consider defining a style and enforcing it.
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.
Code Snippets
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;
});
}
});
}
});
});Context
StackExchange Code Review Q#154528, answer score: 3
Revisions (0)
No revisions yet.