8
\$\begingroup\$

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);
                                                    }
                                                });
                                },
    };
}
\$\endgroup\$
  • \$\begingroup\$ would you please include a link to the other questions that you posted, it seems that they might be relevant when reviewing the code \$\endgroup\$ – Malachi Dec 9 '13 at 18:05
  • 1
    \$\begingroup\$ @Malachi: codereview.stackexchange.com/q/36938/507 \$\endgroup\$ – Martin York Dec 9 '13 at 19:41
  • \$\begingroup\$ @LokiAstari The code is highlighted as SQL, it needs to be highlighted as JavaScript. \$\endgroup\$ – aaaaaaaaaaaa Dec 13 '13 at 22:52
6
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Thanks. Working through your comments. I was told by another JS expert to avoid named functions (ie prefer anonymous) as the names can potentially clash with other code (which is a real problem as the code grows in size). In your second example are the names not exposed beyond the module? or is there a technique to make them not exposed further? \$\endgroup\$ – Martin York Jan 3 '14 at 22:31
  • 2
    \$\begingroup\$ @Loki: Read about scoping in JS. All variables declared with var (or function, for that matter) are local to the function they are declared inside of. So in your case named functions will be local to updateUser and getSavedUser, so there can be no clash with other parts of code. Moreover, what you declare inside NodeJS module is local to that module, unless you put it in exports. So no, it isn't a problem. \$\endgroup\$ – Dan Jan 3 '14 at 22:40
  • 1
    \$\begingroup\$ OK. Java Expert redeemed. My interpretation of his instructions was off. I have fixed my assumptions. \$\endgroup\$ – Martin York Jan 6 '14 at 21:00
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I agree with the indentation (totally bad (in my version)). But using named functions I know is a bad idea as it causes clashes. Best practice is to use unamed functions. \$\endgroup\$ – Martin York Dec 13 '13 at 21:28
  • \$\begingroup\$ @LokiAstari Clashes? You do know that you can make inner functions that only take up namespace in the local scope? \$\endgroup\$ – aaaaaaaaaaaa Dec 13 '13 at 22:46
  • \$\begingroup\$ No I was note. But that is the kind of detailed explanation that would be good inside a code review. \$\endgroup\$ – Martin York Dec 14 '13 at 18:17
  • \$\begingroup\$ @LokiAstari I can't know for sure where the holes in your knowledge is. But discussing my answer bore fruit, and you gained a new piece of knowledge. You can read more about scope and closures on a load of sites, you might as well Google it as me. The most relevant thing for me to add is probably that Node has an extra scope layer, a module's root scope is local to that module, but there is also a global scope that you can access through the global object, or by simply not declaring a variable with var. \$\endgroup\$ – aaaaaaaaaaaa Dec 14 '13 at 18:47

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.