3
\$\begingroup\$

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?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Don't store plaintext passwords in the database. Use a proper password hash, like bcrypt, scrypt or PBKDF2. See How to securely hash passwords? for details. \$\endgroup\$ Mar 21, 2014 at 10:48

2 Answers 2

4
\$\begingroup\$

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] );
      });
    }
  });
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank you. Nice answer. How can I make the same for get by id for example ? Now I have something like this db.execute('SELECT device_id, platform FROM users WHERE id=$1', [id], function (err, rows) { ... \$\endgroup\$
    – Valery
    Mar 20, 2014 at 5:12
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ it is actually sharp decision to use ORM, there can be new question about use it or not. So I am not ready to make it. The another thing you said was right, especially about objects instead of ordinary arguments \$\endgroup\$
    – Valery
    Mar 20, 2014 at 3:27
  • \$\begingroup\$ I was going to write something about pooling, however pooling is inherent with pg, all that must be done is call done so that the connection is returned to the pool which the OP does. \$\endgroup\$
    – konijn
    Mar 20, 2014 at 12:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.