readability of node.js asynchronous functions

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);

Tobias · Accepted Answer · 2014-04-27 14:59:51Z

1

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.

answered Apr 27, 2014 at 14:59

Tobias

7,7671 gold badge30 silver badges44 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

nomongo Over a year ago

So is it ok to return a callback function like below without any side effects? //return callback(err, null);

mjhm Over a year ago

@nomongo. It's perfectly fine functionally in your case. But if you think about it, you need to keep straight 3 different contexts -- the return value in the callback, the function that called the callback, and where that return value is going after the final return. This can sometimes create mysteriously hard bugs to track down, and I personally don't like to think so hard. I honestly prefer your original version with the "{...}", partly because it will pass JSLint/JSHint. Again -- I don't like to think so hard.

nomongo Over a year ago

Thanks @mjhm and @ still_learning. Eagerly waiting for node v0.12 so I could use generators..

Tobias Over a year ago

@nomongo @mjhm No asynchronous function should return a value. Assuming next does not return a value, return next(err); actually returns undefined and is therefore equivalent to next(err); return;. So yes, it is okay to use one of the short forms, whether they'll pass lint or not.

mjhm · Accepted Answer · 2014-04-27 14:56:18Z

1

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).

answered Apr 27, 2014 at 14:56

mjhm

16.8k10 gold badges47 silver badges56 bronze badges

Collectives™ on Stack Overflow

readability of node.js asynchronous functions

2 Answers 2

4 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related