Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've got server running to make serial data available on a TCP/IP port. My goal is to collect data coming from a realtime device connected to the serial port and deliver it to web clients connected to an express server using socket.io as the data is collected from a backend socket. I want the backend socket to reconnect if for some reason data stops or the backend socket disconnects. I also want to be able to expose the ability to change the back end data host and port. I've managed to put something together that does all of this, and it works reasonably well.

I am not really sure how this will hold up to a production environment. There is a problem already with any client being about to change the backend data source, which is behavior that I will need to avoid.

Short of that, would anyone care to comment on my code with regards to making this more bullet-proof or maybe point out some shortcomings? This is my first time using socket.io in a production environment.

var express = require('express'),
    app = module.exports = express.createServer(),
    io = require('socket.io').listen(app, { log: true }),
    routes = require('./routes'),
    delimiter ="\n", 
    bufferSendCommand = "$X",
    net = require('net'),
    serverPort = 3000,
    dataServerTimeOut = 2000,
    host = "somehost.com",
    dataSourcePort = 5000,
    buffer = [],
    events = require('events'),
    line = "",
    gotAChunck = new events.EventEmitter(),
    dataConnection = new events.EventEmitter(),
    connectionMonitor,
    reconnect = 0,
    reconnectAttemptTime = 10000,
    dataMonitorThresholdTime =  5000,
    dataStream = net.createConnection(dataSourcePort, host);

function startReconnect(doReconnect){
    dataConnection.emit('status',false);
    io.sockets.emit('error',{message:"Lost Data Source - Attempting to Reconnect"});
}

dataStream.on('error', function(error){
    io.sockets.emit('error',{message:"Source error on host:"+ host + " port:"+dataSourcePort});
});

dataStream.on('connect', function(){
    dataConnection.emit('status',true);
    io.sockets.emit('connected',{message:"Data Source Found"});
});

dataStream.on('close', function(){
    io.sockets.emit('error',{message:"Data Source Closed"});
});

dataStream.on('end',function(){
    io.sockets.emit('error',{message:"Source ended on host:"+ host + " port:"+dataSourcePort});
});

dataStream.on('data', function(data) {

    dataConnection.emit('status',true);

    clearTimeout(connectionMonitor);
    connectionMonitor = setTimeout(function(){startReconnect(true);}, dataMonitorThresholdTime);

    // Collect a line from the host
    line += data.toString();
    // Split collected data by delimiter
    line.split(delimiter).forEach(function (part, i, array) {
        if (i !== array.length-1) { // Fully delimited line.
            //push on to buffer and emit when bufferSendCommand is present
            buffer.push(part.trim());
            if (part.substring(0, bufferSendCommand.length) == bufferSendCommand){
                gotAChunck.emit('new', buffer);
                buffer=[];
            }
        }
        else {
            // Last split part might be partial. We can't announce it just yet.
            line = part;
        }
    });
});

dataConnection.on('status', function(connected){

    if(!connected){
        reconnect = setInterval(function(){dataStream.connect(dataSourcePort, host);}, reconnectAttemptTime);
    }
    else{
        clearInterval(reconnect);
    }
});

gotAChunck.on('new', function(buffer){
    io.sockets.emit('feed', {feedLines: buffer});
});

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

    dataConnection.emit('status',false);

    // Handle Client request to change data source
    socket.on('message',function(data) {
        var clientMessage = JSON.parse(data);
            if('connectString' in clientMessage
                && clientMessage.connectString.dataHost !== ''
                && clientMessage.connectString.dataPort !== '') {
                dataStream.destroy();
                dataSourcePort = clientMessage.connectString.dataPort;
                host = clientMessage.connectString.dataHost;
                dataStream = net.createConnection(dataSourcePort, host);
            }
    });
});

app.configure(function(){
    app.set('views', __dirname + '/views');
    app.use(express.bodyParser());
    app.use(express.methodOverride());
    app.use(app.router);
    app.use(express.static(__dirname + '/public'));
});

app.configure('development', function(){
    app.use(express.errorHandler({ dumpExceptions: true, showStack: true })); 
});

app.configure('production', function(){
    app.use(express.errorHandler()); 
});

app.get('/', routes.index.html);

app.listen(serverPort);
share|improve this question
    
You are defining a lot of constants at the beginning of the file. I would suggest that you use ONLY_CAPS_FOR_CONSTANTS like proposed here google-styleguide.googlecode.com/svn/trunk/…. This way it's clear that's not meant to change later in the application. Ex: DATA_SERVER_TIMEOUT –  jackdbernier Mar 17 at 14:21

1 Answer 1

A fun piece of code,

  • You clearly took the single comma separated var statement all the way, I would still group the related variables ( requires, module, server info, timeouts etc.):

    var express = require('express'),
        routes  = require('./routes'),
        net     = require('net'),
        events  = require('events'),    
        app     = module.exports = express.createServer(),
        io = require('socket.io').listen(app, { log: true }),
        serverPort = 3000,    
        host  = "somehost.com", 
        delimiter ="\n", 
        bufferSendCommand = "$X",
        gotAChunck = new events.EventEmitter(),
        dataConnection = new events.EventEmitter(),
        dataServerTimeOut = 2000,
        dataSourcePort = 5000,
        reconnectAttemptTime = 10000,
        dataMonitorThresholdTime =  5000,
        dataStream = net.createConnection(dataSourcePort, host),
        buffer = [],
        line = "",
        reconnect = 0,
        connectionMonitor;
    
  • I am not a big fan of your handling the last bit of info, I would counter-propose this:

    // Collect data from the host
    var lines ( line += data.toString() ).split(delimiter);
    // Last split part might be partial. We can't announce it just yet.
    line = lines.pop();
    // Split collected data by delimiter
    lines.forEach(function (part) {
      //push on to buffer and emit when bufferSendCommand is present
      buffer.push(part.trim());
      if (part.substring(0, bufferSendCommand.length) == bufferSendCommand){
        gotAChunck.emit('new', buffer);
        buffer=[];
      }
    });
    

    The first line might be too Golfic for you, feel free to split it out.

All in all, I think the code is well written and should be easy to understand/maintain.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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