NodeJS broker between MongoDB and RabbitMQ

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

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.

StackExchange Code Review Q#49892, answer score: 7