Question

I understand that we should not return objects in node.js asynchronous functions and that every path inside the asynchronous function should lead to the callback function. To solve the "pyramid of doom" problem to some extent and for better readability, is it ok to just say "return;" after calling the callback function so I don't have to put the rest of the code in the else block and skip indenting and get better readability. The code has been working fine so far but just wondering if there are any potential issues I'm overlooking.

(function(database) {
    var mongodb = require("mongodb");
    database.ObjectID = mongodb.ObjectID;
    var mongoUrl = "mongodb://localhost:27017/mydb";    
    var dbconn = null;
    database.getDBConn = function(next){
        if(dbconn){ next(null, dbconn); return; } //already connected: return dbconn
        mongodb.MongoClient.connect(mongoUrl,function(err, database){
            if(err){ next(err, null); return; } //connection fail: return error 
            dbconn = {db: database,  
                      movies: database.collection("movie") }; 
            next(null, dbconn); //connection success: return dbconn
        }); 
    } 


})(module.exports);
Was it helpful?

Solution

No, there are no issues, but you can argue about the readability.

if(foo) {
  // do something
} else {
  // do something else
}

Is not much less readable than

if(foo) {
  // do something
  return;
}
// do something else

Although I personally think the first version is a better representation of the logic sequence and the only way to run code after the first and second alternative, this does not apply to your case.

It is fine to abbreviate the flow as you did. When using JavaScript, I make it even shorter:

if(err) return next(err, null);

As most callbacks should ignore data parameters if the err parameter is set, the following should be enough:

if(err) return next(err);

This is the shortest possible form and I'd prefer it over every if-else statement.

OTHER TIPS

What you have looks fine to me. You just need to make very sure that every if/else code path in every callback has a "next".

At some point you'll probably want to look at avoiding the pyramid of doom by using either an async module (more accessible) or a promises module (does more with less).

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top