Stack Code Review

AJAX chat client

maria

Problem

I have code that is used for retrieving and storing conversation for a chat application. I found this from a tutorial and tweak it a little bit. This script works fine but my concern is if it’s the standard way. Can somebody please check this for me if to see if it’s deprecated or a bad practice? And is there any other way of writing this script?

 //Gets the current messages from the server
            function getChatText() {
                if (receiveReq.readyState == 4 || receiveReq.readyState == 0) {
                    receiveReq.open("GET", 'includes/getChat.php?chat='+uid+'&last=' + lastMessage, true);
                    receiveReq.onreadystatechange = handleReceiveChat; 
                    receiveReq.send(null);
                }           
            }
            //Add a message to the chat server.
            function sendChatText() {

                if (sendReq.readyState == 4 || sendReq.readyState == 0) {
                    sendReq.open("POST", 'includes/getChat.php?last=' + lastMessage, true);
                    sendReq.setRequestHeader('Content-Type','application/x-www-form-urlencoded');
                    sendReq.onreadystatechange = handleSendChat; 
                    var param = 'message=' + document.getElementById('txtA').value;
                    param += '&name='+user;
                    param += '&uid='+uid;
                    param += '&rid='+document.getElementById('trg').value;
                    sendReq.send(param);
                    document.getElementById('txtA').value = '';
                }                           
            }
            //When our message has been sent, update our page.
            function handleSendChat() {
                //Clear out the existing timer so we don't have 
                //multiple timer instances running.
                clearInterval(mTimer);
                AjaxRetrieve();

            }
            function AjaxRetrieve()
            {
                var rid = document.getElementById('trg').value;
                $.get('includes/getChat.php?chat='+uid + '&rid=' + rid + '&name=' + user,function(data)
                {

                   $("#clog").html(data);



                });
            }

Solution

This is really odd. As I read the code, I was thinking you’d go all-native. Then, suddenly a $.get is found at the bottom. You had jQuery all along! Why not go ahead and use it for all the AJAX functions you have. Overhead is not really a concern here, but readability and maintainability is. Use jQuery all the way, keeps your code short.

// If they remain static, then move them out so you won't be running around the DOM
var message = $('#txtA').val();
var roomId = $('#trg').val();
var clog = $("#clog");

//Gets the current messages from the server
function getChatText() {
  $.get('includes/getChat.php', {
    chat: uid,
    last: lastMessage
  }, handleReceiveChat);
}

//Add a message to the chat server.
function sendChatText() {

  $.post('includes/getChat.php', {
    last: lastMessage,
    message: message,
    name: user,
    uid: uid,
    rid: roomId
  }, handleSendChat);
  message.val('');
}

function handleSendChat() {
  // No need to stop the timers. Have it run continuously.
}
// This function should retrieve the latest messages, and not everything.
// Also, if you are polling, use one timer that perpetually runs.
function AjaxRetrieve() {
  $.get('includes/getChat.php', {
    chat: uid,
    rid: roomId,
    name: user
  }, function (data) {
    clog.html(data);
  });
}

Other things I notice:

  • You manually pick out the values to send to the server. Why not place everything in a form, like place it in hidden input tags, and then take advantage of jQuery.serialize. This makes your code much easier by not doing document.getElement...value. You can hook up the form with a submit handler, prevent the normal submit, serialize the form and do ajax.

    <form id="chat">
  ...
  <input type="text" name="message">
  ...
</form>

<script>
  $('#chat').on('submit',function(event){
    // Prevent the normal submit
    event.preventDefault(); 
    // From here, it's as simple as a grab and go
    var formData = $(this).serialize();
    $.post('includes/getChat.php',formData,function(){...});
  });
</script>

  • Polling is a good starter for doing semi-live chat. But that’s stressful for the server. Imagine millions of people using your app, polling every 1 second for live data. That’s 60 million requests to your server a minute! I suggest you look into WebSockets for a much more efficient implementation.

Exit mobile version