NodeJS broker between MongoDB and RabbitMQ

Question

I wrote a small program that aims to take some data from the MongoDB database and put them in the RabbitMQ queue.

I tried to use only promise style but I am a beginner in JavaScript. Could you please help me to improve my first draft?

var mongo = require('mongod');
var amqp = require('amqplib');
var _ = require('underscore');
var TaskBroker = function () {
 this.queueName = 'task_queue';
 this.rabbit = {};
 this.mongo = {};
};
TaskBroker.prototype.connectRabbit = function() {
 return amqp.connect('amqp://localhost')
 .then(function (connection) {
 this.rabbit.connection = connection;
 return connection.createChannel()
 }.bind(this))
 .then(function(channel) {
 this.rabbit.channel = channel;
 return channel.assertQueue(this.queueName, {durable: true});
 }.bind(this))
};
TaskBroker.prototype.connectMongo = function(){
 return function() {
 this.mongo.db = mongo('mongodb://127.0.0.1:27017/test', ['test']);
 return this.mongo.db;
 }.bind(this);
};
TaskBroker.prototype.connect = function () {
 return this.connectRabbit()
 .then(this.connectMongo());
};
TaskBroker.prototype.disconnect = function() {
 this.mongo.db.close();
 this.rabbit.channel.close();
 this.rabbit.connection.close();
};
TaskBroker.prototype.get_url_array = function(_data) {
 return _.chain(_data).pluck('probes').flatten().pluck('url').uniq().value();
};
TaskBroker.prototype.getTask = function() {
 return function () {
 return this.mongo.db.test.find({ 'status': 'ONGOING' }, { 'probes.url':1, '_id':0})
 .then(function(results) {
 var url_array = [];
 if (results != null && results.length > 0) {
 url_array = this.get_url_array(results);
 }
 return this.mongo.db.test.find({ 'probes.url' : { $nin: url_array } });
 }.bind(this))
 .then(function(results) {
 if (results.length > 0) return results[0];
 return null;
 });
 }.bind(this);
};
TaskBroker.prototype.produceTask = function() {
 return function(_message) {
 if(_message != null) {
 _message.status = 'ONGOING';
 this.rabbit.channel.sendToQueue(this.queueName, new Buffer(JSON.stringify(_message)), { deliveryMode: true });
 return this.mongo.db.test.update({_id: _message._id}, { $set: { 'status': _message.status }}).then(function() {
 return _message;
 });
 }
 return null;
 }.bind(this);
};
var taskBroker = new TaskBroker();
taskBroker.connect()
 .then(function() {
 setInterval(
 function () {
 taskBroker.getTask()()
 .then(taskBroker.produceTask())
 .then(function(result) {
 if(result == null) {
 console.log('No job to produce');
 } else {
 console.log('Produce', result);
 }
 //console.log('Disconnected');
 //taskBroker.disconnect();
 }
 , function(error) {
 console.log('ERROR', error.stack);
 }
 );
 }
 , 10000
 );
 });

Answer 1

Interesting question, it was hard to find things I would do different.

• To Malachi's point, there are a few strings you could put in a config object, I would definitely add also 10000 to that config object as config.delay and your statuses like 'ONGOING'.

• Also, I would write this:

  .then(function(results) {
   if (results.length > 0) return results[0];
   return null;
  });
  

  As

  .then(function(results) {
   return results[0] || null;
  });
  

• In the bottom part, you have very stylized code, very functional except for the 2 blocks. I would create 2 named functions called logResults and logError and use those.

• In produceTask, you are stretching quite a bit your horizontally, part of that is your atypical use of then on the same line ( different from the rest of your code ). I would counter-propose cutting the code a bit up:

  TaskBroker.prototype.produceTask = function() {
   var sendOptions = { deliveryMode: true };
   var statusUpdate = { $set: { 'status': 'ONGOING' }};
   return function(message) {
   if(!message) {
   return;
   }
   message.status = 'ONGOING';
   var content = new Buffer(JSON.stringify(message));
   this.rabbit.channel.sendToQueue(this.queueName, content , sendOptions);
   return this.mongo.db.test.update({_id: message._id}, statusUpdate)
   .then(function() {
   return message;
   });
   }.bind(this);
  };
  

  • I also declared sendOptions and statusUpdate outside of the returned function, there is no sense in re-creating these all the time.
  • I also return early if there is no message to reduce arrow pattern code
  • I renamed _message to message, more on that later

• Anonymous functions; you are using a number of anonymous functions. This becomes a problem if you have to analyze stack traces, simply add well named function names, and your code becomes that much easier to support:

  TaskBroker.prototype.connectRabbit = function() {
   return amqp.connect('amqp://localhost')
   .then(function onConnect(connection) {
   this.rabbit.connection = connection;
   return connection.createChannel()
   }.bind(this))
   .then(function onChannelCreated(channel) {
   this.rabbit.channel = channel;
   return channel.assertQueue(this.queueName, {durable: true});
   }.bind(this))
  };
  

• JsHint.com -> Use it, you have missing semi colons and bad line breaks

• Commented out code -> Dont do that in production code

• Console.log -> I would wrap this in a function where I can control whether I really want to log and where I can optionally also log into a file for later analysis

• I am not sure why you prefix some variables and parameters with underscore, I would advise against it. I know that mongo does this, to distinguish between mongo properties and data properties on an object, which is okay. But I see no good reason in your code for you to prefix variables/parameters with _

• Connection to Mongo db; your approach is okay for development, for quality and production I would get the details from the environment:

  this.mongo.db = mongo('mongodb://'+ process.env.MY_APP_MONGODB_HOST + '/myapp' , ['myapp']);
  

• You can write

  if (results != null && results.length > 0) {
   url_array = this.get_url_array(results);
  }
  

  as

  if (results instanceof Array) {
   url_array = this.get_url_array(results);
  }
  

• I dont like url_array, I tend to take whatever is in the array and an 's' in JavaScript, but urls does not read much better in my mind. All I can say is that you should have some deep thoughts about that variable name.

• The usage of null; the usage of null is not idiomatic JavaScript. Consider converting your code to check for undefined instead. Since functions return undefined if you dont return something yourself, you can then avoid statements like return null

All in all, I wish could write code like that when I am a beginner in a new language ;) Sometimes the code feels a bit forced into the promise style, but all in all this is pretty good.

• Julio: Sorry the answer is more complete ;). Btw, I think this part is really not pretty: taskBroker.getTask()()
• konijn: @Julio I stared for the longest time at your code, and still didn't pick that up..

Answer 2

You have some magic strings:

return amqp.connect('amqp://localhost')

this.mongo.db = mongo('mongodb://127.0.0.1:27017/test', ['test']);

These strings should be kept in a high scope variable, so that you can easily maintain them by changing out the strings in one place. Eventually you will have bigger applications where you use the connection in several places or just in one place, but you don't want to be looking all over for a missing connection string or pray that you didn't miss one.

---

You need to watch your formatting very closely in code that is nested like this:

 .then(function(results) {
 if (results.length > 0) return results[0];
 return null;
 });

You didn't indent this code and you didn't use braces. I am sure that this is syntactically correct, but very confusing. The first time I read your code I thought that if (results.length > 0) would return null;. I had to read it a second time to catch that this is an inline if statement. Nowhere else in your code do you write a statement like this.

Use braces please, and indentation, and new lines; that is what minifying is for. Make it readable when you are coding and shrink it for deployment. Computers and the Internet are getting faster and able to handle larger files every day; don't be afraid of a brace or a newline.