Node exports, architecture

Question

My project is written in node JS + Express and has the following architecture:

• routes (e.g controllers)
• models (stateless functional modules)
• app

Models objects only doing SQL queries like this:

exports.setPassword = function (id, password, callback) {
  pg.connect(conString, function (err, client, done) {
    if (err) callback(err);
    else {
      client.query('UPDATE users SET password=$1 WHERE id=$2', [password, id], function (err) {
        done();
        if (err) callback(err);
        else callback(null);
      });
    }
  });
};

exports.setStatus = function (id, status, callback) {
  pg.connect(conString, function (err, client, done) {
    if (err) callback(err);
    else {
      client.query('UPDATE users SET status=$1 WHERE id=$2', [status, id], function (err) {
        done();
        if (err) callback(err);
        else callback(null);
      });
    }
  });
};


exports.getStayInTouch = function (userId, callback) {
  pg.connect(conString, function (err, client, done) {
    if (err) callback(err);
    else {
      client.query('SELECT * FROM stay_in_touch_contacts WHERE user_id=$1', [userId], function (err, results) {
        done();
        if (err) callback(err);
        else callback(null, results.rows[0]);
      });
    }
  });
};

and many many functions connected with the user in that user.js module. The only thing I can do with that architecture is to place functions to the different module.

I'd appreciate any critiques of that architecture. How can I do better?

Answer 1

That does not look very DRY ( Dont repeat yourself ), not to mention that it does not look good that every function must be aware of the connection string.

For updates, you could do something like this:

exports.setPassword = function (id, password, callback) {
  genericUpdate('UPDATE users SET password=$1 WHERE id=$2', [password, id], callback );
};

exports.setStatus = function (id, status, callback) {
  genericUpdate('UPDATE users SET status=$1 WHERE id=$2', [status, id], callback );
};

function genericUpdate( queryString, parameters, callback )
{
  callback = callback || function(){};
  pg.connect(conString, function (err, client, done) {
    if (err){ 
      callback(err);
    } else {
      client.query(queryString, parameters, function (err) {
        done();
        callback(err || null);
      });
    }
  });
}

I added curly braces and newlines to your if statements, other than that your code seems fine.

EDIT: some thoughts on selects, you probably want a few generic selection functions, if you create one that is meant for selecting on an id, then you can get away with always returning the first data record if any.

exports.getStayInTouch = function (userId, callback) {
  selectById( 'SELECT * FROM stay_in_touch_contacts WHERE user_id=$1' , userId , callback );
};

function selectById( queryString, id, callback )
{
  callback = callback || function(){};
  pg.connect(conString, function (err, client, done) {
    if (err){ 
      callback(err);
    } else {
      client.query(queryString, id, function (err,results) {
        done();
        if(err)
          callback(err)
        else
          callback(null, results.rows[0] );
      });
    }
  });
}

Answer 2

I think the most important thing to improve on would be what konjin's answer already alluded to which is to keep DRY. The pooling interface of your driver seems a bit lacking to me and might be making it difficult to write DRYer code. I prefer the pooling interface that the MYSQL driver has.

var pool  = mysql.createPool({
  host     : 'example.org',
  user     : 'bob',
  password : 'secret'
});

pool.query('SELECT 1 + 1 AS solution', function(err, rows, fields) {
  if (err) throw err;

  console.log('The solution is: ', rows[0].solution);
});

---

One improvement I would make would be to have the same signature for your exports. Currently they follow arg1 arg2 callback, arg1 arg2 callback, arg1 callback. Having the callback as the first argument can avoid confusion. I would consider moving the arguments just an object and have all of the signatures match (callback, args). Having params on an object would make it easier for consumers of the exports not have to be concerned about the order of the arguments.

For example:

updateUser(5, 'Patrick', '123456', 1)

vs

updateUser({id: 5, name: 'Patrick', password: '123456', status: 1})

The second example is much easier to understand without looking at the implementation for updateUser.

---

One other thing I would consider is looking to using an ORM. It could help with the tedious writing DBMS specific queries and help add functionality. For example if I was using an ORM I could probably could update attributes on a user with only a little bit of code.

For example:

new User(id).update({status: 1, password: '123456'}, callback)

vs

setStatus(id, status, function() { setPassword(id, password, callback })

Besides being less code and less complex which is the most important the first chunk of code will have twice as many DB hits.