Local user registration

Question

More javascript (nodejs) to go with the passport wrapper I just posted:

UserBook.js

/*
 *  A wrapper for user Registration for a web site.
 *  This is an MYSQL version of the client to provide persistent state.
 *
 * Exports a single function that returns the object (UserBook) that does the work.
 * UserBook exposes two public methods:
 *
 *      updateUser(profile, userRegister(profile))
 *          profile:        An object with information about the user.
 *                          From this we will try and retrieve the local-id of the user.
 *                          If there is no local-user we create one.
 *          userRegister:   A callback used when the user-id is found.
 *                          The id is added to the profile and this is passed as the
 *                          parameter to the callback.
 *      getSavedUser(localId, userRegister(profile))
 *          localId:        The local-id of a user retrieved from the session information.
 *          userRegister:   A callback used when the user information is found.
 *
 */

module.exports = function() {
    // Local DB connection
    var mysql   = require('mysql');
    var theDB   = mysql.createConnection({
                                            host:       '127.0.0.1',
                                            user:       'user',
                                            password:   'password',
                                            database:   'db'
                                            });

    // Create the UserBook object
    return {
                // The db object kept locally
                db:             theDB,
                updateUser:     function(profile, userRegistered) {
                                    var db = this.db;
                                    // See if the user exists.
                                    db.query(   'SELECT * FROM Users WHERE Provider=? AND ProviderId=? LIMIT 1',
                                                [profile.provider, profile.providerId],
                                                function(err, rows) {
                                                    if (err) throw err;
                                                    if (rows.length == 1) {
                                                        // If we have found a local user the set the ID
                                                        profile.id = rows[0].ID;
                                                        if (profile.displayName != rows[0].DisplayName) {
                                                            // See if we need to update the display name.
                                                            db.query(   'UPDATE Users SET DisplayName=? WHERE Provider=? AND ProviderId=?',
                                                                        [profile.displayName, profile.provider, profile.providerId],
                                                                        function(err) {
                                                                            if (err) throw err;
                                                                        });
                                                        }
                                                        // Callback when user is identified.
                                                        userRegistered(profile);
                                                    } else {
                                                        // No local user found.
                                                        // So lets create a new one.
                                                        db.query(   'INSERT INTO Users (Provider, ProviderId, DisplayName) VALUES(?,?,?)',
                                                                    [profile.provider, profile.providerId, profile.displayName],
                                                                    function(err, result) {
                                                                        if (err) throw err;
                                                                        profile.id=result.insertId;
                                                                        // Callback when new user is created
                                                                        userRegistered(profile);
                                                                    });
                                                    }
                                                });
                                },
                getSavedUser:   function(localId, userRegistered) {
                                    var db = this.db;
                                    // get the local user from the DB
                                    db.query(   'SELECT * FROM Users WHERE ID=? LIMIT 1',
                                                [localId],
                                                function(err, rows) {
                                                    if (err) throw err;
                                                    if (rows.length == 1) {
                                                        profile = { id:         rows[0].ID,
                                                                    provider:   rows[0].Provider,
                                                                    providerId: rows[0].ProviderId,
                                                                    displayName:rows[0].DisplayName};
                                                        // Callback when information is retrieved.
                                                        userRegistered(profile);
                                                    }
                                                });
                                },
    };
}

Answer 1

Indentation

The rightward drift in callback code makes code very hard to read—you're making the problem worse by choosing such large indentation. Most JS projects I've seen use 2 spaces.

Personally, I find this much easier to read:

module.exports = function() {
  // Local DB connection
  var mysql = require('mysql');
  var theDB = mysql.createConnection({
    host: '127.0.0.1',
    user: 'user',
    password: 'password',
    database: 'db'
  });

  // Create the UserBook object
  return {
    // The db object kept locally
    db: theDB,

    updateUser: function(profile, userRegistered) {
      var db = this.db;

      // See if the user exists.
      var selectArgs = [profile.provider, profile.providerId];
      db.query('SELECT * FROM Users WHERE Provider=? AND ProviderId=? LIMIT 1', selectArgs, function(err, rows) {
        if (err) throw err;

        if (rows.length == 1) {
          // If we have found a local user the set the ID
          profile.id = rows[0].ID;
          if (profile.displayName != rows[0].DisplayName) {
            // See if we need to update the display name.
            var updateArgs = [profile.displayName, profile.provider, profile.providerId];
            db.query('UPDATE Users SET DisplayName=? WHERE Provider=? AND ProviderId=?', updateArgs, function(err) {
              if (err) throw err;
            });
          }

          // Callback when user is identified.
          userRegistered(profile);
        } else {
          // No local user found.
          // So lets create a new one.
          var insertArgs = [profile.provider, profile.providerId, profile.displayName];
          db.query('INSERT INTO Users (Provider, ProviderId, DisplayName) VALUES(?,?,?)', insertArgs, function(err, result) {
            if (err) throw err;

            profile.id=result.insertId;
            // Callback when new user is created
            userRegistered(profile);
          });
        }
      });
    },

    getSavedUser: function (localId, userRegistered) {
      var db = this.db;
      // get the local user from the DB
      db.query('SELECT * FROM Users WHERE ID=? LIMIT 1', [localId], function (err, rows) {
        if (err) throw err;

        if (rows.length == 1) {
          profile = {
            id: rows[0].ID,
            provider: rows[0].Provider,
            providerId: rows[0].ProviderId,
            displayName:rows[0].DisplayName
          };

          // Callback when information is retrieved.
          userRegistered(profile);
        }
      });
    }
  };
};

Instead of wrapping query params each on a line I extracted SQL arguments into variables. I think it's reasonable given that it also allowed me to fit query call on one line, and thus avoid additional indentation inside a function.

Still, I don't think this is good either—we need to go deeper.

Callbacks

Why are you throwing errors instead of returning them to the caller in callback as first argument, like query itself does? NodeJS's convention is function(err, result), you should follow it. Otherwise, the client code won't be able to catch them.

Also, in your code, getSavedUser never calls the callback if there is no such user. Which means the client will just hang forever waiting for result.

Personally, I try to avoid callbacks when they represent an asynchronous operation that can fail or succeed, and use promises instead. They reduce nesting and allow much easier control flow, especially with regards to error handling.

AFAIK Promises are not yet supported out of the box in Node.js, but there are plenty libraries like Q. It's easy to “translate” NodeJS-style functions to promise-returning functions, for example Q has a method called Q.denodeify that does just that.

This is your code rewritten to use promises. The problems with non-bubbling errors and hanging getSavedUser are solved here:

var mysql = require('mysql'),
    Q = require('q'),

var db = mysql.createConnection({
  host: '127.0.0.1',
  user: 'user',
  password: 'password',
  database: 'db'
});

var promiseQuery = Q.denodeify(db.query.bind(db));

function updateUser(profile) {
  var selectArgs = [profile.provider, profile.providerId];

  return promiseQuery('SELECT * FROM Users WHERE Provider=? AND ProviderId=? LIMIT 1', selectArgs)
    .then(function (rows) {
      if (rows.length == 1) {
        // If we have found a local user the set the ID
        profile.id = rows[0].ID;

        if (profile.displayName == rows[0].DisplayName) {
          return;
        }

        // See if we need to update the display name.
        var updateArgs = [profile.displayName, profile.provider, profile.providerId];
        return promiseQuery('UPDATE Users SET DisplayName=? WHERE Provider=? AND ProviderId=?', updateArgs);
      }

      // No local user found.
      // So lets create a new one.

      var insertArgs = [profile.provider, profile.providerId, profile.displayName];
      return promiseQuery('INSERT INTO Users (Provider, ProviderId, DisplayName) VALUES(?,?,?)', insertArgs)
        .then(function (result) {
          profile.id = result.insertId;
        });
    })
    .thenResolve(profile);
}

function getSavedUser(localId) {
  return promiseQuery('SELECT * FROM Users WHERE ID=? LIMIT 1', [localId])
    .then(function (rows) {
      if (rows.length !== 1) {
        throw new Error('Could not find user');
      }

      return {
        id: rows[0].ID,
        provider: rows[0].Provider,
        providerId: rows[0].ProviderId,
        displayName:rows[0].DisplayName
      };
    });
}

module.exports = function() {
  return {
    updateUser: updateUser,
    getSavedUser: getSavedUser
  };
};

To use this code, instead of supplying a callback in the client code, you'll need to consume a promise by calling then to handle result, optionally catch to catch an error, and done to prevent errors from being unobserved.

Read more about promises. (This article is focused on native promises that just arrived to V8, but Q promises act the same.)

Minor considerations

• theDB is a bad name, why not just db? Also,

    var db = this.db;
  

  is useless inside the methods since you can just use db variable from outer scope. (See my second example.)

• Why do you expose the DB to client at all? This breaks encapsulation and doesn't seem safe.

• I removed useless comments from the second code snippet: some of them just repeat the code. Comments must specify why the code does what it does, not just rephrase the code in human language.

  Comments like

    // Local DB connection
    var mysql = require('mysql');

    // Create the UserBook object
    return {

    // Callback when user is identified.
    userRegistered(profile);

serve no valuable purpose, they only litter the code.

(And when someone changes the code, they often forget to change the comments, resulting in inconsistency.)

• In the second example, I moved functions out of the module.exports to reduce nesting even further. This will work just as fine.

Answer 2

I have never seen so relatively little code with so much indentation before. There are many acceptable style choices, but I think you'll regret this one.

Personally I usually avoid inlining functions, I know it is what all the cool kids are doing, but I find that it makes it harder to spot function boundaries.

So instead of:

foo(bar,function(){
    //code
})

I would do:

foo(bar,name)
function name(){
    //code
}

Alternately, if you do want functions inline take a lineshift to get the function beginning back to a reasonable spot.