4
\$\begingroup\$

Background

I work at a company that makes hardware products. All the test settings are stored on the server so it updates all our test stands.

I'm currently working on a web-based app using ASP.NET so we can quickly change the test settings. I've been working to make the UI easier to use by adding modals, AJAX, and some custom JavaScript to do things like change the order in which things are tested, but I don't want to move forward without dealing with, what I'm sure is, bad code structure.

Here's a short summary of what the page does:

  1. User Selects a Product Test
  2. A partial page is loaded in as a table of Limits (basically steps in the test)
  3. From here, the user can:

Upon pressing the toggle:

  1. Pagination is removed from the table by getting rid of DataTables
  2. The numeric Order field is replaced by toggle switches*
  3. From here, users may:
    • Drag and drop limits into a new order
    • Toggle a limit on or off
    • Save/cancel the new order
    • Access the right-click context menu
    • If you scroll down, a menu will pop up for the save/cancel buttons

In the context menu, you may:

  • Add a limit
  • Move a limit (for moving them long distances; only available in reordering mode)
  • Delete a limit

*about the toggle switches: By the time a test is finished being made, there will be hundreds or thousands of limits. The process of creating all the toggles a once takes several seconds, so instead I load them in as the user scrolls down using onScreen. A number <= 0 indicates that the limit is turned off.

Concerns

By far my biggest concern is the structure. This was made in a very iterative way, and while it wasn't hard to maintain in the beginning, it gets harder every time I change/add something. While I wouldn't consider myself 'new' to JavaScript, I have no proper training on it, and so I don't know many of the best practices. I plan to refactor this code to be more maintainable, but I cannot move forward with it's current structure.

I'm not looking for any UX suggestions, as I get feedback directly from the person(s) who will primarily use this page. But if you have them, you are of course free to go ahead and let me know.

Here's a couple things I know I need to change:

  • jQuery variables should start with $
  • I should keep it consistent in my choice of quotes
  • I shouldn't use anonymous functions as callbacks like in ajax's
  • onclicks and other events should be done purely in JavaScript, not set in the HTML

Code

JavaScript

"use strict";
/////////////////////GLOBAL FUNCTIONS
$(function () {
    window.last = false;

    $(window).resize(function () {//Re-positions the save/cancel popup in the reorder mode
        positionPopup();
    });

    //Create the toggle for going between the modes
    $('#reordering-toggle').bootstrapToggle({
        on: "Cancel Reorder",
        off: "Reorder Limits",
        size: "small"
    });
    $('#reordering-toggle').change(function () {
        if (this.checked != window.last) {
            if (isEditing()) {
                console.log("isOnSortTable: " + isOnSortTable());
                console.log("Checked: " + this.checked);
                console.log("Last: " + window.last);
                $('#reordering-toggle').bootstrapToggle('toggle')
                pleaseSave();
            }
            else {
                window.last = this.checked;
                reorder(this.checked);
            }
        }
    });

    $('#MoveBefore').bootstrapToggle({
        on: "Before",
        off: "After",
        onstyle: "default",
        offstyle: "default",
        width: "70px"
    });

    $('#reordering-cancel-button').hide();//I think I got rid of this
    $('#reordering-save-button').hide();

    //Add context menu to tr's
    $('tbody tr').contextmenu({
        target: '#context-menu',
        before: function (e, context) {
            window.rightClicked = $(e.target).parent();
            return true;
        }
    });

    var table = $('#display-table').DataTable({
        "aLengthMenu": [[50, 100, 250, 500, -1], [50, 100, 250, 500, "All"]],
        "iDisplayLength": 50,
        "aaSorting": [[1, "asc"]]
    });

    $('#reordering-save-button').onScreen({//Brings up the save/cancel buttons in reorder when the save button is offscreen
        doIn: function () {
            positionPopup();
            $('#save-cancel-button-popup').addClass('animating');
            $('#save-cancel-button-popup').addClass('off-screen');
            $('#save-cancel-button-popup').animate({
                top: "+=" + ($('#save-cancel-button-popup').outerHeight() + 8),
            }, 400, function () {
                $('#save-cancel-button-popup').removeClass('animating');
            });
        },
        doOut: function () {
            positionPopup();
            $('#save-cancel-button-popup').addClass('animating');
            $('#save-cancel-button-popup').removeClass('off-screen');
            $('#save-cancel-button-popup').animate({
                top: "-=" + ($('#save-cancel-button-popup').outerHeight() + 8),
            }, 400, function () {
                $('#save-cancel-button-popup').removeClass('animating');
            });
        },
        tolerance: 0
    });

    $('#save-cancel-button-popup').width(256).css('position', 'fixed').css('z-index', 1049);//z-index brings below modal shadows
    positionPopup();
});

function isOnSortTable() {//This used to have more logic, now it just doesn't make sense
    return !window.last;
}

function reload() {
    $('#save-cancel-button-popup').addClass('off-screen');
    positionPopup();
    $('#id').parent().parent().submit();//Submits an AJAX outside the partial that will reload said partial
    window.scrollTo(0, 0);
}

function saveFailed(msg) {
    notifyFailed("Save", msg);
}

function saveSuccess(msg) {
    notifySuccess("Save", msg);
}

function pleaseSave() {
    $.notify({
        title: '<b>Please Save Limits</b><br/>',
        message: 'Other limits must be saved before continuing to edit. Please try again after they have been saved.<br /><button onclick="saveNewCancel();cancel();" class="btn btn-outline btn-xs pull-right">Cancel All Edits/Adds</button>'
    },
    {
        type: 'warning',
        placement: {
            from: 'bottom'
        }
    });
}

function isEditing() {
    return (isOnSortTable() && $("tr:has('input')").length > 1) || $('#addingNow').length//length of 1 for the hidden 'add' field
}

/////////////////////CONTEXT MENU FUNCTIONS
function addAfter() {//Copies a hidden row below the one you right-clicked on
    if (isEditing()) {
        pleaseSave();
    }
    else {
        $(window.rightClicked).after($('#newLimitInline').clone().attr('id', 'addingNow'));

        if (!isOnSortTable()) {
            var checked = !$('#addingNow').next().find('div').hasClass('off');;
            $('#addingNow #Order').parent().html('<input type="checkbox" data-toggle="toggle"' + (checked ? ' checked' : '') + ' /><span hidden>' + (checked ? '1' : '0') + '</span>').find('input').bootstrapToggle({ size: 'small' });
        }
    }
}

function saveNew() {
    //Indicate Saving...
    $('#addingNow').before("<tr class='overlay' style='height:" + $('#addingNow').height() + "px'><td colspan='7' style='text-align: center;'><i class='fa fa-refresh fa-spin'></i></td></tr>");
    $('#addingNow').hide();

    var order = 0;
    if (isOnSortTable()) {
        order = $('#addingNow #Order').val();
    }
    else {
        order = ($('#addingNow').next().find('span').html() - $('#addingNow').prev().prev().find('span').html()) / 2 + parseFloat($('#addingNow').prev().prev().find('span').html());
        order = $.isNumeric(order) ? order : (parseFloat($('#addingNow').next().find('span').html()) - 1);
        order = $.isNumeric(order) ? order : (parseFloat($('#addingNow').prev().prev().find('span').html()) + 1);
    }

    var data = {
        ProductTestID: $('#id option:selected').val(),
        TestID: $('#addingNow #TestID').val(),
        Order: order,
        TestName: $('#addingNow #Name').val(),
        Unit: $('#addingNow #Unit').val(),
        LowerLimit: $('#addingNow #LowerLimit').val(),
        UpperLimit: $('#addingNow #UpperLimit').val(),
        CategoryID: $('#addingNow #hiddenDropdown option:selected').val()
    };



    //AJAX Save
    ajaxSaveNew(data)
}

function saveNewCancel() {
    $('#addingNow').remove();
}

function deleteAsk() {
    if (isEditing()) {
        pleaseSave();
    }
    else {
        $('#deleteModal #Name').html(window.rightClicked.find('.Name').html().trim());
        $('#deleteModal #TestID').html(window.rightClicked.find('.TestID').html().trim());
        $('#deleteModal').modal('show');
    }
}

function moveToWindow() {
    $('#moveModal').modal('show');
}

function moveLim(testID, before) {
    var vlu = $('.TestID').filter(function () {
        return $(this).html().trim() == $('#MoveTestID').val();
    }).parent();
    if (vlu.length) {
        $('#moveModal').modal('hide');
        $('#MoveTestID').parent().removeClass('has-error');

        if (!$('#MoveBefore').parent().hasClass('off')) {
            vlu.before($(window.rightClicked).clone());
        }
        else {
            vlu.after($(window.rightClicked).clone());
        }
        window.rightClicked.remove();
        notifySuccess("Move",
            '"' + window.rightClicked.find('.Name').html().trim() + '"' +
            " moved " +
            (!$('#MoveBefore').parent().hasClass('off') ? "before " : "after ") +
            '"' + vlu.find('.Name').html().trim() + '"' +
            "<br />Remeber to save.");
    }
    else {
        $('#MoveTestID').parent().addClass('has-error');
    }
}

/////////////////////REORDER FUNCTIONS
function getOrdering() {
    var f = [];
    $('#display-table').find('tbody').children().each(function () {
        f.push({
            ID: $(this).attr('id'),
            Active:
                $(this).find('.Order input').length ?//Checks if initialized
                $(this).find('.Order input').prop('checked') :
                $(this).find('.Order').html() > 0
        });
    });

    return f;
}

function positionPopup() {//Re-positions the save/cancel popup
    $('#save-cancel-button-popup:not(.off-screen):not(.animating)')
        .css('top', $(window).height() - $('#save-cancel-button-popup').outerHeight() - 8)
        .css('left', $(window).width() - $('#save-cancel-button-popup').outerWidth() - 8);

    $('.off-screen:not(.animating)')
        .css('top', $(window).height())
        .css('left', $(window).width() - $('#save-cancel-button-popup').outerWidth() - 8);
}

function hasChanged() {//Tests if the order of limits has changed
    var now = getOrdering();

    var eq = window.beginOrder.length == now.length && window.beginOrder.every(function (element, index) {
        return element.ID === now[index].ID && element.Active === now[index].Active;
    });

    return !eq;
}

function makeOrdering() {//Switches the page to the 'reordering' mode
    window.last = true;
    $('#reordering-toggle').bootstrapToggle('on');
    $('#reordering-cancel-button').show();
    $('#reordering-save-button').show();

    $('#display-table').DataTable().fnDestroy();//Remove pagination, etc
    $('#display-table').sortable({//Setup the ability to sort
        containerSelector: 'table',
        itemPath: '> tbody',
        itemSelector: 'tr',
        pullPlaceholder: false,
        placeholder: '<tr class="placeholder text-muted info"/>',
        onDrag: function ($item, position, _super, event) {
            var $ph = $('.placeholder')

            $ph.html($item.html());
            $ph.css({
                height: $item.outerHeight(),
                width: $item.outerWidth()
            });

            $item.hide();
        }
    });

    $('.edit-button').hide();
    var foo = $('th').filter(function () { return ($(this).html().trim() == "Order"); });
    foo.html('<span hidden>' + foo.html() + '</span');

    $('#addAfterOption').removeClass('disabled')

    //Make Enable/Disable Checkboxes
    $('.Order').onScreen({//Initalized them as they come on screen so it doesn't hang
        doIn: function () {
            if (!$(this).find('input').length) {//Not initilized
                //Change HTML
                var val = $(this).html().trim();
                var checked = val > 0;
                var span = '<span hidden>' + val + '</span>';//This is no longer needed because the page is just reloaded.
                var check = '<input type="checkbox" data-toggle="toggle"' + (checked ? ' checked' : '') + ' />'
                $(this).html(span + check);

                //Make into a toggle switch
                $(this).find('input').bootstrapToggle({ size: 'small' });
            }
        },
        tolerance: -850,
        throttle: 1
    });

    positionPopup();

    window.beginOrder = getOrdering();
}

function makeSorting() {
    //Because it's faster to reload than remove all those toggles.
    reload();
}

function revert() {
    $.notify({
        title: '<b>Changes Reverted</b><br/>',
        message: 'Ordering changes to limits have been reverted.'
    },
    {
        type: 'info',
        placement: {
            from: 'bottom'
        }
    });
    reload();
}

function cancelOrdering() {
    if (isEditing()) {
        pleaseSave();
    }
    else {
        if (hasChanged()) {
            $('#cancelModel').modal('show');
        }
        else {
            makeSorting();
        }
    }
}

function reorder(reordering) {
    saveNewCancel();

    if (reordering) {
        makeOrdering();
    }
    else {
        if (!hasChanged()) {
            makeSorting();
        }
        else {
            window.last = true;
            $('#reordering-toggle').bootstrapToggle('on');
            $('#cancelModel').modal('show');
        }
    }

    $('tbody tr').contextmenu({
        target: '#context-menu',
        before: function (e, context) {
            window.rightClicked = $(e.target).parent();
            return true;
        }
    });//Sometimes it doesn't show up, so I'm calling this again.
}

/////////////////////EDIT FUNCTIONS
function editField(f) {
    if (isEditing()) {
        pleaseSave();
    }
    else {
        showInputs(f);
    }
}

function cancel() {//Hides edit row
    $("#editing").next().show();
    $("#editing").remove();
}

function showInputs(f) {//hides current row, adds a copy of a hidden row with inputs and copies the correct values over
    f.addClass("limit-editing");

    f.before($("#newLimitInline").clone());
    f.prev().attr('id', 'editing').find("#hiddenDropdown").attr("id", "Category");
    f.hide();

    $("#editing").find("button:contains('Cancel')").attr("onclick", "cancel();")
    $("#editing").find("button:contains('Save')").attr("onclick", "ajaxEditSave();")

    $("#editing").find("#TestID").val(f.find(".TestID").html().trim()).prop("disabled", true);
    $("#editing").find("#Order").val(f.find(".Order").html().trim());
    $("#editing").find("#Category option").filter(function () { return $(this).html().trim() == f.find(".Category").html().trim(); }).prop("selected", true);
    $("#editing").find("#Name").val(f.find(".Name").html().trim());
    $("#editing").find("#Unit").val(f.find(".Unit").html().trim());
    $("#editing").find("#LowerLimit").val(f.find(".LowerLimit").html().trim());
    $("#editing").find("#UpperLimit").val(f.find(".UpperLimit").html().trim());
}






/////////////////////AJAX'S (I moved them into separate functions just because VS won't code-fold JS with Razor in it.)
function ajaxEditSave() {
    $("#editing").before("<tr class='overlay'><td colspan='7' style='text-align: center;'><i class='fa fa-refresh fa-spin'></i></td></tr>");
    $("#editing").hide();

    var data = {
        ProductTestID: $('#id option:selected').val(),
        GlobalID: $("#editing").next().attr('id'),
        TestID: $("#editing").find("#TestID").val(),
        Order: $("#editing").find("#Order").val(),
        TestName: $("#editing").find("#Name").val(),
        Unit: $("#editing").find("#Unit").val(),
        LowerLimit: $("#editing").find("#LowerLimit").val(),
        UpperLimit: $("#editing").find("#UpperLimit").val(),
        CategoryID: $("#editing").find("#Category").val()
    };

    $.handledAjaxRequest(//This is a method I started on for the refactor, as a lot of the error handling is the same throughout. Look at the AJAX's below to get the gist of it, as I'll probably just scrap most of it on my refactor.
        "@Url.Action("SaveAsync")",
        data,
        "Save",
        "Limit settings have been saved.",
        function () {
            //Change to new values
            $("#editing").next().find('.Order').html(data.Order);
            $("#editing").next().find('.Category').html($("#editing").find("#Category option:selected").html().trim());
            $("#editing").next().find('.Name').html(data.TestName);
            $("#editing").next().find('.Unit').html(data.Unit);
            $("#editing").next().find('.LowerLimit').html(data.LowerLimit);
            $("#editing").next().find('.UpperLimit').html(data.UpperLimit);

            $(".overlay").remove();
            $("#editing").next().show();
            $("#editing").remove();
            $('#display-table').DataTable().fnDraw('full-hold');
        },
        function () {
            $(".overlay").remove();
            $("#editing").show();
        }
    );
}

function ajaxSaveNew(data) {
    $.ajax({
        url: '@Url.Action("CreateAsync")',
        type: "POST",
        data: data,
        complete: function (resp, txt) {
            if (txt == "success" && resp.responseJSON.success == true) {
                saveSuccess('New Limit has been created.');
                $('.overlay').remove();
                reload();
            }
            else {
                var msg = (
                    txt == "success" ?
                    resp.responseJSON.reason.replace(/-/g, '<br />') :
                    resp.statusText);
                //Indicate the user's stupidity
                $('.overlay').remove();
                $('#addingNow').show();
                saveFailed(msg);
            }
        }
    })
}

function saveOrder() {
    if (isEditing()) {
        pleaseSave();
    }
    else {
        $.ajax({
            url: '@Url.Action("SaveOrder")',
            type: "POST",
            data: {
                order: getOrdering(),
                ProductTestID: '@Model.First().ProductTestID'
            },
            complete: function (resp, txt) {
                if (txt == "success" && resp.responseJSON.success == true) {
                    window.last = false;
                    $('#reordering-toggle').bootstrapToggle('off');
                    saveSuccess('Limit orders have been saved.')
                    reload();
                }
                else {
                    var msg = (txt == "success" ? resp.responseJSON.reason : resp.statusText);
                    saveFailed(msg);
                }
            }
        });
    }
}

function deleteConfirmed() {
    var lim = window.rightClicked.attr('id');
    $.ajax({
        url: '@Url.Action("DeleteAsync")',
        type: "POST",
        data: { ID: lim },
        complete: function (resp, txt) {
            if (txt == "success" && resp.responseJSON.success == true) {
                notifySuccess("Delete", 'The limit "' + $('#' + lim).find('.Name').html().trim() + '" (ID: ' + lim + ') has been deleted.')
                $('.overlay').remove();
                reload();
            }
            else {
                var msg = (
                    txt == "success" ?
                    resp.responseJSON.reason.replace(/-/g, '<br />') :
                    resp.statusText);
                notifyFailed("Delete", msg);
                $('.overlay').remove();
                $('#addingNow').show();
            }
        }
    });
}

Razor/HTML Partial

Note: outside this partial, I do access a dropdown with the id of 'id'. It has the list of product tests in it.

<div class="box box-primary">
    <div class="box-header  with-border">
        <h3 class="box-title">Limits for @Model.First().ProductTest.DisplayName</h3>
    </div><!-- /.box-header -->
    <div class="box-body">
        <input id="reordering-toggle" type="checkbox" data-toggle="toggle">
        <button id="reordering-save-button" class="btn btn-primary btn-sm pull-right" onclick="if (hasChanged()) { saveOrder(); } else { makeSorting(); }">Save Reorder</button>
        <br />
        <br />
        <table class="table table-condensed table-hover" id="display-table">
            <thead>
                <tr>
                    <th>
                        @Html.DisplayNameFor(model => model.TestID)
                    </th>
                    <th>
                        @Html.DisplayNameFor(model => model.Order)
                    </th>
                    <th>
                        Category
                    </th>
                    <th>
                        @Html.DisplayNameFor(model => model.TestName)
                    </th>
                    <th>
                        @Html.DisplayNameFor(model => model.Unit)
                    </th>
                    <th>
                        @Html.DisplayNameFor(model => model.LowerLimit)
                    </th>
                    <th>
                        @Html.DisplayNameFor(model => model.UpperLimit)
                    </th>
                    <th></th>
                </tr>
            </thead>
            <tbody>
                @foreach (var item in Model)
                {
                    <tr id="@item.GlobalID">
                        <td class="TestID">
                            @Html.DisplayFor(modelItem => item.TestID)
                        </td>
                        <td class="Order">
                            @Html.DisplayFor(modelItem => item.Order)
                        </td>
                        <td class="Category">
                            @Html.DisplayFor(modelItem => item.TestCategory.Name)
                        </td>
                        <td class="Name">
                            @Html.DisplayFor(modelItem => item.TestName)
                        </td>
                        <td class="Unit">
                            @Html.DisplayFor(modelItem => item.Unit)
                        </td>
                        <td class="LowerLimit">
                            @Html.DisplayFor(modelItem => item.LowerLimit)
                        </td>
                        <td class="UpperLimit">
                            @Html.DisplayFor(modelItem => item.UpperLimit)
                        </td>
                        <td style="width: 0%;" nowrap>
                            <button class="btn btn-primary btn-xs edit-button" onclick="editField($(this).parent().parent())">Edit</button>
                        </td>
                    </tr>
                }
            </tbody>
        </table>
    </div>
</div>
<div>
    <div id="saveModal" class="modal modal-info" hidden>
        <div class="modal-dialog">
            <div class="modal-content">
                <div class="modal-header">
                    <button type="button" class="close" data-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span></button>
                    <h4 class="modal-title">Save Ordering</h4>
                </div>
                <div class="modal-body">
                    <p>Would you like to save the order and switch views?</p>
                </div>
                <div class="modal-footer">
                    <button type="button" data-dismiss="modal" class="btn btn-outline pull-left">No, Continue Reordering</button>
                    <button type="button" onclick="saveOrder()" data-dismiss="modal" class="btn btn-outline">Yes</button>
                </div>
            </div>
        </div>
    </div>

    <div id="cancelModel" class="modal modal-danger" hidden>
        <div class="modal-dialog">
            <div class="modal-content">
                <div class="modal-header">
                    <button type="button" class="close" data-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span></button>
                    <h4 class="modal-title">Cancel Ordering</h4>
                </div>
                <div class="modal-body">
                    <p>Are you sure you want to cancel? New order will not be saved.</p>
                </div>
                <div class="modal-footer">
                    <button type="button" data-dismiss="modal" class="btn btn-outline pull-left">Continue Reordering</button>
                    <button type="button" onclick="revert()" data-dismiss="modal" class="btn btn-outline">Revert Changes</button>
                </div>
            </div>
        </div>
    </div>

    <div id="deleteModal" class="modal modal-danger" hidden>
        <div class="modal-dialog">
            <div class="modal-content">
                <div class="modal-header">
                    <button type="button" class="close" data-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span></button>
                    <h4 class="modal-title">Confirm Delete</h4>
                </div>
                <div class="modal-body">
                    <p>Are you sure you want to delete this limit? This action cannot be undone.</p>
                    <dl class="dl-horizontal">
                        <dt>Test ID</dt>
                        <dd id="TestID"></dd>
                        <dt>Name</dt>
                        <dd id="Name"></dd>
                    </dl>
                </div>
                <div class="modal-footer">
                    <button type="button" data-dismiss="modal" class="btn btn-outline pull-left">Cancel</button>
                    <button type="button" onclick="deleteConfirmed()" data-dismiss="modal" class="btn btn-outline">Delete</button>
                </div>
            </div>
        </div>
    </div>

    <div id="moveModal" class="modal" hidden>
        <div class="modal-dialog">
            <div class="modal-content">
                <div class="modal-header">
                    <button type="button" class="close" data-dismiss="modal" aria-label="Close"><span aria-hidden="true">&times;</span></button>
                    <h4 class="modal-title">Cancel Ordering</h4>
                </div>
                <div class="modal-body">
                    <div class="row">

                        <div class="form-group col-lg-10">
                            <label>Test ID</label>
                            <input class="form-control" id="MoveTestID" />
                        </div>
                        <div class="form-group col-lg-2" style="margin-top:25px">
                            <input class="form-control" id="MoveBefore" type="checkbox" checked />
                        </div>
                    </div>
                </div>
                <div class="modal-footer">
                    <button type="button" data-dismiss="modal" class="btn btn-default pull-left">Cancel</button>
                    <button type="button" onclick="moveLim()" class="btn btn-primary">Move</button>
                </div>
            </div>
        </div>
    </div>

    <div class="hidden">
        <table>
            <tr id="newLimitInline">
                <td><input class="form-control" style="width:72px" name="TestID" id="TestID" type="number" placeholder="Test ID" /></td>
                <td><input class="form-control" style="width:72px" name="Order" id="Order" type="number" placeholder="Order" /></td>
                <td>@Html.DropDownList("CategoryID", null, htmlAttributes: new { @class = "form-control", @id = "hiddenDropdown" })</td>
                <td><input class="form-control" style="width:192px" name="Name" id="Name" type="text" placeholder="Name" /></td>
                <td><input class="form-control" style="width:72px" name="Unit" id="Unit" type="text" placeholder="Unit" /></td>
                <td><input class="form-control" style="width:72px" name="LowerLimit" id="LowerLimit" type="number" placeholder="Lower Limit" /></td>
                <td><input class="form-control" style="width:72px" name="UpperLimit" id="UpperLimit" type="number" placeholder="Upper Limit" /></td>
                <td nowrap><button class="btn btn-primary btn-xs" onclick="saveNew()">Save</button> <button class="btn btn-default btn-xs" onclick="saveNewCancel();">Cancel</button></td>
            </tr>
        </table>
    </div>

    <div id="save-cancel-button-popup" class="alert alert-info off-screen">
        <button class="btn btn-default btn-sm" onclick="cancelOrdering()">Cancel Reorder</button>
        <button class="btn btn-primary btn-sm pull-right" onclick="if (hasChanged()) { saveOrder(); } else { makeSorting(); }">Save Reorder</button>
    </div>

    <div id="context-menu">
        <ul class="dropdown-menu">
            <li><a onclick="addAfter($(this).parent().parent().parent())">Add After</a></li>
            <li id="addAfterOption" class="disabled"><a onclick="if (!$(this).parent().hasClass('disabled')) moveToWindow();">Move To...</a></li>
            <li><a onclick="deleteAsk();">Delete...</a></li>
        </ul>
    </div>
</div>

If you'd like any further explanation, or have a way for me to improve my question, let me know.

\$\endgroup\$
1
\$\begingroup\$

I agree with all of your points you made above about what you need to do to this code. I noticed a few other things I think would help improve your code. Most of these are pretty straightforward.

First, would be to encapsulate your code in an IIFE. This will create a closure and ensure you are working in your own private scope. If you aren't sure what a closure is, you can read more about them. Since you are using jQuery, you can pass that into your closure as $ which will allow you to reference it as $ safely throughout your code.

Also, you should move the 'use strict'; declaration out of the global scope because this can cause unexpected side effects with other code (you can read all about strict mode from MDN. You should include it in your private scope. Here is how I would recommend you start your code:

(function( $, undefined ){
  'use strict';
  // your code will go here
})( jQuery );

You could also pass in the window and document objects as well for a minor performance increase, but it isn't as necessary.

The next recommendation is to cache all of your selectors. Accessing the DOM is one of jQuerys slowest methods so you want to do this as little as possible. I typically create a single function to do this. I also recommend as little code in document.ready as possible. Again, I create a single function call for that. As an example:

(function( $, undefined ){
  'use strict';
  var $reorderingToggle, $reorderingCancelBtn , $reorderingSaveBtn;

  function getSelections() {
    $reorderingToggle = $('#reordering-toggle');
    $reorderingCancelBtn = $('#reordering-cancel-button');
    $reorderingSaveBtn = $('#reordering-save-button');
  }      

  function init() {
    getSelections();
  }

  $(function() {
    init();
  });
})( jQuery );

Note: You obviously don't have to use a single function in document.ready, it's just my preferred pattern.

After the getSelections function runs, you can reference these objects by the variable name everywhere else in your code.

Next, I would use as few anonymous functions as you can. This will help you with debugging your code later on. So continuing with the same code from above I would add a defineEvents function similar to below:

(function( $, undefined ){

  'use strict';

  var $reorderingToggle, $reorderingCancelBtn , $reorderingSaveBtn;
  var reorderingToggleOptions ={
         on: "Cancel Reorder",
         off: "Reorder Limits",
         size: "small"
  };

  function init() {
    getSelections();
    defineEvents();
  }

  function defineEvents() {
    $reorderingToggle
        .bootstrapToggle( reorderingToggleOptions )
        .on('change', changeReorderToggle);
  }

  function changeReorderToggle() {
    //your code here
  }

I also created a variable to hold the options for the bootstrap toggle and added this to the top of the code. That way, you can easily find and change these properties without having to search through the code. Also, in the above code, I use jQuerys ability to chain functions together. I don't mind using the chaining as long as the code stays readable. If it starts to get too long or unmanageable, you can always start again with your variable (or if you just prefer this style):

$reorderingToggle.bootstrapToggle( reorderingToggleOptions );
$reorderingToggle.on( 'change', changeReorderToggle);

Another thing to do would be to namespace your events. That way your change code wouldn't interfere with any other change code. You namespace your events just by adding the namespace to the event name. So the above change code could be this instead:

// in this example, hwt would be your event namespace
$reorderingToggle.on( 'change.hwt', changeReorderToggle);

That way you can remove just your change event code with:

$reorderingToggle.off( 'change.hwt');

The next thing I would do is set up your own namespace for your code. Since this for testing hardware, use something hwt (like above) or something more specific like hardwareTests. You can define this object and add any functions that you would want exposed as public methods. Any functions you don't add to this object would then be private methods. For example:

(function( $, undefined ) {
  'use strict';
   // the rest of the code

  window.hardwareTests = {
    init : init
  };

  $(function(){
    hardwareTests.init();
  });

})( jQuery );

Here we added our namespace to the global window object and added the init function to it. Again, you should any functions you want the user to be able to execute. A good example might also be a destroy method that can be called to remove your code. Any function not added to the object can only be called by your code inside of the IIFE. So, in this example, the defineEvents function would not be available to be called.

One of the other things I see in the code is manipulating an element using the css method. If possible, I would recommend adding/removing CSS classes instead of this whenever possible. Obviously some of these you are doing calculations in JS that you can't do in CSS but, for everything else, use a class instead. This helps keep your separation of concerns (CSS for styling and JavaScript for interaction).

Also, I see a lot of parent() calls and even parent().parent(). This is a very brittle approach. What happens if your DOM changes? Your code breaks. If you need to reference something, add a class or an id to it and use that to do your selection. It will save you headaches later on.

The last thing I will mention is the need to DRY your code. There are numerous places where you have the same (or very similar) pieces of code. As a general rule of thumb, if you are typing in the same line of code more than once, you should consider making it a stand alone function. For example, pleaseSave and revert are basically the same function. Why not create a single function that handles both instances?

function notifyUser( title, message, type, placement ) {
  $.notify({
    title: title,
    message: message
  }, {
    type: 'type',
    placement: {
        from: placement
    }
  });  
}

If you look through your code I'm sure can find other examples. I hope that helps. Feel free to leave a comment and best of luck!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.