Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have moved all my web-server code dealing with user management into a single Node.js module that I pasted below. I have spent time polishing it, and churned it through JSHint. I am generally happy with the code, except for the error handling. I find it clunky at best.

How can I improve error handling in the following code? Are there design or API guidelines I have not followed?

// ???
// Deal with the case where an admin no longer has admin rights, but still has a session open

var server = require('./server.js').server;
var User = require('./database.js').User;

// Define the user API
var API = {
    list: [list, 'private'],
    login: [login, 'public'],
    logout: [logout, 'private'],
    add: [add, 'admin'],
    remove: [remove, 'admin'],
    edit: [edit, 'admin']
};

// Attach path handlers
for(var label in API) {
    var denied = 'Permission denied';

    var wrapper = (function (label) {
        return function (req, res) {
            var callback = API[label][0];
            var permission = API[label][1];

            if(!req.session) {
                if(permission !== 'public') {
                    res.send(denied);
                    return;
                }
            } else if((permission === 'admin') && (req.session.rights !== 'Administrator')) {
                res.send(denied);
                return;
            }

            callback(req, res);
        };
    }(label));

    server.post('/user/' + label, wrapper);
}

function list(req, res) {
    User.find({}, null, {sort: {username: 1}}, function (err, users) {
        res.send(users);
    });
}

function login(req, res) {
    var errorMessage = 'Invalid user/password combination';

    find(req.body.username, function(err, user) {
        if(printError(err)) {
            return;
        }

        // Check we have a user
        if(!user) {
            res.send(errorMessage);
            return;
        }

        // Check for a username/password match
        user.comparePassword(req.body.password, function(err, isMatch) {
            if(printError(err)) {
                return;
            }

            if(isMatch) {
                // Populate the session cookie
                req.session.username = user.username;
                req.session.rights = user.rights;

                res.send('OK');
            } else {
                res.send(errorMessage);
            }
        });
    });
}

function logout(req, res) {
    // Clear session
    req.session.username = undefined;
    req.session.rights = undefined;
    res.send('OK');
}

function add(req, res) {
    find(req.body.username, function (err, user) {
        if(user) {
            res.send('The user already exists');
        } else {
            new User(req.body).save(function (err) {
                if(printError(err)) {
                    return;
                }

                res.send('OK');
            });
        }
    });
}

function remove(req, res) {
    find(req.body.username, function (err, user) {
        user.remove(function (err) {
            if(printError(err)) {
                return;
            }

            res.send('OK');
        });
    });
}

function edit(req, res) {
    find(req.body.username, function (err, user) {
        if(printError(err)) {
            return;
        }

        // Populate username and rights
        user.username = req.body.user.username;
        user.rights = req.body.user.rights;

        // Only update nonempty passwords
        if(req.body.user.password !== '') {
            user.password = req.body.user.password;
        }

        user.save(function(err) {
            if(printError(err)) {
                return;
            }

            res.send('OK');
        });
    });
}

function find(username, callback) {
    User.findOne({
        username: username
    }, callback);
}

function printError(err) {
    if(err) {
        console.error('ERROR: ', err.message);
        // console.trace();
    }

    return err;
}
share|improve this question

1 Answer 1

From a once over

  • Not everybody in CR agrees, but I find tabular data best presented in an aligned format:

    var API = {                            
      list:   [list  , 'private'],     
      login:  [login , 'public' ],    
      logout: [logout, 'private'],     
      add:    [add   , 'admin'  ],   
      remove: [remove, 'admin'  ],   
      edit:   [edit  , 'admin'  ]      
    };                           
    
  • For your error handing, I would mention that using express is very nice for error handling. If you insist on doing it yourself you could do something like

    function handle( f ){
      return function( err ){
        if(err){
          console.error('ERROR: ', err.message);
          return;
        }
        f.apply( this , arguments );
      }
    }
    

    Then you can

    function remove(req, res) {
      find(req.body.username, handle( function (err, user) {
        user.remove( handle( function (err) {
          res.send('OK');
        }));
      }));
    }
    
  • For your error message constants, I would declare them all together on top
  • I am ambivalent about using an array for your API config instead of an object, if you keep using that, I would suggest you use named constants declared on top:

        var callback   = API[label][CALLBACK];   //Where CALLBACK is earlier declared as 0
        var permission = API[label][PERMISSION]; //Where PERMISSION is earlier declared as 1
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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