HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Verifying requirements before deleting a user-parent-student-school relationship

Submitted by: @import:stackexchange-codereview··
0
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:

  • 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_id is 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:

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.