HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Creating chat box with comet

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
chatcometcreatingwithbox

Problem

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(''+ text +'');
    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(''+ text +'');


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);

Solution

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);

Code Snippets

$(window).load(displayMessage(lastTimestamp));
$(window).load(displayMessage.bind(null, lastTimestamp));
$text = DB::Escape($_POST['text']);
$text = DB::Escape($request->input('text'));
$text = DB::Escape($request->text);

Context

StackExchange Code Review Q#78819, answer score: 3

Revisions (0)

No revisions yet.