Javascript - How to improve this code?

Asked

Viewed 191 times

4

As there is the Code Review Stack Exchange in Portuguese and Stack Overflow PT covers this topic I ask for help in a code that I am working on: suggestions and criticism regarding a chat script that I’m developing javascript to streamline an existing PHP.

This idea turned out to be a challenge for me because I can not change the source code of PHP since the script is a suggested improvement to a site that I use daily and the responsible for it does not have time at the moment, nor made available the code.

The current script works by generating a table where each sent message is composed of two <tr>, the first with three <td> the data of who sent and the second with a single <td> with the message. Sending messages is by a POST request, with the message in the mensagem and they are deleted with a GET request with the ID of the message to be deleted in the field del.

To streamline it I decided to implement a function that compares the current page with a new one charged via AJAX. Cases where messages are added and removed are considered, and there is also the warning of new messages per message at the top of the page and in the page title. I used jQuery because the library is already used on other pages of the site.
The current version of the code is published in that gist, and here too:

// ==UserScript==
// @name        Sugestões de Mudança para a Shoutbox
// @version     1.5.14
// @grant       none
// @updateURL   https://rawgit.com/qgustavor/fc66dc1aa54c2c3a9970/raw/shoutbox.js
// @downloadURL https://rawgit.com/qgustavor/fc66dc1aa54c2c3a9970/raw/shoutbox.js
// ==/UserScript==

;(function functionWrapper(){
  // Coloque `true` para voltar a forma antiga de apagar mensagens:
  var APAGAR_USANDO_CONFIRM = false;
  
  // Para testar só colar o código no console
  // (aperte F12 e ESC que deve abrir)
  // Se for bloqueado siga as instruções do navegador
  
  // Caso queira testar esse script como user script
  // as linhas abaixam lidam com a adaptação.
  if (typeof $ === 'undefined') {
    var el = document.createElement('script');
    el.innerHTML = '(' + functionWrapper  + '());';
    document.head.appendChild(el);
    return;
  }
  
  // Se quiser aplicar as mudanças no site
  // leia os comentários que explicam
  // o que o script altera.

  // Desative o script atual de atualização
  // As linhas abaixo fazem isso, porém
  // não é a melhor maneira e sim removendo.
  $.fn.load = $.noop;

  // Adicione esse código CSS em um arquivo 
  // O código da animação veio do css-spinners.com
  // então processado no AutoPrefixer e finalmente Devilo.us
  $('<style>', {
    html: '@-webkit-keyframes spinner-loader{0%{-webkit-transform:rotate(0);transform:rotate(0)}100%{-webkit-transform:rotate(360deg);transform:rotate(360deg)}}@keyframes spinner-loader{0%{-webkit-transform:rotate(0);transform:rotate(0)}100%{-webkit-transform:rotate(360deg);transform:rotate(360deg)}}.spinning{-webkit-animation:spinner-loader 1500ms infinite linear;animation:spinner-loader 1500ms infinite linear}.message-alert{position:absolute;text-align:center;background:#bbdefb ;left:0;top:0;right:0;padding:1em}.shoutbox_contain{height:245px/* valor padrão para navegadores antigos */;height:calc(100vh - 45px)}@-webkit-keyframes new-message{0%{background:#bbdefb}100%{background:#f4f4f4!important}}@keyframes new-message{0%{background:#bbdefb}100%{background:#f4f4f4!important}}.highlight{-webkit-animation:new-message 3s ease-in 1;animation:new-message 3s ease-in 1}.history-link-row{text-align:center;background:#f4f4f4}.history-link-row td{padding:1em}.shoutboxform_input{margin-left:10px;margin-right:150px}.shoutboxform_input input{width:100%}.shoutboxform_links{float:right;width:100px;text-align:center;padding:6px 0}.shoutboxform_links a{margin:0 5px}td:only-child a{word-break:break-all}form{margin-top:5px}'
  }).appendTo('head');
  
  // Substitua o formulário baseado em tabelas com um baseado em classes:
  $('form').html('<div class="shoutboxform_links"><a href="#" class="emoticon-link"><i title="Emoticons" class="icon-heart"></i></small></a><a href="?"><i title="Atualizar" class="icon-refresh"></i></a><a href="shoutbox.php?history=1" target="_top"><i title="Histórico" class="icon-book"></i></a></div><div class="shoutboxform_input input-append"><input class="shoutbox_msgbox" placeholder="→ CAMPO DE CHAT ←" type="text" name="message"><button class="btn" type="submit" name="submit">Enviar</button></div>');
  
  var DEFAULT_TITLE = top.document.title;
  var container = $('.shoutbox_contain');
  var containerBody = $('.shoutbox_contain tbody');
  var refreshIcon = $('.icon-refresh').click(loadAjax);
  var unreadMessages = $();
  var timeBetweenReloads;
  var timer;
  
  // Retorna a função de emoticons:
  $('.emoticon-link').on('click', function(evt) {
    evt.preventDefault();
    PopMoreSmiles('shoutboxform', 'message');
  });
  
  // Adiciona um link para ver o histórico de mensagens no final
  // Se for adicionado no HTML é melhor
  containerBody.append('<tr><td></td><td></td><td></td><tr>'); // As <tr> são sempre aos pares
  $('<tr>', {'class': 'history-link-row'}).appendTo(containerBody)
  .append($('<td>',{colspan:3}).append(
    $('<a>', {
    href: 'shoutbox.php?history=1',
    target: '_top', // _blank = nova janela, _top = na janela atual
    text: 'Ver histórico de mensagens' // ou 'Ver mensagens anteriores'
  })));

  setRefreshTimer();
  setMessageDeleteHandlers();
  
  function setMessageDeleteHandlers() {
    $('a[href*="shoutbox.php?del="][onclick]')
    .removeAttr('onclick')
    .on('click', handleMessageDelete);
  }
  
  function handleMessageDelete(evt) {
    var $this = $(this);
    var deleteHref = $this.attr('href').match(/\?(del=\d+)/)[1];
    
    evt.preventDefault();
    
    // Caso queira voltar a forma antiga só
    // alterar o valor dessa constante
    if (APAGAR_USANDO_CONFIRM) {
      if (confirm('Tem certeza que quer deletar esta mensagem?')) {
        loadAjax({ data: deleteHref });
      }
      return;
    }
    
    // Ao invés de confirm, precisa de alguns ajustes...
    showMessage('Você tem certeza? Clique nessa mensagem para confirmar', function() {      
      loadAjax({
        data: deleteHref
      });
    });
  }
  
  $('form').on('submit', handleMessageSent);
  
  function handleMessageSent(evt) {
    // Por praticidade (i.e. não ter que lembrar da documentação)
    // copiei isso do http://ginpen.com/2013/05/07/jquery-ajax-form/
    var $form = $(this),
        $input = $form.find('.shoutbox_msgbox');
        
    evt.preventDefault();
    
    loadAjax({
      url: $form.attr('action'),
      type: $form.attr('method'),
      data: $form.serialize()
    })
    .always(function () {
      $input.removeAttr('disabled').val('');
    });
    
    $input.attr('disabled', true).val('Enviando mensagem...');
  }

  // As duas funções abaixo chamam uma a outra
  // para que seja possível controlar o timer
  // e assim pará-lo quando clicar no .icon-refresh
  function refreshLoop() {
    loadAjax(setRefreshTimer);
  }
  
  function setRefreshTimer() {
    // Usar valor padrão de 30, caso não tenha sido definido
    timer = setTimeout(refreshLoop, timeBetweenReloads || 30e3);
  }

  // Carregar as novas mensagens usando $.ajax
  // Além disso adicionar uma animação de carregando
  // e parar o timer, evitando que a página
  // seja carregada várias vezes seguidas
  function loadAjax(opt) {
    // Se a função for chamada por um evento:
    if (opt instanceof $.Event) {
      opt.preventDefault();
      opt = {};
    }
    
    refreshIcon.addClass('spinning');
    clearTimeout(timer);
    
    return $.ajax('shoutbox.php', opt || {})
      .done(gotNewMessages)
      .fail(failedGettingMessages)
      .always(afterChatLoaded);
  }
  
  // Independente se carrregou ou não
  // parar a animação e agendar para carregar novamente
  function afterChatLoaded() {
    refreshIcon.removeClass('spinning');
    setRefreshTimer();
  }
  
  // Avisar caso as mensagens não carreguem e voltar
  // o tempo entre recarregamentos ao padrão
  function failedGettingMessages() {
    showMessage('Não foi possível carregar novas mensagens');
    timeBetweenReloads = null;
  }

  // Caso a página tenha sido carregada então processá-la
  function gotNewMessages(returnedHtml) {
    // Amarzena estado anterior de scroll antes de aplicar as modificações:
    var firstOldMessage = containerBody.children().first();
    var oldPosition = firstOldMessage.offset().top;
    var scrolledLength = container.scrollTop();
    
    // Tentar adivinhar quantas mensagens são novas
    // comparando a página atual com a carregada
    var currentMessages = $('tr td:only-child');
    var currentMessagesData = currentMessages.map(function (){
      return $(this).html();
    }).get();
    var returnedMessages = $('<div>', {html: returnedHtml})
    .find('.shoutbox_contain tr td:only-child');
    
    var lastIndex = -1;
    var newMessages = $();
    
    returnedMessages.each(function () {
      var $this = $(this);
      var elementData = $this.html();
      // O segundo argumento do indexOf garante a ordem das mensagens:
      var index = currentMessagesData.indexOf(elementData, lastIndex + 1);
      
      // A mensagem já tinha sido registrada:
      if (index !== -1) {
        // Elimina mensagens apagadas:
        if(index - lastIndex > 1) {
          currentMessages.slice(lastIndex + 1, index).each(function (n) {
            // Mais informação sobre essa corrente nas linhas abaixo
            var removedElements = $(this).parent().prev().addBack();
            
            // Remover da lista de mensagens não lidas:
            // Adicionado porque o número de mensagens estava alto
            // Removido por que ficou baixo. Motivo: desconhecido.
            // unreadMessages.find(removedElements).remove();
            
            // Remover da lista de mensagens mostradas:
            removedElements.fadeOut(1200, function(){
              $(this).remove();
            });
          });
        }
        lastIndex = index;
        
      // A mensagem não tinha sido
      // registrada então adicioná-la
      } else {
        // $this é o <td> no segundo <tr>
        // logo para pegar toda a mensagem
        // seleciona o <tr> e o anterior a ele.
        var newMessagesElements = $this.parent().prev().addBack();
        currentMessages.eq(lastIndex + 1).parent().prev().before(newMessagesElements);
        
        // Só considerar mensagens novas aquelas que forem adicionadas anteriormente as já existentes:
        // Por algum motivo está considerando menos mensagens do que deveria
        if (lastIndex + 1 <= newMessages.length) {
          newMessages = newMessages.add(newMessagesElements.prevAll().andSelf());
        }
      }
    });
    
    // Se estiver houvendo uma conversa ativa então
    // carregar mensagens em um intervalo mais rápido
    // se não aumentar o intervalo para aliviar o servidor
    // ajuste os valores se necessário
    timeBetweenReloads = Math.max(
      10, // Intervalo mínimo de 10 segundos
      60 - newMessages.length * 15 // Cada nova mensagem reduz o intervalo em 15 segundos
    ) * 1000; // Finalmente converte para milisegundos

    if (newMessages.length === 0) { return; }
    setMessageDeleteHandlers();
    
    // Se não estiver na página então deixar
    // um aviso pelo título da página
    if (document.hidden) {
      // unreadMessages amarzena os elementos das novas mensagens
      unreadMessages = unreadMessages.add(newMessages);
      // Como cada mensagem é composta de dois elementos logo se divide por dois
      top.document.title = '(' + (unreadMessages.length / 2) + ') ' + DEFAULT_TITLE;
      
      // Se for a primeira vez que isso acontece:
      if (unreadMessages.length === newMessages.length) {
        // O código para detectar quando voltou a página é simplificado,
        // a versão completa dele está nessa página:
        // https://developer.mozilla.org/en-US/docs/Web/Guide/User_experience/Using_the_Page_Visibility_API
        $(document).one('visibilitychange', function () {
          // volta o título ao padrão:
          top.document.title = DEFAULT_TITLE;
          // Seleciona a ultima mensagem e alinha o fundo do scroll com ela:
          scrollToElement(unreadMessages.last(), function () {
            unreadMessages.addClass('highlight');
            // Limpa a fila de mensagens não lidas:
            unreadMessages = $();
          });
        });
      }
    // Se estiver lendo mensagens anteriores
    } else if (scrolledLength !== 0) {
      // Manter o scroll no mesmo ponto,
      // para não interromper a leitura:
      container.scrollTop(scrolledLength + firstOldMessage.offset().top - oldPosition);
      
      // Avisar que há novas mensagens por
      // meio de um aviso no topo da página
      showMessage(newMessages.length === 2 ? 'Há uma mensagem nova' :
        ('Há ' + (newMessages.length / 2) + ' novas mensagens'), function () {
          scrollToElement(newMessages.last(), function () {
            newMessages.addClass('highlight');
          });
        });
    } else {
      newMessages.addClass('highlight');
    }
  }
  
  // Cria uma mensagem de aviso no topo da página
  // Aceita uma função no segundo argumento
  // que é chamada caso a mensagem seja clicada
  function showMessage(message, callback) {
    var el = $('<div>', {
      text: message,
      'class': 'message-alert'
    })
    .appendTo('body')
    .on('mousedown touchstart', function handler(evt) {
      // Evita ser chamado mais de uma vez
      el.off('mousedown touchstart', handler);
      
      // Chama o callback, se definido
      if (typeof callback == 'function') {
        callback();
      }
      
      // Faz a animação de fadeout
      el.stop(true).fadeOut(300, function () {
        el.remove();
      });
    })
    // Usando as animações do jQuery
    // Há CSS3, mas nesse caso tudo bem
    .hide().fadeIn(1000)
    .delay(5e3 /* 5 segundos*/ )
    .fadeOut(1000, function () {
      el.remove();
    });
  }
  
  // Coloca o fundo do scroll no elemento
  // Se não for possível irá o mais próximo
  function scrollToElement(el, callback) {
    container.animate({
      scrollTop: el.position().top - container.height() + container.scrollTop() + el.height()
    }, 600, 'swing', callback || $.noop);
  }
}());

I also considered that with only this information maybe it would not be enough for you to have an idea of what I am working on I thought about making a fiddle, but it would not be possible to make one that simulated the server. So I implemented a simulation using Service Workers (I tested on Google Chrome) and so I hope it serves for better understanding.

I am asking for help as some users are reporting the following problems: missing messages, duplicates and wrong count of new messages. I’ve run several tests on this simulation, the development server and the production server, with no results. Finally if I have not given enough information comment and if the question is not in accordance with the correct style edit.

  • 1

    I find it impossible to solve in an informal environment due to the cost of time. And, time is money. When I opened the code, I thought it would have two or three functions and at most 30 lines. Actually it has nothing very complex, but anyway it costs time.

  • 1

    @Gustavorodrigues, as much as I believe that Nodejs with Socketio or ASP.NET with Signalr are more appropriate for a chat, I believe that PHP with Ratchet should allow the creation of a Broadcast, thus simplifying your server-side... something else to consider, is your server-side receive and send only JSON, and mount the messages using something like Handlebars

  • I understand your dilemma, but I regret to inform you that no one has the right to tell you what and as you will develop it all based on opinions, if you want some direction I think the comments have been enough Nodejs, ASP.NET and Ratchet have examples ready for that your question just choose a test it adapts it or discard it.

  • I don’t know if they ever saw the simulation source code: He’s based in Handlebars, as suggested. I’m used to Node, but as I said in the question I’m not in a position to suggest changes to the server. The current responses are already very helpful anyway.

1 answer

2

An interesting challenge, but unfortunately I’m running out of time to analyze all the code in detail and propose improvements.

Beware of the competition

In general, when random problems occur, such as these duplicated or disappearing messages, the problem is related to the competition, so in simple tests this does not occur, but when several users use the system the errors appear.

One way to better determine the problem is to add some kind of log in production. When someone complains, review the log and see which messages were displayed and when. So you can easily isolate the problem.

Beware of the Network

Another possible scenario is that a bad connection causes some Ajax calls to be lost and others duplicated. This is another case where no problems are found in the tests, but the same appear in customers.

Assuming there is no silly or logical error in the code, my suggestion is to use some value that can identify each message individually rather than rely on the order and amount of messages and try guess (as it is in the code) which are the new messages.

Step away from HTML

Another possible improvement would be to decrease the amount of code that handles and directly selects HTML.

Maybe using a library like Angularjs or Reactjs is a little easier in this case, because then you can only worry about getting the data and let the library show the messages correctly.

Considerations

Anyway, I know it was abstract tips and it takes some time to implement, but I hope it was of some help.

  • I will contact the developer of the site to see if I can access the page code and create a JSON output, so that way get the values that are not exposed in HTML, so no need guess the data. At first I considered using the libraries, but personally I am avoiding them at the moment (I never liked Angularjs and Reactjs I did an entire project with it so I never want to use it again), preferring small modules and vanilla js (but as they already used jQuery so I kept it).

Browser other questions tagged

You are not signed in. Login or sign up in order to post.