Creating chat box with comet












3












$begingroup$


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.










share|improve this question











$endgroup$

















    3












    $begingroup$


    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.










    share|improve this question











    $endgroup$















      3












      3








      3





      $begingroup$


      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.










      share|improve this question











      $endgroup$




      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.







      javascript php jquery performance ajax






      share|improve this question















      share|improve this question













      share|improve this question




      share|improve this question








      edited May 2 '18 at 23:38









      Sᴀᴍ Onᴇᴌᴀ

      9,88162165




      9,88162165










      asked Jan 27 '15 at 22:58









      GarmeGarme

      384




      384






















          1 Answer
          1






          active

          oldest

          votes


















          3












          $begingroup$


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





          share|improve this answer











          $endgroup$













            Your Answer





            StackExchange.ifUsing("editor", function () {
            return StackExchange.using("mathjaxEditing", function () {
            StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
            StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
            });
            });
            }, "mathjax-editing");

            StackExchange.ifUsing("editor", function () {
            StackExchange.using("externalEditor", function () {
            StackExchange.using("snippets", function () {
            StackExchange.snippets.init();
            });
            });
            }, "code-snippets");

            StackExchange.ready(function() {
            var channelOptions = {
            tags: "".split(" "),
            id: "196"
            };
            initTagRenderer("".split(" "), "".split(" "), channelOptions);

            StackExchange.using("externalEditor", function() {
            // Have to fire editor after snippets, if snippets enabled
            if (StackExchange.settings.snippets.snippetsEnabled) {
            StackExchange.using("snippets", function() {
            createEditor();
            });
            }
            else {
            createEditor();
            }
            });

            function createEditor() {
            StackExchange.prepareEditor({
            heartbeatType: 'answer',
            autoActivateHeartbeat: false,
            convertImagesToLinks: false,
            noModals: true,
            showLowRepImageUploadWarning: true,
            reputationToPostImages: null,
            bindNavPrevention: true,
            postfix: "",
            imageUploader: {
            brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
            contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
            allowUrls: true
            },
            onDemand: true,
            discardSelector: ".discard-answer"
            ,immediatelyShowMarkdownHelp:true
            });


            }
            });














            draft saved

            draft discarded


















            StackExchange.ready(
            function () {
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f78819%2fcreating-chat-box-with-comet%23new-answer', 'question_page');
            }
            );

            Post as a guest















            Required, but never shown

























            1 Answer
            1






            active

            oldest

            votes








            1 Answer
            1






            active

            oldest

            votes









            active

            oldest

            votes






            active

            oldest

            votes









            3












            $begingroup$


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





            share|improve this answer











            $endgroup$


















              3












              $begingroup$


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





              share|improve this answer











              $endgroup$
















                3












                3








                3





                $begingroup$


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





                share|improve this answer











                $endgroup$




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






                share|improve this answer














                share|improve this answer



                share|improve this answer








                edited 4 hours ago

























                answered May 2 '18 at 23:49









                Sᴀᴍ OnᴇᴌᴀSᴀᴍ Onᴇᴌᴀ

                9,88162165




                9,88162165






























                    draft saved

                    draft discarded




















































                    Thanks for contributing an answer to Code Review Stack Exchange!


                    • Please be sure to answer the question. Provide details and share your research!

                    But avoid



                    • Asking for help, clarification, or responding to other answers.

                    • Making statements based on opinion; back them up with references or personal experience.


                    Use MathJax to format equations. MathJax reference.


                    To learn more, see our tips on writing great answers.




                    draft saved


                    draft discarded














                    StackExchange.ready(
                    function () {
                    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f78819%2fcreating-chat-box-with-comet%23new-answer', 'question_page');
                    }
                    );

                    Post as a guest















                    Required, but never shown





















































                    Required, but never shown














                    Required, but never shown












                    Required, but never shown







                    Required, but never shown

































                    Required, but never shown














                    Required, but never shown












                    Required, but never shown







                    Required, but never shown







                    Popular posts from this blog

                    How to make a Squid Proxy server?

                    Is this a new Fibonacci Identity?

                    19世紀