patternjavascriptMinor
Creating chat box with comet
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:
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?
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);
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
Accessing elements by class name can return multiple
Also:
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,
Note that in the line below,
To really make that function
PHP
Separate endpoints
It would be wise to separate the code in the
Laravel code?
It looks like some of the Laravel
Can be updated to use the
Or using the dynamic input style:
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.