2
\$\begingroup\$

I currently have an HTML <Table> with all the socket.io-rooms. Client-side, the table gets fetched if you visit the page. If some room gets created or deleted, it get updated with the following code. There's also a button that gets updated to show if the room is full or not.

Now I want to know if there's any "better" way. The argument is room_list (array with objects for each room). Should I split it into two functions (fetchRoomList and updateRoomList) or use "pure" JavaScript instead of jQuery? Any other ideas?

socket.on( 'updateRoomsList', function ( room_list ) {
    var i;
    for (i = room_list.length - 1; i >= 0; i -= 1) {
        var r = room_list[ i ];
        var el = $( '.room-' + r.id ).length;
        var parts = [];

        // Create Row
        if( !el ) {
            parts.push( '<tr class="room-' + r.id + '">' );
            parts.push( '<td>' + r.id + '</td>' );
            parts.push( '<td>' + r.name + '</td>' );
            parts.push( '<td class="room-players">' + r.players.length + ' / ' +r.slots + '</td>' );
            parts.push( '<td class="room-button"><a class="btn btn-block btn-success btn-xs" onclick="joinRoom(' + r.id + ');">JOIN</a></td>' );
            $( '.lobby-container .room-list tbody' ).prepend( parts.join() );
        } 
        else // Update Row
        {
            $( '.lobby-container .room-list tbody .room-' + r.id + ' .room-players' ).text( r.players.length + ' / ' +r.slots );
            if( r.players.length >= r.slots ) { // Full Room
                $( '.lobby-container .room-list tbody .room-' + r.id + ' .room-button .btn' )
                    .text( 'FULL' )
                    .addClass( 'btn-warning' )
                    .removeClass( 'btn-success' )
                    .addClass( 'disabled' );
            } else { // Not full
                $( '.lobby-container .room-list tbody .room-' + r.id + ' .room-button .btn' )
                    .text( 'JOIN' )
                    .addClass( 'btn-success' )
                    .removeClass( 'btn-warning' )
                    .removeClass( 'disabled' );
            }
        }
    }
} );
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Your code looks quite clear, and I see only some possible improvements.
Regarding your questions:

  • I don't think it'd better to split into fetchRoomList and updateRoomList separate functions: there are a few common vars that should be computed twice; additionally it'd prevent increasing performance like I suggest below (by traversing HTML tree less times).
  • using pure JS instead of jQuery would obviously increase performance a bit, but IMO it doesn't worth the negative impact on readability.

Then I can suggest what follows:

  1. caching jQuery traversal:
    • currently you're looking:
      1. for .room-X once for each room
      2. for .lobby-container .room-list tbody once for each room to add a <tr class="room-X"> (when creating row)
      3. for .lobby-container .room-list tbody .XXX, where XXX is .room-players (once), then .room-btn .btn (twice).
    • So to create C rows and update U rows you spend 2xC + 4xU "big" traversals for each room.
    • Instead you can:
      1. first get var $rooms = $('.lobby-container .room-list tbody'); once for all
      2. then look for $room = $(#${id}, $rooms); once for each room
      3. and use $rooms.prepend() when creating row or $('XXX', $room) when updating room (these are only "little" traversals, since inside of $rooms).
    • this way you spend only 1 "big" traversal, plus 0xC + 3xU "little" traversals.
      Note that room-X is unique so to be more strictly significant you might change the class into id.
  2. less important: you also can cache r.id (used several times)
  3. factorizing operations when creating room:
    • depending on r.players.length >= r.slots you execute different but analogue operations on the room, using twice the same jQuery traversal
    • instead you can get the condition var isFull = r.players.length >= r.slots;, then use it in each operation, all operations being attached to the same unique traversal
    • this way you actually spend only 1 "big" traversal, plus 0xC + 2xU "little" traversal
  4. Last point, I suggest using tagged strings for row creation, which at the same time reduces code and improves readability. You'll notice I also choosed to omit the closing tags </tr> and </td> (optional in HTML5).

Here is the whole refactored function:

socket.on( 'updateRoomsList', function (room_list) {
  var $rooms = $('.lobby-container .room-list tbody');
  for (var i = room_list.length - 1; i >= 0; i -= 1) {
    var r = room_list[i],
        id = `room-${r.id}`,
        $room = $(`#${id}`, $rooms);
    if (!$rooms.length) {
      // Create row
      $rooms.prepend(`
<tr id="${id}">
  <td>${r.id}
  <td>${r.name}
  <td class="room-players">${r.players.length}/${r.slots}
  <tdclass="room-button">
    <a class="btn btn-block btn-success btn-xs" onclick="joinRoom(${r.id});">
      JOIN
    </a>
        `);
    } else {
      // Update row
      $('.room-players', $room).text(`${r.players.length}/${r.slots}`);
      var isFull = r.players.length >= r.slots;
      $('.room-button .btn', $room)
        .text(isFull ? 'FULL' : 'JOIN')
        .toggleClass('btn-warning disabled', isFull)
        .toggleClass('btn-success', !isFull);
    }
  }
}
\$\endgroup\$