2
\$\begingroup\$

I have a simple socket.io server written in node. Everything is working just fine, however I am curious whether what I am doing is acceptable. I am especially interested in this line

listOfMessages = listOfMessages.slice(-1000);

Essentially what I am doing is each time a new user connects, I resize the array and send the newly connected user the last 1000 messages.

I do not want to let the array grow without bounds since the server can stay running indefinitely.

var listOfMessages = [];

io.on('connection', function(socket){
  console.log('a user connected (id=' + socket.id + ').');

  // This way list of messages doesn't grow larger than 1000...
  listOfMessages = listOfMessages.slice(-1000); 
  // Loop over the list of messages and send them to the new connections
  for (index = 0; index < listOfMessages.length; index++) {
      socket.emit('chat message', listOfMessages[index]);
  }

  // When we recieve a message
  socket.on('chat message', function(msg){
    console.log('message: ' + msg + ' from client (id=' + socket.id + ').');
    socket.broadcast.emit('chat message', msg);
    listOfMessages.push(msg);
  });
});

Some feedback on whether this is acceptable or good practice is what I'm looking for.

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

From what I understand, your code only keeps at most 1000 messages. First, you need to move out 1000 to a variable and away from your logic. That way, it's visible and easily configured.

Next, there is a possibility that your list of messages will grow greater than a thousand because you only slice it off when a connection is made. What if people kept chatting, but nobody new connected? Your array will continually grow. I suggest you move that check to the "chat message" event.

Another way to do it is to just separate this slicing logic into something periodic, like run on intervals. That way, you don't have to slice every time. The drawback to this, though, is when people chat faster than you can slice off.

Now for the logic that keeps your history in check. slice is expensive because it does a shallow copy of your array. Having a full 1000 messages in the list, if a person connects, you'd have 2 arrays of length 1000 at one moment. I suggest you use shift when the count runs over 1000. Assuming this system is only a single process, pushing in the 1001th message should immediately shift off the oldest one. Still an expensive process, but it's a lesser evil.

Lastly, to get the latest 1000 messages, it's an overhead to send 1000 events down the line to the new user individually. I suggest you create a new event that sends all 1000 at once instead of individually.

Let's try optimizing your logic:

var listOfMessages = [];
var MAX_HISTORY_LENGTH = 1000;

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

  // Send down all old messages at once to this connection.
  socket.emit('new messages', listOfMessages);

  // When we recieve a message
  socket.on('chat message', function(msg){

    socket.broadcast.emit('chat message', msg);
    listOfMessages.push(msg);

    // Only splice when we need to
    if(listOfMessages.length > MAX_HISTORY_LENGTH) listOfMessages.shift();

  });
});
\$\endgroup\$

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.