A server response written in node/express/mongodb: does this look right?

Question

This is my very very very first attempt to write anything meaningful in node. The client side is a simple form that has login, workspace, and simply saves them onto the database. It would have been a 5-liner in PHP!

My question really is: does this code look sane/good? I am trying to avoid using async.js for now, just because I want to get my hands dirty a little to start with.

A few notes:

• I want to check if workspace and user names are taken first, but I want to check for BOTH in case they are taken

• There is still a slight race condition here: what two users submit different usernames and same workspace names at the same time? OK it's unlikely, but one of them will end up failing (one of them will be workspace-less). How would you address it? Simply deleting the user just created? Or...?

• There is a lot of indentation here. I know that it's a known issue with node... I cannot really use "return" as a trick to interrupt execution, because it's often used in an anonymous function context. So...

• I sometimes go next("Bad error here"). I plan on writing an error handler in Connect to handle these (I will probably log things onto the database).

Here we go...

// Helper function. The javascript on the other side calls this with Ajax, and expects
// a hash with "response" (can be OK or ERRORS) and a "errors" vector
var serverResponse = function(res, errors){
  if(errors.length){
    res.json( { response: 'ERRORS', errors: errors } , 200)
    console.log("Sent ERROR");
  } else {
    res.json( { response: 'OK' } , 200)
    console.log("Sent OK");
  }
}

// The actual call
exports.nonApiRegister = function(req, res, next){

  // ***** UNINDENTED ON PURPOSE: artificial delay
  setTimeout(function(){
  // *****

  var errors = [];
  var Workspace = mongoose.model("Workspace");
  var User = mongoose.model("User");

  User.findOne( { login: req.body.login}, function(err, docs){

    // Log database error if it's there
    if(err){
      next("Database error fetching user"); // TODO: add error handler
    } else {
      // If the user exists, add it to the error vector BUT keep going
      if(docs){
        errors.push({ field:'login', message: 'Login name taken, sorry!' } );
      }
      Workspace.findOne({ name: req.body.workspace }, function(err, docs){
        if(err){
          next("Database error fetching workspace");
        } else {
          if(docs){
            errors.push( {field: "workspace", message: "Workspace taken, sorry!"} );
          } 

          if(errors.length != 0){
            serverResponse(res, errors);        
          } else {

            //
            // AT THIS POINT, UNLESS SOMETHING JUMPS ON US, both user and workspace are available
            //

            // User doesn't exist: create it
            var u = new User();
            u.login = req.body.login;
            u.password = req.body.password[0];
            u.workspaceIds = [];

            u.save( function(err) {
              if(err){
                next("Database error saving user");
              } else {
                var w = new Workspace();
                w.name = req.body.workspace;
                w.activeFlag = true;
                w.ownerUserId = u._id;
                w.countryId = null;
                w.save( function(err){
                  if(err){
                    next("Database error saving workspace. WATCH OUT: user " + u.login + " doesn't have a workspace!");
                  } else{
                    serverResponse(res, errors);
                  }
                })
              }
            })
          }
        }
      })    
    }
  });

  //
  } , 500); // Artificial timeout
  //
}

Answer 1

Here are some simple tips.

1)

You can combine the two if conditions for if(docs) and if(errors.length != 0), since if docs is true, then errors has positive length.

Old Code:

if (docs) {
    errors.push({
        'field' : field,
        'message' : errorMessage
    });
}
if (errors.length) {
    errors.push({
        field : "workspace",
        message : "Workspace taken, sorry!"
    });
    serverResponse(res, errors);
    return;
}

New Code:

if (docs) {
    errors.push({
        field : "workspace",
        message : "Workspace taken, sorry!"
    });
    serverResponse(res, errors);
    return;
}

2)

For this sitation, it's best not to make multiple function calls to the same function.

Old Code:

var serverResponse = function(res, errors){
  if(errors.length){
    res.json( { response: 'ERRORS', errors: errors } , 200)
    console.log("Sent ERROR");
  } else {
    res.json( { response: 'OK' } , 200)
    console.log("Sent OK");
  }
}

New Code:

var serverResponse = function (res, errors) {
    var status = {};
    if (errors.length) {
        status.response = 'ERRORS';
        status.errors  = errors;
    } else {
        status.response = 'OK';
    }
    res.json( status, 200 );
    console.log( "SENT " + status.response );
}

3)

Pass warnings but not errors. Deal with errors immediately.

4)

Avoid having too many else conditions. Use a if guard to avoid this.

Old Code:

function a( err ){
    if( err ){
        throw new Error( err );
    }else{
        //... code
    }
}   

New Code:

function a( err ){
    if( err ){
        throw new Error( err );
        return
    }
    //... code  
}   

*/

5)

Don't repeat yourself.

6)

Split up functions longer than 8 - 12 lines into smaller functions.

7)

if(errors.length != 0){ is the same as if(error.length){.

8)

Try to only have 1 nexted functions created in a single function. The setTimeout section has this problem.

9)

If it doesn't hurt the performance or security of the program then I would make User and Workspace global variables. Otherwise you could pass them where needed.

Final Code Revision

var User, Workspace;
var serverResponse = function (res, errors) {
    var status = {};
    if (errors.length) {
        status.response = 'ERRORS';
        status.errors  = errors;
    } else {
        status.response = 'OK';
    }
    res.json( status, 200 );
    console.log( "SENT " + status.response );
};
var getDatabaseRequestFunc = function (field, errorMessage, callback) {
    return function (err, docs) {
        if (err) {
            next("Database error fetching field: " + field);
            return;
        }
        if (docs) {
            var error = [{
                'field' : field,
                'message' : errorMessage
            }];
            serverResponse(res, error);
            return;
        }
        callback();
    };
};
var getUserSaveFunc = function (req, res, next, user) {
    return function (err) {
        if (err) {
            next("Database error saving user");
            return;
        }
        var w = new Workspace();
        w.name = req.body.workspace;
        w.activeFlag = true;
        w.ownerUserId = user._id;
        w.countryId = null;
        w.save(function (err) {
            if (err) {
                next("Database error saving workspace. WATCH OUT: user " + user.login + " doesn't have a workspace!");
                return;
            }
            serverResponse(res, []);
        });
    };
};
var registerWorkspace = function (req, res, next) {
    Workspace.findOne({
        name : req.body.workspace
    }, getDatabaseRequestFunc( "workspace", "Workspace taken, sorry!" , function(){
            var u = new User();
            u.login = req.body.login;
            u.password = req.body.password[0];
            u.workspaceIds = [];
            u.save(getUserSaveFunc(req, res, next, u));
        })
    );
};
var registerUserLogin = function (req, res, next) {
    User.findOne({
        login : req.body.login
    }, getDatabaseRequestFunc( "login", "Login name taken, sorry!" , function(){
            registerWorkspace(req, res, next);
        })
    );
};
exports.nonApiRegister = function (req, res, next) {
    setTimeout(function () {
        User = mongoose.model("User");
        Workspace = mongoose.model("Workspace"); 
        registerUserLogin(req, res, next);
    }, 500);
};

Answer 2

There is a lot of indentation here. I know that it's a known issue with node...

In JavaScript you can avoid deep nesting by defining the functions ahead of time. For example:

var userFindOneHandler = function(err, docs) {
   // code here 
}

User.findOne( { login: req.body.login}, findOneHandler );

and you can do this for all the nested calls (e.g. create a workspaceFindOneHandler.) I've got small example of something like this in here: http://hectorcorrea.com/Blog/JavaScript-Async-Programming-for-Sync-Heads