7
\$\begingroup\$

Sorry, if you were looking for fresh baked pies, I only have a web client to help you sell them.

I've recently picked up server-side JavaScript and have added it to my toolbox, and while I've had a great time learning so far, I've got some code which needs another pair of eyes to review it.

A quick objective summary: this code uses three big JS libraries: Node.js, Express, and Socket.io. For data storage, I've tied in MongoDB. What's going on is a simple back-and-forth communication between the server code (index.js) and the client code (not listed, I don't want that reviewed yet. If you really need it, I'll edit it in).

For some background, especially if you're not familiar with this technology: with socket.io, there are two main methods: emit and on. Emit sends a packet, either direction. On receives data, from either direction too. With my code, I first listen for a connection, and then emit some data to the client, and then listen for a response. When a response is heard, the server sends data back in return.

Index.js

/*
*
* Setup app details
*
*/

"use strict";

var express = require('express');
var app = express();
var http = require('http').Server(app);
var io = require('socket.io')(http);
var moment = require('moment');

/*
*
* Setup database details
*
*/

var mongodb = require('mongodb').MongoClient;
var assert = require('assert');
var objectID = require('mongodb').ObjectID;

var url = 'mongodb://localhost:27017/pps';

// Setup all public static files (Styles, scripts, images, etc.)
app.use(express.static('public'));

// Route the main page to index.html
app.get('/', function (req, res) {
    res.sendFile(__dirname + '/index.html');
});


var COLLECTIONS = {};
mongodb.connect(url, function (err, db) {
    assert.equal(null, err);
    COLLECTIONS = {
        FLAVORS: db.collection('Flavors'),
        UF_ORDERS: db.collection('UnfinishedOrders'),
        F_ORDERS: db.collection('FinishedOrders')
    };
});


/*
*
* Socket.io Connection with event handling
*
*/


io.on('connection', function (socket) {

    simpleEmit('flavors', COLLECTIONS.FLAVORS);
    simpleEmit('orders', COLLECTIONS.UF_ORDERS);
    simpleEmit('finished orders', COLLECTIONS.F_ORDERS);

    mongodb.connect(url, function (err, db) {
        assert.equal(null, err);
        socket.on('add flavor', function (msg) {
            insertFlavor(db, msg, function () {
                simpleEmit('flavors', COLLECTIONS.FLAVORS);
            });
        });
        socket.on('update flavor', function (msg) {
            updateFlavor(db, msg, function () {
                simpleEmit('flavors', COLLECTIONS.FLAVORS);
            });
        });
        socket.on('delete flavor', function (msg) {
            deleteFlavor(db, msg, function () {
                simpleEmit('flavors', COLLECTIONS.FLAVORS);
            });
        });
        socket.on('add order', function (msg) {
            insertUnfinishedOrder(db, msg, function () {
                simpleEmit('orders', COLLECTIONS.UF_ORDERS);
            });
        });
        socket.on('finish order', function (data) {
            getOrderDetails(db, 'UnfinishedOrders', data, function (data) {
                insertFinishedOrder(db, data, function () {
                    deleteUnfinishedOrder(db, data, function () {
                        simpleEmit('orders', COLLECTIONS.UF_ORDERS);
                        simpleEmit('finished orders', COLLECTIONS.F_ORDERS);
                    });
                });
            });
        });
        socket.on('delete order', function (msg) {
            deleteOrder(db, msg, function () {
                simpleEmit('orders', COLLECTIONS.UF_ORDERS);
            });
        });
    });
});

/*
*
* Emit Call
*
*/

function simpleEmit(command, collection) {
    mongodb.connect(url, function (err, db) {
        assert.equal(null, err);

        var data = [];
        var cursor = collection.find();
        cursor.each(function (err, doc) {
            assert.equal(err, null);
            if (doc !== null) {
                data.push(doc);
            } else {
                //console.log('SENDING... ' + command, data)
                io.emit(command, data);
                db.close();
            }
        });
    });
}

/*
*
* Database Calls
*
*/

var insertFlavor = function (db, data, callback) {
    db.collection('Flavors').insertOne({
        "flavor": data[0],
        "color": data[1]
    }, function (err, result) {
        assert.equal(err, null);
        callback(result);
    });
};

var getFlavors = function (db, callback) {
    var data = [];

    var cursor = db.collection('Flavors').find();
    cursor.each(function (err, doc) {
        assert.equal(err, null);
        if (doc !== null) {
            data.push(doc);
        } else {
            callback(data);
        }
    });
};

var updateFlavor = function (db, data, callback) {
    db.collection('Flavors').updateOne(
        { "flavor": data[0] },
        {
            $set: { "flavor": data[1] }
        }, function (err) {
            assert.equal(err, null);
            callback();
        });
};

var deleteFlavor = function (db, data, callback) {
    db.collection('Flavors').deleteOne(
        { "flavor": data },
        function (err) {
            assert.equal(err, null);
            callback();
        });
};

var insertUnfinishedOrder = function (db, data, callback) {
    db.collection('UnfinishedOrders').insertOne({
        "customer": data[0],
        "flavor": data[2],
        "date": data[1],
        "notes": data[3]
    }, function (err, result) {
        assert.equal(err, null);
        callback(result);
    });
};

var getUnfinishedOrders = function (db, callback) {
    var data = [];

    var cursor = db.collection('UnfinishedOrders').find();
    cursor.each(function (err, doc) {
        assert.equal(err, null);
        if (doc !== null) {
            data.push(doc);
        } else {
            callback(data);
        }
    });
};

var deleteOrder = function (db, data, callback) {
    db.collection('UnfinishedOrders').deleteOne(
        { "_id": objectID(data) },
        function (err) {
            assert.equal(err, null);
            callback();
        });
};

var getOrderDetails = function (db, collection, data, callback) {
    var cursor = db.collection(collection).find({_id: objectID(data[0])});
    cursor.each(function (err, doc) {
        assert.equal(err, null);
        if (doc !== null) {
            callback([doc, data[1]]);
        }
    });
};

var deleteUnfinishedOrder = function (db, data, callback) {
    db.collection('UnfinishedOrders').deleteOne(
        { "_id": objectID(data[0]._id) },
        function (err) {
            assert.equal(err, null);
            callback();
        });
};

var insertFinishedOrder = function (db, data, callback) {
    db.collection('FinishedOrders').insertOne({
        "customer": data[0].customer,
        "flavor": data[0].flavor,
        "date-finished": +moment(),
        "price": data[1]
    }, function (err, result) {
        assert.equal(err, null);
        callback(result);
    });
};

var getFinishedOrders = function (db, callback) {
    var data = [];

    var cursor = db.collection('FinishedOrders').find();
    cursor.each(function (err, doc) {
        assert.equal(err, null);
        if (doc !== null) {
            data.push(doc);
        } else {
            callback(data);
        }
    });
};

/*
*
* Listen for the initial connection
*
*/

http.listen(3000, function () {
    console.log('Node.js listening at http://localhost:3000/');
});

There are a few issues I have with this code...

  1. A lot is repeated. In an attempt to cut back on emit code, I made simpleEmit(). It works great. Any ideas on how I could do something similar to clean up the many on methods?
  2. Because almost every function requires the database, I had a lot of mongodb.connects at one point. I've cleaned them up mostly, but I'm wondering if I'm handling this connection correctly. I was forced to take out the close() method on mongodb. Will this hurt me? (This is what makes the title relevant!)
  3. Everything is in the global scope, should I change this? If so, in what way?

Anything else goes!

\$\endgroup\$
1
\$\begingroup\$

Not entirely sure why you'd keep calling connect and close. As far as my DB knowledge goes, you open a connection once and keep it open. Here's an answer from StackOverflow regarding the issue. So I suggest that once you get the db object, you simply just pass it on. In this case, simpleEmit should be handed over the db object.

I recommend that you use promises in your API. It appears that your mongo API already supports promises. You can simply reuse the db from the initial call. connect appears to return a promise.

// Your only connect call which resolves to a db object
var connectionPromise = mongodb.connect(url);

// Hook your collection shorthand
connectionPromise.then(db => {
  COLLECTIONS = {
    FLAVORS: db.collection('Flavors'),
    UF_ORDERS: db.collection('UnfinishedOrders'),
    F_ORDERS: db.collection('FinishedOrders')
  };
});

// Hook up your socket connections only when we connect to the db
connectionPromise.then(db => {
  io.on('connection', socket => {
    /* socket listeners */
  }
});

The db methods also appear to return promises. insertOne returns a Promise according to the docs.

// Here's a sample of how a db operation call would look like
function insertFinishedOrder(db, data, callback) {
  return db.collection('FinishedOrders').insertOne({
    "customer": data[0].customer,
    "flavor": data[0].flavor,
    "date-finished": +moment(),
    "price": data[1]
  });
};

// Here's an example of a caller with multiple calls one after the other
socket.on('finish order', function (data) {
  getOrderDetails(db, 'UnfinishedOrders', data)
    .then(data => insertFinishedOrder(db, data))
    .then(data => deleteUnfinishedOrder(db, data))
    .then(() => simpleEmit(db, 'finished orders', COLLECTIONS.F_ORDERS));
});

Now if you rewrote everything to take advantage of promises, notice how insertFinishedOrder and finish order is also simplified. insertFinishedOrder simply just returns a promise for finish order to consume. finish order takes advantage of promises to appear linear instead of nested.

Now that we've started to consider promises, I notice that there are several functions that are not async like getFlavors. So that we can be consistent with the promise-returning api we have, wrap the operation in a promise. Also, each is deprecated. Use forEach.

function getFlavors(db, callback) {
  return new Promise((resolve, reject) => {
    var data = [];
    var cursor = db.collection('Flavors').find();
    cursor.forEach(doc => (doc !== null) ? data.push(doc) : resolve(data););
  });
}

Also suggesting to use a function declaration instead of function expressions. With function declarations, you take advantage of what they call "hoisting", where in variable and function declarations are visible regardless of where they are placed. But it's also a best practice to put your functions together so you don't have to go out and hunt them.

\$\endgroup\$
  • \$\begingroup\$ This is some good info, I'll need to learn more about promises and the other few things you mentioned. Thanks for the good review \$\endgroup\$ – Alex L Jan 7 '16 at 3:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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