2
\$\begingroup\$

I've built a simple chat server/client on Node.js and socket.io that I would like reviewed. My main concern is making the chat.js (client) running as cleanly as possible (OO) and streamlining data back and forth so I can add client monitoring (updated list of connected users) features down the line without issue.

Aside from that, I'm open to any critique/feedback about these sections of code on best practices or design patterns. This project is mostly for learning an unfamiliar technology (socket.io and, somewhat, node.js).

Here is the server code:

server.js:

var express = require('express');
var app = express();
var PORT = 7029;
var APPNAME = "Hermes";

//Set Views
app.set('views', __dirname + '/templates');
app.set('view engine', 'jade');
app.engine('jade', require('jade').__express);
app.get('/', function(req,res) {
    res.render('page');
});

//Setup server
app.use(express.static(__dirname + '/public'));
var io = require('socket.io').listen(app.listen(PORT));
io.set('transports', ['xhr-polling']);
console.log('Listening on port ' + PORT);

//Event Handlers
io.sockets.on('connection', function(socket) {
    welcome = new Message("Welcome to the ZeroDae chat service.", APPNAME);
    socket.emit('message', welcome );
    socket.on('send', function(data) {
        message = new Message(data.content, data.user);
        io.sockets.emit('message', message);
    });
    socket.on('join', function(data) {
        join = new Message(data + " has joined the chat.", APPNAME);
        io.sockets.emit('message', join);
    });
    socket.on('disconnect', function() {
        leaving = new Message("Someone has left.", APPNAME);
        io.sockets.emit('message', leaving);
    });
});



function Message(content, user) {
    this.content = content;
    this.user = user;
}

Here is the chat client:

chat.js:

$(document).ready(function () {
    var messages = [];
    var socket = io.connect('http://chat.zerodaedalus.com');
    var field = $('#field');
    var send = $('#send');
    var content = $('#content');
    var current = $('#current');
    var warning = $('#browserWarning');
    var user = getUser();
    var username = $('#username');
    var update = $('#update');
    var id;

    function User() {

    }

    //Hide warning
    warning.hide();

    //set username and display it on screen
    username.val(user);
    currentUser();

    //event handlers
    send.click(submit);
    update.click(setUser);
    field.keyup(function (e) {
        if(e.which === 13) {
            submit();
        } 
    });
    username.keyup(function (e) {
        if(e.which === 13) {
            setUser();
        } 
    });

    socket.on('message', function(data) {
        if(data.content) {
            if(data.user === 'Hermes')
                classes = 'message text-muted ';
            else
                classes = 'message ';
            content
                .append('<p></p>')
                .find('p:last-child')
                .addClass(classes + id)
                .append('<span></span>')
                .find('span:last-child')
                .addClass('user')
                .text(data.user + ': ')
                .closest('.message')
                .append('<span></span>')
                .find('span:last-child')
                .addClass('content')
                .text(data.content)
        }
        else {
            console.log('There is a problem:', data);
        }
    });

    function getUser() {
        if(typeof(Storage) !== "undefined") {
            if(localStorage.getItem('username'))
                name = localStorage.getItem('username');
            else
                name = generateUsername();
        }
        else {
            warning.show();
        }
        return name;
    }

    function setUser() {
        localStorage.setItem('username', username.val() );
        if(username.val() === '')
            user = generateUsername();
        else
            user = username.val();
        currentUser();
    }

    function generateUsername() {
        return 'zd-' + Math.floor((Math.random() * 729) + 1);
    }

    function currentUser() {
        current.html('&nbsp;(Currently: ' + user + ')');
        socket.emit('join', user);
        console.log(user);
    }

    function submit() {
        var text = $('#field').val();
        socket.emit('send', { content: text, user: user });
        field.val('');
        content.animate({scrollTop: content.prop('scrollHeight')});
    }
});
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

From a once over:

  • There is no need to set views or view engine for the server part, you are not using it
  • Be careful about polluting the global namespace, variables like welcome, join and leaving should be declared with var
  • I would probably call APPNAME -> APPNICK, this tells me that this will be used for chat messages
  • From a memory perspective, you are creating a fresh set of functions on each connection. I would declare the 3 events handlers as functions outside of your connection handler, and then assign them

    function onSend(data) {
        message = new Message(data.content, data.user);
        io.sockets.emit('message', message);
    }
    
    function onJoin(data) {
        join = new Message(data + " has joined the chat.", APPNAME);
        io.sockets.emit('message', join);
    }
    
    function onDisconnect() {
        leaving = new Message("Someone has left.", APPNAME);
        io.sockets.emit('message', leaving);
    }
    
    //Event Handlers
    io.sockets.on('connection', function(socket) {
        welcome = new Message("Welcome to the ZeroDae chat service.", APPNAME);
        socket.emit('message', welcome );
        socket.on('send', onSend);
        socket.on('join', onJoin);
        socket.on('disconnect', onDisconnect);
    });
    
  • From a readability and annoyance perspective I would modify Message so that the user gets defaulted to APPNAME or APPNICK

    function Message(content, user) {
       this.content = content;
       this.user = user || APPNICK;
    }
    

    This way you can simply

    function onJoin(data) {
        join = new Message(data + " has joined the chat." );
        io.sockets.emit('message', join);
    }
    
  • Then from a DRY perspective, you are repeating io.sockets.emit('message', a ton of times. What if you made the sending of a Message a function of it ?

    Message.prototype.send = function(){
      io.sockets.emit('message', this);
    }
    

    Then your 3 handlers would look like this:

    function onSend(data) {
        new Message(data.content, data.user).send();
    }
    
    function onJoin(data) {
        new Message(data + " has joined the chat.").send();
    }
    
    function onDisconnect() {
        new Message("Someone has left.").send();
    }
    

    The next thing that would bother me is the repeat new, but I will stop here ;)

For the client part

  • 13 should be a constant var ENTER = 13 with if(e.which === ENTER ) {
  • This :

        content
            .append('<p></p>')
            .find('p:last-child')
            .addClass(classes + id)
            .append('<span></span>')
            .find('span:last-child')
            .addClass('user')
            .text(data.user + ': ')
            .closest('.message')
            .append('<span></span>')
            .find('span:last-child')
            .addClass('content')
            .text(data.content)
    

    looks impressive, but is definitely not the most performing piece of code, but it is fast enough, then I would leave it alone.

  • console.log('There is a problem:', data); <- Problems happen with chat, I would make this part of the DOM, users will thank you for it

Fun piece of code, easy to follow.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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