HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Node exports, architecture

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
exportsnodearchitecture

Problem

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?

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#44744, answer score: 4

Revisions (0)

No revisions yet.