1
\$\begingroup\$

This function updates the user last login date on the user collection. I guess there are too many brackets and much spaghetti. How can I shorten this code?

(function () {
    'use strict';

    var database = require('mongodb');
    var util = require('util');

    var constants = require('../constants');

    var dbhelper = {
        updateLastSeenDate: function (me) {
            database.connect(constants.variables.connection, function (err, db) {
                if (err) {
                    db.close();
                    throw err;
                }
                var bsonID = database.BSONPure.ObjectID(me);
                var collection = db.collection('users');
                collection.findOne({
                    _id: bsonID
                }, function (err, result) {
                    if (result) {
                        result.lastLoginDate = Date.now();
                        collection.update({
                            _id: bsonID
                        }, result, function (err, u) {
                            if (err) {
                                db.close();
                                throw err;
                            }
                            util.log('Update complete: updateLastSeenDate');
                            db.close();
                        }); //end of update
                    }
                });
            });
        }
    };

    module.exports = dbhelper;
}());
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! Your question only contains code. Could you edit it with a proper context ? You could specify what your code is doing, what you think is the major point that need refactoring (note that we could do general review but if you a special point you can mention it). \$\endgroup\$ – Marc-Andre Mar 3 '14 at 16:07
1
\$\begingroup\$

Here's some points. I commented directly in the code.

(function () {
    'use strict';

    //Why do you use `database` instead of `mongo` or `mongodb`.
    //It can become confusing later in the code when you are using `db`.
    //It will be hard to tell the diffence between database and db.
    var database = require('mongodb');
    var util = require('util');

    var constants = require('../constants');

    var dbhelper = {
        //`me` Will it be you all the time ?
        //You should think a more meaningful name such as `logedInUserId`
        updateLastSeenDate: function (me) {
            //Do you really want to connect and close the database everything you want to update a field ?
            //You should consider keeping the connection alive and share it across the whole process.
            database.connect(constants.variables.connection, function (err, db) {
                if (err) {
                    db.close();
                    throw err;
                }
                var bsonID = database.BSONPure.ObjectID(me);
                var collection = db.collection('users');

                //nesting calls to async function is affecting readability, testability and maintability.
                //You should consider using workflow library such as async
                collection.findOne({
                    _id: bsonID
                }, function (err, result) {
                    if (result) {
                        result.lastLoginDate = Date.now();

                        //Here's another nested call
                        collection.update({
                            _id: bsonID
                        }, result, function (err, u) {
                            //Using async you could handle errors in only one place.
                            if (err) {
                                db.close();
                                throw err;
                            }
                            util.log('Update complete: updateLastSeenDate');
                            db.close();
                        });
                    }
                });
            });
        }
    };

    module.exports = dbhelper;
}());

I would use async https://github.com/caolan/async#waterfall, change a few variable names and share the connection. Since node.js in single threaded you can open only one connection and share it to all your modules.

\$\endgroup\$
2

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.