Creating chat box with comet

Question

I'm writing a chat application. I would like to know if there are problems in this code.

This function sends message:

function sendMessage()
{
    var text = $('#messageBox').val();
    $('#messageBox').val('');
    if($('.context').scrollTop() + $('.context').innerHeight() >= $('.context')[0].scrollHeight)
    {
        $(".context").animate({ scrollTop: $('.context')[0].scrollHeight}, 1000);
    }
    $('.context').append('<div class="inner">'+ text +'</div>');
    if(text != '')
    {
        $('#send').attr('disabled', 'disabled');
        $.ajax({
            url: 'http://localhost/chat/display',
            async: true,
            type: "POST",
            data: 'text=' + text,
            cache: false,
            //dataType: 'json',
            success: function (msg) {
                if (msg == 1)
                {     
                    $('#send').removeAttr('disabled');
                }
            },
            error: function (msg) {
                alert('app has an error!');
            }
        });
    }
}

When the user sends a message, the message is written on the page before saving.

This code comes before the AJAX call. Is it right?

$('.context').append('<div class="inner">'+ text +'</div>');

This function shows messages:

$(document).ready(function () {
    $(window).load(displayMessage(lastTimestamp));
});

function displayMessage(time)
{
    $.ajax({
        url: 'http://localhost/chat/display',
        async: true,
        type: "POST",
        data: 'time=' + time,
        cache: false,
        dataType: 'json',
        success: function (msg) {
            if(msg)
            {
                if($('.context').scrollTop() + $('.context').innerHeight() >= $('.context')[0].scrollHeight)
                {
                    $(".context").animate({ scrollTop: $('.context')[0].scrollHeight}, 1000);
                }
                $('.context').append(msg[0]);
                time = msg[1];
            }
            setTimeout(displayMessage, 2000, time);
        },
        error: function (msg) {
            displayMessage(time);
        }
    });
}

After the page loads, this function will be run to send data to be displayed.

Display method:

public function display()
    {
        if(isset($_POST['text']))
        {
            $text = DB::Escape($_POST['text']);
            $time = time();
            DB::Query("INSERT INTO `messages` (`user_id`, `to_message_id`, `text`, `timestamp`) VALUES ('{$_SESSION['id']}', '1', '{$text}', '{$time}')");
            if(DB::AffectedRows() > 0)
            {
                return 1;
            }
            else
            {
                return DB::LastError();
            }
        }

        if(isset($_POST['time']))
        {
            $time = intval($_POST['time']);
            $current = time();
            while (time() - $current < 10)
            {
                $result = self::findAll("WHERE (`timestamp` > '{$time}' AND `user_id` != '{$_SESSION['id']}')");
                if(count($result) > 0)
                {
                    $html = '';
                    foreach ($result as $obj)
                    {
                        $html .= '<div class="inner">'. $obj->text .'</div>';
                    }
                    return json_encode(array($html, $obj->timestamp));
                }
                else
                {
                    sleep(3);
                }
            }
        }
    }

Note this section:

while (time() - $current < 10)

This code slows sending and receiving messages so it takes several seconds to send and insert a message when I press Enter or click the sending button.

I'm using last timestamp for selecting messages from database. Is there a problem as the same time in timestamp column? I'd like to fix this apparent performance problem.

Answer 1

This code comes before the AJAX call. Is it right?

I would suggest adding the message to the DOM via the success callback - that way, if an error occurs then the message isn't displayed on the page.

Other feedback

Javascript

Cache DOM lookups

I would suggest you cache DOM references - e.g. store $('#messageBox'), $('#send'), $('.context') in variables (or better yet, constants if you are using ecmascript-6), preferably in a DOM-ready callback, because DOM-lookups aren't exactly cheap....

Accessing elements by class name can return multiple

Also: $('.context') is equivalent to document.getElementsByClassName()... which returns multiple elements. While methods like .scrollTop() and .innerHeight() operate for the first matched element, it would be more appropriate to select an element by id (i.e. with an id selector).

$(document).ready() with nested $(window).load()

I don't believe it is necessary to add the window load callback within the DOM ready callback. Also, bearing in mind that you posted this code a couple years ago, .load() is now deprecated as of jQuery version 1.8, because there is a new method .load() in the AJAX section.

Note that in the line below, displayMessage() is called and does not return a function, so the call to $(window).load() appears superfluous:

$(window).load(displayMessage(lastTimestamp));

To really make that function displayMessage an onload handler, one could make a partially applied function using Function.bind() :

$(window).load(displayMessage.bind(null, lastTimestamp));

PHP

Separate endpoints

It would be wise to separate the code in the display function into two different endpoints - one for fetching messages (which should perhaps use the GET HTTP verb), and one for creating a new post (which can stay as a POST or a PUT request). That way display isn't concerned with creating messages, so as to adhere to the Separation of Concerns principle.

Laravel code?

It looks like some of the Laravel DB facade and Eloquent methods (e.g. findAll()) are used, which makes me believe this is a Laravel controller method. If that is the case, it would be wise to access POST data via the Request object. Then lines like

$text = DB::Escape($_POST['text']);

Can be updated to use the Request::input() method:

$text = DB::Escape($request->input('text'));

Or using the dynamic input style:

$text = DB::Escape($request->text);