6
\$\begingroup\$

I am developing a chat program (mountreus-chat and GitHub) in Node.js using Socket.io and my code looks awful. I'm starting to use commands and now it's even worse.

Here's a code snippet (you can find every detail on the Github repo):

io.on('connection', function(socket){
      if(socketConnections().length <= 1024){
        socket.username = socket.handshake.query.username;
        socket.toString = function(){return this.name};
        socket.join(socket.handshake.query.room);
        socket.on('postName', function(username){
                socket.username = username;
                });
        var socketsConnected = socketConnections(socket.handshake.query.room, "/");
        io.emit('connections', socketsConnected.length + 1);
        socket.on('chat message', function(msg){
            var markedMessage =  markdown.renderInline(msg.message);
            var messageDate = moment(msg.date).format("LT, D/M");
            var firstWord = markedMessage.substr(0, markedMessage.indexOf(" "));
            if(msg.message !== "/help"){
            if(firstWord === "/broadcast"){
            var finalMessage = markedMessage.substr(markedMessage.indexOf(" ") + 1);
             var messageToBeSent = '<p class="alignLeft"> BROADCAST: ' + finalMessage + '</p><p class="alignRight">' + messageDate + '</p>';
            }else if(firstWord === "/bot-say"){
                var finalMessage = markedMessage.substr(markedMessage.indexOf(" ") + 1);
                var messageToBeSent = '<p class="alignLeft"> Chat bot: ' + finalMessage + '</p><p class="alignRight">' + messageDate + '</p>';
            }else{
            var messageToBeSent = '<p class="alignLeft">' + escapeHTML(msg.username) + ': ' + markedMessage + '</p><p class="alignRight">' + messageDate + '</p>';
            }
            if(messageToBeSent.length <= 8192){
                  if(!verifyEmptyness(msg.message)){
                    io.to(socket.handshake.query.room).emit('chat message', messageToBeSent);
                  }else{
                  socket.emit('chat message', 'PM: Sorry, you cannot send empty messages.');
                  }
                }else{
                  socket.emit('chat message', 'PM: Oops! You cannot send messages longer than 8192 characters. Sorry!');
                }
            }else{
                socket.emit('chat message', 'Montreus Chat - v1.3.3<br>Available commands:<br>/help - Display help commands<br>/bot-say &lt;message&gt; - Give something for the bot to say!<br>/broadcast &lt;message&gt; - Broadcast a message');
            }
        });
      socket.on('users', function(){
                var socketsConnected = socketConnections(socket.handshake.query.room, "/");
                io.to(socket.handshake.query.room).emit('connections', socketsConnected.length + 1);
                });
      socket.on('disconnect', function(){
                var socketsConnected = socketConnections(socket.handshake.query.room, "/");
                io.to(socket.handshake.query.room).emit('connections', socketsConnected.length + 1);
                });
      }else{
      socket.emit('chat message', 'PM: Sorry, we cannot allow more than 1024 connections in the server');
      socket.emit('chat message', 'PM: Disconnecting! Try again later.');
      socket.emit('connections', 'You are not connected.');
      socket.disconnect();
      }
      });
Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
5
\$\begingroup\$

You can gain a lot of readability by formatting the code (every decent editor will do a passable job at this with a single keystroke) and extracting several small functions (refactoring). I'll start with this but only extract the reusable functions.

Note: I assumed you can emit the initial connection count after binding the commands. If not, move the first call to emitConnectionCount inside setupSocket.

io.on('connection', function (socket) {
    if (socketConnections().length <= 1024) {
        setupSocket();
        emitConnectionCount();
    } else {
        disconnect();
    }

    function setupSocket() {
        socket.username = socket.handshake.query.username;
        socket.toString = function () {
            return this.name
        };
        socket.join(socket.handshake.query.room);
        socket.on('postName', handleUsername);
        socket.on('chat message', handleMessage);
        socket.on('users', emitConnectionCount);
        socket.on('disconnect', emitConnectionCount);
    }

    function disconnect() {
        emitToUser('PM: Sorry, we cannot allow more than 1024 connections in the server');
        emitToUser('PM: Disconnecting! Try again later.');
        emitToUser('You are not connected.', 'connections');
        socket.disconnect();
    }

    function handleUsername(username) {
        socket.username = username;
    }

    function handleMessage(msg) {
        var markedMessage = markdown.renderInline(msg.message);
        var messageDate = moment(msg.date).format("LT, D/M");
        var firstWord = markedMessage.substr(0, markedMessage.indexOf(" "));
        var messageToBeSent;
        var finalMessage;
        if (msg.message === "/help") {
            emitToUser('Montreus Chat - v1.3.3<br>Available commands:<br>/help - Display help commands<br>/bot-say &lt;message&gt; - Give something for the bot to say!<br>/broadcast &lt;message&gt; - Broadcast a message');
        } else if (firstWord === "/broadcast") {
            finalMessage = markedMessage.substr(markedMessage.indexOf(" ") + 1);
            messageToBeSent = '<p class="alignLeft"> BROADCAST: ' + finalMessage + '</p><p class="alignRight">' + messageDate + '</p>';
        } else if (firstWord === "/bot-say") {
            finalMessage = markedMessage.substr(markedMessage.indexOf(" ") + 1);
            messageToBeSent = '<p class="alignLeft"> Chat bot: ' + finalMessage + '</p><p class="alignRight">' + messageDate + '</p>';
        } else {
            messageToBeSent = '<p class="alignLeft">' + escapeHTML(msg.username) + ': ' + markedMessage + '</p><p class="alignRight">' + messageDate + '</p>';
        }
        if (messageToBeSent.length <= 8192) {
            if (!verifyEmptyness(msg.message)) {
                emitToRoom(messageToBeSent);
            } else {
                emitToUser('PM: Sorry, you cannot send empty messages.');
            }
        } else {
            emitToUser('PM: Oops! You cannot send messages longer than 8192 characters. Sorry!');
        }
    }

    function emitToRoom(message, command) {
        io.to(socket.handshake.query.room).emit(command || 'chat message', message);
    }

    function emitToUser(message, command) {
        socket.emit(command || 'chat message', message);
    }

    function emitConnectionCount() {
        var socketsConnected = socketConnections(socket.handshake.query.room, "/");
        emitToRoom(socketsConnected.length + 1, 'connections');
    }
});

That's much easier to grok already, but let's tackle that complicated handleMessage function next. What are its responsibilities?

  • Parse the message to extract an optional command (/help, /broadcast, and /bot-say).
  • Handle the help command.
  • Fail if the message is empty.
  • Format the message using Markdown.
  • Embed it into an HTML snippet.
  • Fail if the formatted message is too long.
  • Emit the formatted message.

Wow, that's a lot of work--too much for a single function. Let's simplify it by extracting functions for emitting the help text and formatting the message.

function handleMessage(chat) {
    var message = chat.message;
    if (message === "/help") {
        handleHelp();
    } else {
        var split = message.indexOf(" ");
        var command;
        if (split === -1) {
            command = '/chat';
        } else {
            command = message.substr(0, split);
            message = message.substr(split);
        }
        if (verifyEmptyness(message)) {
            emitToUser('PM: Sorry, you cannot send empty messages.');
        } else {
            var realMessage = formatMessage(command, message, chat.username, moment(chat.date).format("LT, D/M"));
            if (realMessage.length > 8192) {
                emitToUser('PM: Oops! You cannot send messages longer than 8192 characters. Sorry!');
            } else {
                emitToRoom(realMessage);
            }
        }
    }
}

function formatMessage(command, message, user, date) {
    var intro;
    switch (command) {
        case '/broadcast':
            intro = 'BROADCAST';
            break;
        case '/bot-say':
            intro = 'Chat bot';
            break;
        case '/chat':
            intro = escapeHTML(user);
            break;
        default:
            intro = 'UNKNOWN COMMAND';
            message = command;
            break;
    }
    return '<p class="alignLeft"> ' + intro + ': ' + message + '</p><p class="alignRight">' + date + '</p>';
}

function handleHelp() {
    sendToUser(
        'Montreus Chat - v1.3.3'
        + '<br>Available commands:'
        + '<br>/help - Display help commands'
        + '<br>/bot-say &lt;message&gt; - Give something for the bot to say!'
        + '<br>/broadcast &lt;message&gt; - Broadcast a message'
    );
}

That's better, but handleMessage is still doing too much. I don't like that it has to parse the original message, check for an empty chat message, and send the formatted message if it's not too long. Let's extract the parsing first.

function handleMessage(chat) {
    if (chat.message === '/help') {
        handleHelp();
    } else {
        var parsed = parseMessage(chat.message);
        if (verifyEmptyness(parsed.message)) {
            emitToUser('PM: Sorry, you cannot send empty messages.');
        } else {
            var message = formatMessage(parsed.command, parsed.message, chat.username, moment(chat.date).format("LT, D/M"));
            if (message.length > 8192) {
                emitToUser('PM: Oops! You cannot send messages longer than 8192 characters. Sorry!');
            } else {
                emitToRoom(chat, command, message);
            }
        }
    }
}

function parseMessage(message) {
    var split = message.indexOf(' ');
    if (split === -1 || message.charAt(0) !== '/') {
        return {command: '/chat', message: message};
    } else {
        return {command: message.substr(0, split), message: message.substr(split)};
    }
}

If you don't mind formatting an empty message that won't be sent, you can extract the format/check/send logic to yet another function.

function handleMessage(chat) {
    if (chat.message === '/help') {
        handleHelp();
    } else {
        handleChat(chat);
    }
}

function handleChat(chat) {
    var parsed = parseMessage(chat.message);
    var formatted = formatMessage(parsed.command, parsed.message, chat.username, moment(chat.date).format("LT, D/M"));
    if (verifyEmptyness(parsed.message)) {
        emitToUser('PM: Sorry, you cannot send empty messages.');
    } else if (formatted.length > 8192) {
        emitToUser('PM: Oops! You cannot send messages longer than 8192 characters. Sorry!');
    } else {
        emitToRoom(chat, command, formatted);
    }
}

Here's the final product (after a little reorganization) for ease of understanding and discussion:

io.on('connection', function (socket) {
    if (socketConnections().length > 1024) {
        disconnect();
    } else {
        setupSocket();
        emitConnectionCount();
    }

    function disconnect() {
        emitToUser('PM: Sorry, we cannot allow more than 1024 connections in the server');
        emitToUser('PM: Disconnecting! Try again later.');
        emitToUser('You are not connected.', 'connections');
        socket.disconnect();
    }

    function setupSocket() {
        socket.username = socket.handshake.query.username;
        socket.toString = function () {
            return this.name
        };
        socket.join(socket.handshake.query.room);
        socket.on('postName', handleUsername);
        socket.on('chat message', handleMessage);
        socket.on('users', emitConnectionCount);
        socket.on('disconnect', emitConnectionCount);
    }

    function handleUsername(username) {
        socket.username = username;
    }

    function handleMessage(chat) {
        if (chat.message === '/help') {
            handleHelp();
        } else {
            handleChat(chat);
        }
    }

    function handleHelp() {
        sendToUser(
            'Montreus Chat - v1.3.3'
            + '<br>Available commands:'
            + '<br>/help - Display help commands'
            + '<br>/bot-say &lt;message&gt; - Give something for the bot to say!'
            + '<br>/broadcast &lt;message&gt; - Broadcast a message'
        );
    }

    function handleChat(chat) {
        var parsed = parseMessage(chat.message);
        var formatted = formatMessage(parsed.command, parsed.message, chat.username, moment(chat.date).format("LT, D/M"));
        if (verifyEmptyness(parsed.message)) {
            emitToUser('PM: Sorry, you cannot send empty messages.');
        } else if (formatted.length > 8192) {
            emitToUser('PM: Oops! You cannot send messages longer than 8192 characters. Sorry!');
        } else {
            emitToRoom(chat, command, formatted);
        }
    }

    function parseMessage(message) {
        var split = message.indexOf(' ');
        if (split === -1 || message.charAt(0) !== '/') {
            return {command: '/chat', message: message};
        } else {
            return {command: message.substr(0, split), message: message.substr(split)};
        }
    }

    function formatMessage(command, message, user, date) {
        var intro;
        switch (command) {
            case '/broadcast':
                intro = 'BROADCAST';
                break;
            case '/bot-say':
                intro = 'Chat bot';
                break;
            case '/chat':
                intro = escapeHTML(user);
                break;
            default:
                intro = 'UNKNOWN COMMAND';
                message = command;
                break;
        }
        return '<p class="alignLeft"> ' + intro + ': ' + message + '</p><p class="alignRight">' + date + '</p>';
    }

    function emitConnectionCount() {
        emitToRoom(socketConnections(socket.handshake.query.room, "/").length + 1, 'connections');
    }

    function emitToRoom(message, command) {
        io.to(socket.handshake.query.room).emit(command || 'chat message', message);
    }

    function emitToUser(message, command) {
        socket.emit(command || 'chat message', message);
    }
});

The main benefits of this version over the original are

  • Short functions with descriptive names make it easier to understand the program in small pieces and verify its correctness.
  • Repeated code has been deduplicated with reusable functions with sane defaults.
  • Separating responsibilities into functions allows each function to live at the same abstraction level. For example, handleMessage is only concerned with detecting /help or passing off to handleChat while the latter is only concerned with checking for a valid message. Likewise for parseMessage and formatMessage: they are both at even lower abstraction levels.
  • Splitting the functions also allowed reusing variable names across functions to avoid wordy repetition such as markedMessage, finalMessage, and messageToBeSent.

Here are some suggested improvements:

  • Add JSDoc documentation to each function that requires further explanation, especially those that take parameters. It would also be good to document the chat object passed to handleMessage.
  • Find better names for handleChat and handleMessage. I would reserve handle for functions bound to commands, though handleHelp does make sense. Naming can be tough at times, but a good name goes a long way. Don't be afraid of longer names like formatAndSendMessage to remove confusion.
  • Consider extracting sendMessage(original, formatted) from handleChat so the latter doesn't do any parsing, formatting, length-checking, or sending but merely ties those all together.
Improve this answer
\$\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.