patternjavascriptMinor

Local user registration

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

localregistrationuser

Problem

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.p

Solution

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 (Provid

Code Snippets

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);
        }
      });
    }
  };
};

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
  };
};

var db = this.db;

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

    // Create the UserBook object
    return {

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

Context

StackExchange Code Review Q#36940, answer score: 6

Revisions (0)

No revisions yet.