Problem
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 <message> - Give something for the bot to say!<br>/broadcast <message> - 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();
}
});
Solution
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
insidesetupSocket
.
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 <message> - Give something for the bot to say!<br>/broadcast <message> - 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 <message> - Give something for the bot to say!'
+ '<br>/broadcast <message> - 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 <message> - Give something for the bot to say!'
+ '<br>/broadcast <message> - 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 tohandleChat
while the latter is only concerned with checking for a valid message. Likewise forparseMessage
andformatMessage
: 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
, andmessageToBeSent
.
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 tohandleMessage
. - Find better names for
handleChat
andhandleMessage
. I would reservehandle
for functions bound to commands, thoughhandleHelp
does make sense. Naming can be tough at times, but a good name goes a long way. Don’t be afraid of longer names likeformatAndSendMessage
to remove confusion. - Consider extracting
sendMessage(original, formatted)
fromhandleChat
so the latter doesn’t do any parsing, formatting, length-checking, or sending but merely ties those all together.