1
\$\begingroup\$
  • Is it a bad idea to use the notfound callback like this? If so, why not? (bad scope/closure? I'm a noob node.js dev.)
  • Is it a bad idea to use the saveCallback callback like this? I have a feeling it's dangerous, but I don't want to declare saveCallback before queryCallback as save happens after query.
create: function(registerDetails, found, notfound) {

    var query = db.User
        .findOne({ username: registerDetails.username });

    var queryCallback = function(err, user) {
        if (user != null) found(user);
        else {
            var newUser = new db.User();
            newUser.username = registerDetails.username;
            newUser.password = registerDetails.password;
            newUser.save(saveCallback);
        }
    };

    var saveCallback = function(err, user) {
        notfound(user);
    };

    query.exec(queryCallback);
},

What's a more elegant way of writing this so that

// 1) don't nest like crazy.
.exec(function(){
    if () {
        newUser.save(function(){
            notfound(); // really? because we know node.js will ever chain 2 callbacks max. </sarcasm>
        }
    }

// 2) don't make people read "down and back up"
var saveCallback = function() {
    // Dejavu... I've been here before...
};
var queryCallback = function() {
    saveCallback(); // Back from last line, ohhhhh this is what saveCallback used for. Go back up.
};
query.exec(queryCallback); // Ohhhhh so that's what it's used for. Now go back up and read what it does.
\$\endgroup\$
1
\$\begingroup\$

Interesting question,

I would go with this:

create: function (registerDetails, found, notfound) {

    db.User.findOne({
        username: registerDetails.username
    }).exec(queryCallback);

    function queryCallback(err, user) {
        if (user != null) {
            found(user);
        } else {
            var newUser = new db.User();
            newUser.username = registerDetails.username;
            newUser.password = registerDetails.password;
            newUser.save(saveCallback);
        }
    };

    function saveCallback(err, user) {
        notfound(user);
    };
},

Basically, I

  • Used the fact that JavaScript does hoisting, you can use functions prior to having them declared. Given this I put the .exec() on top, I also did away with query which I would have called userQuery.
  • I put proper braces and newlines around if( user!= null ), to be tested, but if you can get away with it, you should consider if(user)
  • I moved from var xx = function() to function xx() because anonymous functions in stacktraces are not fun
\$\endgroup\$
  • \$\begingroup\$ Let me read through about hoisting first. Much thanks for the input. It's interesting because that's the exact reason I put the query.exec at the end. If it's a standard practice doing then there's no harm putting it at top. \$\endgroup\$ – Sleeper Smith May 12 '14 at 14:14
  • \$\begingroup\$ You know what. I'm marking this an answer. From your link "The reason scoping is so confusing in JavaScript is because it looks like a C-family language." <- exactly what I was looking for. I am a C# developer, and I find JS scope confusing to say the least. \$\endgroup\$ – Sleeper Smith May 12 '14 at 14:16
  • \$\begingroup\$ You could get rid of the else by doing if ( user ) return found(user); But typically your errors are handled first, so I would do if (!user) return saveNewUser(user); then just drop into the found(user) below that. \$\endgroup\$ – chovy May 27 '14 at 3:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.