Simple inbox functionality using JavaScript and Ajax

Question

I've been developing a simple CMS both for practical use and learning about Laravel, and I decided to implement a simple service that makes it possible for the administrators to send messages to one another. However, the finished JavaScript code, while completely functional, ended up being a bit sketchy so I'm looking for ways to improve its performance and security. Any feedback is greatly appreciated.

This is the JavaScript code that sends Ajax calls to server (based on search term, whether we want the sent messages or the incoming messages) and formats and reloads the table shown in my CMS:

var page = 1;
var perPage = 10;
var lastPage = 1;
var searchTerm = '';
var category = 'inbox';

function nextPage () {
    // Incrementing the global 'page' variable
    page++;
    reloadList();
}

function prevPage () {
   // Decrementing the global 'page' variable
    page--;
    reloadList();
}

function search () {
    // Resetting the global 'page' variable
    page = 1;
    /* reloadList() automatically detects the search terms and sends the appropriate AJAX request
based on the value of the 'searchTerm' variable */
    searchTerm = $("#searchBox").val();
    reloadList();
}

function changeCat (cat) {

    // Resetting the global 'page' variable
    page = 1;
    // Resetting the global 'searchTerm' variable
    $("#searchBox").val('');
    searchTerm = '';
    // Setting the global 'category' variable
    category = cat;

    // Hackish way of setting the correct header for the page
    $("#tab-header").html($("#"+cat+"-pill-header").html());

    // Deactivating the old pill menu item
    $("#category-pills>li.active").removeClass("active");
    // Activating the selected pill menu item
    $("#"+cat+"-pill").addClass('active');
    reloadList();
}

function toggleStar (id) {
    // Calling the server to toggle the 'starred' field for 'id'
    var data = 'id=' + id + '&_token=' + pageToken;

    $.ajax({
        type: "POST",
        url: '/admin/messages/toggle-star',
        data: data,
        success: function(data) {
            reloadList();
        },
        error: function(data) {
            showAlert('danger', 'Error', 'Something went wrong. <a href="{{ Request::url() }}">Try again</a>', 'alerts');
        }
    });
}

function deleteChecked () {

    // Getting all the checked checkboxes
    var checkedBoxes = $("#messages>tr>td>input:checkbox:checked");

    // Determining the 'url' based on if this is a soft delete or a permanent delete
    var url = '/admin/messages/trash-checked';
    if (category == 'trash') url = '/admin/messages/delete-checked';

    // Filling the 'deleteIds' array with the soon to be deleted message ids
    var deleteIds = [];
    checkedBoxes.each(function () {
        deleteIds.push($(this).attr('id'));
    });

    // Calling the server to delete the messages with ids inside our 'deleteIds' array
    var data = 'deleteIds=' + JSON.stringify(deleteIds) + '&_token=' + pageToken;

    $.ajax({
        type: "POST",
        url: url,
        data: data,
        success: function(data) {

            // Checking to see if the messages were deleted
            if (data == 'true') {

                // Hackish way of getting the count of unread messages
                var navCount = +($(".navbar-messages-count").html());

                checkedBoxes.each(function () {
                    // Getting the id of the deleted message
                    var id = $(this).attr('id');
                    // Determining if it was on display in the messages section in navbar
                    if ($("#navbar-message-" + id).length) {
                        // Hiding the message
                        /* We are hiding, and not removing, the element to ease the task of redisplaying in case of message restore */
                        $("#navbar-message-" + id).hide();
                        // Decrementing the count of unread messages
                        navCount--;
                    }
                });

                // Updating the count of unread messages shown on numerous places in the panel
                $(".navbar-messages-count").html(navCount);
            }

            reloadList();

        },
        error: function(data) {
            showAlert('danger', 'Error', 'Something went wrong. <a href="{{ Request::url() }}">Try again</a>', 'alerts');
        }
    });
}

function restoreChecked () {

    // Getting all the checked checkboxes
    var checkedBoxes = $("#messages>tr>td>input:checkbox:checked");
    // Filling the 'restoreIds' array with the soon to be restored message ids
    var restoreIds = [];
        checkedBoxes.each(function () {
        restoreIds.push($(this).attr('id'));
    });

    // Calling the server to restore the messages with ids inside our 'restoreIds' array
    var data = 'restoreIds=' + JSON.stringify(restoreIds) + '&_token=' + pageToken;

    $.ajax({
        type: "POST",
        url: '/admin/messages/restore-checked',
        data: data,
        success: function(data) {

            // Checking to see if the messages were restored
            if (data == 'true') {

                // Hackish way of getting the count of unread messages
                var navCount = +($(".navbar-messages-count").html());
                checkedBoxes.each(function () {
                    // Getting the id of the restored message
                    var id = $(this).attr('id');
                    // Determining if it was on display in the messages section in navbar before getting deleted
                    if ($("#navbar-message-" + id).length) {
                        // Redisplaying the message
                        $("#navbar-message-" + id).show();
                        // Incrementing the count of unread messages
                        navCount++;
                    }
                });

                // Updating the count of unread messages shown on numerous places in the panel
                $(".navbar-messages-count").html(navCount);
            }

            reloadList();

        },
        error: function(data) {
            showAlert('danger', 'Error', 'Something went wrong. <a href="{{ Request::url() }}">Try again</a>', 'alerts');
        }
    });
}

function reloadList () {
    // Emptying out all the table rows
    $("#messages").html('');

    // Checking to see if we're on the trash pill item in order to add or remove the restore button accordingly
    if (category == 'trash') {
        if (!$("#restore-checked").length) {
            var restoreButtonHtml = '<button type="button" class="btn btn-default btn-sm" id="restore-checked" onclick="restoreChecked()"><i class="fa fa-rotate-left"></i></button>';
            $("#mailbox-controls").append(restoreButtonHtml);
        }
    } else {
        if ($("#restore-checked").length) $("#restore-checked").remove();
    }

    var i = 0;

    // Getting information to build rows for our table
    getPageRows(page, perPage, '/admin/messages/show-'+category, searchTerm, pageToken, function (data) {
        
        lastPage = data['last_page'];
        rowData = data['data'];

        // If messages were available
        if (rowData.length == 0) {
            var tableRow = '<tr><td style="text-align:center;">No data to show</td></tr>';
            $("#messages").append(tableRow);
        } 

        // Looping through available messages
        for (i = 0; i < rowData.length; i++) {
            // Making a table row for each message
            var tableRow = makeMessageRow(rowData[i]['id'], rowData[i]['starred'], rowData[i]['toOrFrom'], rowData[i]['read'], rowData[i]['subject'], rowData[i]['sent']);
            $("#messages").append(tableRow);
        }

        // Disabling the previous page button if we're at page one
        if (page == 1) {
            $("#prev_page_button").prop('disabled', true);
        } else {
            $("#prev_page_button").prop('disabled', false);
        }

        // Disabling the next page button if we're at the last page
        if (page == lastPage) {
            $("#next_page_button").prop('disabled', true);
        } else {
            $("#next_page_button").prop('disabled', false);
        }
    });
}

// Gets paginated, searched and categorized messages from server in order to show in the inbox section in the panel
function getPageRows (page, perPage, url, search, token, handleData) {

    // Calling the server to get 'perPage' of messages
    var data = 'page=' + page + '&perPage=' + perPage + '&search=' + search + '&_token=' + token;

    $.ajax({
        type: "POST",
        url: url,
        data: data,
        success: function(data) {
            handleData(data);
        },
        error: function(data) {
            showAlert('danger', 'Error', 'Something went wrong. <a href="{{ Request::url() }}">Try again</a>', 'alerts');
        }
    });
}

// Makes a table row based on given information for the inbox section in the panel
function makeMessageRow (id, starred, sender_name, read, subject, sent) {

    var tableRow = '<tr><td><input type="checkbox" id="'+id+'"></td><td class="mailbox-star">';
    if (starred) {
        tableRow += '<a href="javascript:void(0)" onclick="toggleStar('+id+')"><i class="fa fa-star text-yellow"></i></a>';
    } else {
        tableRow += '<a href="javascript:void(0)" onclick="toggleStar('+id+')"><i class="fa fa-star-o text-yellow"></i></a>';
    }
    tableRow += '</td><td class="mailbox-name"><a href="#">'+sender_name+'</a>';
    if (!read) {
        tableRow += '<span class="label label-primary message-label">unread</span>';
    }
    tableRow += '</td><td class="mailbox-subject">'+subject+'</td><td class="mailbox-date">'+sent+'</td>';

    return tableRow;

}

Answer 1

Style guide

I'd recommend picking a style guide to keep all of your whitespace consistent. I've formatted yours according to semistandard. Having a style guide could be considered a form of bike-shedding, but it lets you have your IDE focus on making your code readable while you can just focus on making it work.

Try to limit the use of jQuery

You're using jQuery quite a lot in your code, namely to access elements. If you can afford to (and, really, you should be able to), use standard JavaScript to access DOM elements. One of the main benefits of using the standard methods is that the browser will cache repeated accesses to document.getElementById() cheaply: jQuery won't do this (at least, not the last time I checked).

Comments

You have some comments that are describing what you're doing, like: // Incrementing the global 'page' variable. These bring absolutely no value to your code; it's pretty obvious when you see the -- operator that you're doing that. Stick to having your comments describe why they are doing what they are doing, rather than what they are doing.

Conversely, as your script is quite long you should try and add some descriptions to each function (if it is not already obvious what they do) so that other people (including yourself) understand what your code is doing in 6 months when you have to come to maintain it. We have a standard format for that called jsdoc. There are other versions of this standard, but they all use the same sort of format.

Double equals

Use triple equals instead. Here is why. You almost never want to use the double equals and many linters will yell at you for using it.

Limit use of variables to their appropriate scope

One of your variables (var i = 0) is initialised outside of a loop, but only used inside that loop. It's best practice to try and limit the amount of scope a variable touches (especially a mutable one) as it makes it easier to reason about where that variable is used.

javascript:void(0)/onclick

No! This breaks semantic HTML. If you have <a href='javascript:void(0)>, what you actually want is a <button> instead. A link and a button have two very different semantics.

Do not use inline JavaScript like onclick: use event handlers anyway. Any good content security policy (which helps protect against XSS) will disable inline javascript and thus those attributes will not work.

Generating HTML elements

I'd strongly recommend generating HTML elements based on template tags in your HTML body and using Document.cloneNode instead of pulling together JavaScript strings in your code to help with separation of concerns. This would also work very nicely with something like lodash's template elements.

---

I did originally start refactoring your code to take into account all of these things, but unfortunately it just started taking far too much time. There's a lot of room for improvement here, but I hope what I've given you helps.

I'd strongly suggest trying to reduce the amount of usage of jQuery and refactoring your templates out of your JavaScript, though. This code is very long and is quite hard to follow and it will easily become that sort of code that in 6 months you wish you could just scrap it and start over. This is primarily because your view logic is very intertwined with your business logic.

Answer 2

From a short review:

• I hope your id's are not simply an increasing number, otherwise this:

     var data = 'deleteIds=' + JSON.stringify(deleteIds) + '&_token=' + pageToken;
  

  is just asking a hacker to go to console and delete every single item. To prevent that you should use GUIDs.

• As a very high level comment, check out MVC. You can tell you need something like this because

  • You keep mixing data functionality and screen functionality in the same function.
  • Your global variables are roaming free and wild ;)
  • You are using the UI (var navCount = +($(".navbar-messages-count").html());) to store state information, never a great idea

• A Ternay could have been nice and make the root URL more obvious:

  //Are we permanently deleting trashed messages or not ?
  var url = '/admin/messages/';
  url = url + category == 'trash' ? 'delete-checked' : 'trash-checked';
  

  instead of

  // Determining the 'url' based on if this is a soft delete or a permanent delete
  var url = '/admin/messages/trash-checked';
  if (category == 'trash') url = '/admin/messages/delete-checked';
  

• There is a lot of copy pasted code between delete and restore, you should refactor that

• reloadList seems to be in the business of showing or hiding buttons, I would either call it reloadPage and let it do that, or extract non-list-loading functionality.

• All your error handling seems to be exactly the same, you could write a generic error handler function and your code would be sleeker

• You are generally correctly calling GET and POST except possibly in getPageRows where you use POST to get information

• As per Dan Pantry you should consider a templating library or even your own templating function, they are fun to write

• Contrary to Dan Pantry I think your usage of jQuery is fine, though I do believe that writing this app from an MVC perspective might reduce your usage of jQUery.