4
\$\begingroup\$

At this point I've written a fair amount of code to deal with dynamic line items. I've slowly refactored most of the code into an Order object that's created at page load which contains the methods for various actions within the table.

I'm hoping to get some feedback from another set of eyes as to how to further optimize this code:

<table class="line-items editable table table-bordered">
    <thead class="panel-heading">
        <tr class="panel-heading">
            <th>ITC Part</th>
            <th>Description</th>
            <th>Vendor</th>
            <th>Vendor Part</th>
            <th>Cost</th>
            <th>Price</th>
            <th>Qty</th>
            <th>Subtotal</th>
            <th><span class="glyphicon glyphicon-list-alt"></span></th>
        </tr>
    </thead>

    <tfoot>
        <tr>
            <th><input type="text" class="form-control input-sm" id="itc_part_input" name="itc_part_input"></th>
            <th><textarea class="form-control input-sm" id="description_input" name="description_input" rows="2"></textarea></th>
            <th><input type="text" class="form-control input-sm" id="vendor_input" name="vendor_input"></th>
            <th><input type="text" class="form-control input-sm" id="vendor_part_input" name="vendor_part_input"></th>
            <th><input type="text" class="form-control input-sm" id="cost_input" name="cost_input"></th>
            <th><input type="text" class="form-control input-sm" id="price_input" name="price_input"></th>
            <th><input type="text" class="form-control input-sm" id="qty_input" name="qty_input" value="1"></th>
            <th><input type="text" class="form-control input-sm" id="subtotal_input" name="subtotal_input" disabled="disabled"></th>
            <th><span class="glyphicon glyphicon-plus add-line-item" title="Add Line Item"></span></th>
        </tr>
        <tr>
            <th colspan="9">
                <div class="row">
                    <div class="col-sm-6 order-number">Order Number: 80071</div>
                    <div class="col-sm-6 text-right order-total">Total: $0.00</div>
                </div>
            </th>
        </tr>
    </tfoot>

    <tbody>
    </tbody>
</table>

JS Code:

var Order = function() {
    this.addLineItem = function() {
        // build new table row from line item inputs
        var newRow = '<tr>'+
                        '<td>'+$('#itc_part_input').val()+'</td>'+
                        '<td>'+$('#description_input').val()+'</td>'+
                        '<td>'+$('#vendor_input').val()+'</td>'+
                        '<td>'+$('#vendor_part_input').val()+'</td>'+
                        '<td>'+$('#cost_input').val()+'</td>'+
                        '<td>'+$('#price_input').val()+'</td>'+
                        '<td>'+$('#qty_input').val()+'</td>'+
                        '<td>'+$('#subtotal_input').val()+'</td>'+
                        '<td><span class="glyphicon glyphicon-remove remove-line-item" title="Remove Line Item"></span></td>'+
                     '</tr>';

        // add the new row to the table
        $('.line-items > tbody:last').append(newRow);

        // scroll to the bottom of the page to compensate for the new table row
        $('html, body').animate({ scrollTop: $(document).height() }, 'fast');

        // clear line item inputs
        $('.line-items tfoot input, .line-items tfoot textarea').each(function() {
            $(this).val('');
        });

        // set default qty to 1 for convenience
        $('.line-items > tfoot > tr > th:nth-child(7) input').each(function() {
            $(this).val('1');
        });

        // return focus to first input field after adding line item,
        // specifically useful when tabbing out of qty_input
        $('.line-items > tfoot > tr > th:first-child input').focus();

        // recalculate total
        this.updateTotal();
    }

    this.removeLineItem = function(row) {
        // remove row from table
        row.remove();

        // to delete an existing line item we have to pass its id to the model with an action => delete flag
        new LineItem();
        $('<input>').attr({
            type: 'hidden',
            name: 'data[OrderLineItem]['+LineItem.count+'][id]',
            value: row.attr('data-line-item-id'),
        }).appendTo('form');
        $('<input>').attr({
            type: 'hidden',
            name: 'data[OrderLineItem]['+LineItem.count+'][action]',
            value: 'delete',
        }).appendTo('form');

        // recalculate total
        this.updateTotal();
    }

    // calculate order total from subtotals
    this.calculateTotal = function() {
        var total = 0;
        $('.line-items > tbody > tr > td:nth-child(8)').each(function() {
            total += parseFloat($(this).html()) || 0;
        });
        return total.toFixed(2);
    }

    // write order total to table footer
    this.updateTotal = function() {
        var amount = this.calculateTotal();
        $('.order-total').html('Total: $' + amount);
    }

    // adds hidden form fields for dynamically created line items
    this.processLineItems = function() {
        // add form fields for each line item in Model.{0..n}.field naming convention
        $('.line-items > tbody > tr').each(function() {
            // increments counter for Model.{0..n}.field naming convention
            new LineItem();

            // OrderLineItem id is needed to update records
            $('<input>').attr({
                type: 'hidden',
                name: 'data[OrderLineItem]['+LineItem.count+'][id]',
                value: $(this).attr('data-line-item-id'),
            }).appendTo('form');

            // line item fields
            var colCount = 0; var fields = ['itc_part', 'description', 'vendor', 'vendor_part', 'cost', 'price', 'qty', 'subtotal'];
            $.each(this.cells, function(){
                $('<input>').attr({
                    type: 'hidden',
                    name: 'data[OrderLineItem]['+LineItem.count+']['+fields[colCount]+']',
                    value: $(this).html()
                }).appendTo('form');
                colCount++;
            });
        });
    }

    this.save = function(afterSaveAction) {
        // disable any action buttons on the page to prevent double submit
        $('.btn').button('loading');

        // turn line items table into form fields before submit
        this.processLineItems();

        // afterSaveAction tells controller what to do after save
        // stay on page by default, controller will refresh page
        // this default action is also set inside the controller
        if (typeof(afterSaveAction) === 'undefined') afterSaveAction = 'continue';
        $('<input>').attr({
            type: 'hidden',
            name: 'data[Order][action]',
            value: afterSaveAction,
        }).appendTo('form');

        // submit the form
        $('form[id^=Order]').submit();
    }

    // calculate order total when Order is initialized
    this.updateTotal();
}
Order = new Order();

// helper to keep count of dynamic line items on page
function LineItem() {
    if (typeof LineItem.count == 'undefined') {
        LineItem.count = 0;
    } else {
        LineItem.count++;
    }
}

// force numeric input function
$.fn.ForceNumeric = function() {
    return this.each(function() {
        $(this).keydown(function(event) {
            var key = event.charCode || event.keyCode || 0;
            // allow backspace, tab, delete, arrows, numbers and keypad numbers ONLY
            // home, end, period, and numpad decimal
            return (
                key == 8 ||
                key == 9 ||
                key == 46 ||
                key == 110 ||
                key == 190 ||
                (key >= 35 && key <= 40) ||
                (key >= 48 && key <= 57) ||
                (key >= 96 && key <= 105)
            );
        });
    });
};

$(function() {
    // set datepicker defaults globally
    $.fn.datepicker.defaults.autoclose = true;
    $.fn.datepicker.defaults.todayHighlight = true;

    $('#itc_part_input').change(function(){
        this.value = this.value.toUpperCase(); 
    });

    // force numeric input
    $('#cost_input, #price_input, #qty_input').ForceNumeric();

    // format number to two decimal points on change
    $('#cost_input, #price_input').change(function () {
        $(this).val(
            parseFloat($(this).val()).toFixed(2)
        );
    });

    // calculate subtotal during input
    $('#price_input, #qty_input').change(function () {
        var subtotal = $('#price_input').val() * $('#qty_input').val();
        $('#subtotal_input').val(
            parseFloat(subtotal).toFixed(2)
        );
    });

    // tabbing out of #qty_input automatically adds line item
    $("#qty_input").on('keydown', function(e) { 
        var keyCode = e.keyCode || e.which; 

        if (keyCode == 9) { 
            e.preventDefault(); 
            Order.addLineItem();
        }
    });


    // add line item to table
    $('.add-line-item').click(function(event) {
        Order.addLineItem();
    });

    // make table cell editable when clicked
    function enableLineItemEdit() {
        $('.line-items').on('click', 'tbody td:not(:last-child)', function(event) {
            if (event.currentTarget.cellIndex == 1) {
                $(this).html('<textarea class="form-control input-sm" id="description_input" name="description_input">'+$(this).text()+'</textarea>');
                $(this).find('textarea').focus();
            } else {
                $(this).html('<input type="text" class="form-control input-sm" value="'+$(this).text()+'">');
                $(this).find('input').focus();
            }
            $(this).off(event);
        });
    }; enableLineItemEdit();

    // turn line item cell input back into text when clicked out of
    $('.line-items').on('blur', 'tbody input, tbody textarea', function(event) {
        $(this).replaceWith(this.value);
        enableLineItemEdit();
    });

    // remove line item from table
    $('.line-items').on('click', '.remove-line-item', function(event) {
        Order.removeLineItem($(this).closest('tr'));        
    });

    // save buttons
    $('.save-continue').click(function(event) {
        Order.save('continue');
    });

    $('.save-close').click(function(event) {
        Order.save('close');
    });
});

Specific areas of concern:

  1. I moved most functionality into the Order object and then use jQuery after DOM-ready to bind event handlers. How might I go about replacing most (if not all) the code in the DOM-ready function with an Order.init() or similar?

  2. I had to comment out the $.fn.datepicker.defaults lines in the bootply because the associated js file isn't available. Is there a better place to put these default settings for the datepicker?

  3. This code is functional, but one of the users is reporting what sounds like a possible memory leak (though I cannot recreate it). How much of this code can be refactored away as being redundant?

  4. The more field-specific functionality I add, the more spaghetti-ish this is becoming, so I'm looking for suggestions on how to better organize the code for maintainability.

\$\endgroup\$
  • 3
    \$\begingroup\$ Post JS code here, please. \$\endgroup\$ – sqykly Jan 22 '14 at 3:12
  • \$\begingroup\$ In absence of JS being here, I can take a really wild stab at your memory leak. Usually it happens when you give something a unique ID and store some object with that ID as its key in another object. What you really want there is a weak reference, but you don't have one, so your object can never be GCed. \$\endgroup\$ – sqykly Jan 22 '14 at 3:16
  • 1
    \$\begingroup\$ Ted, the code ends very abruptly, it is not valid JavaScript. \$\endgroup\$ – konijn Jan 22 '14 at 18:43
  • \$\begingroup\$ Fixed the cut-off JS \$\endgroup\$ – Ted Jan 22 '14 at 19:15
  • \$\begingroup\$ Do these inputs have a defaultValue property set to what you want their default to be? for example, the quantity input, is it's defaultValue set to 1? (defaultValue typically contains the original value of the value attribute.) \$\endgroup\$ – user400654 Jan 23 '14 at 15:57
1
\$\begingroup\$

You could generate your row string using .map to eliminate selecting each input one by one by id.

// build new table row from line item inputs
var newRow = "<tr>";
newRow += $(".line-items tfoot").find(":input").map(function(_,obj) {
    return "<td>" + $(obj).val() + "</td>";
}).join("");
newRow += "</tr>";

// add the new row to the table
$('.line-items > tbody:last').append(newRow);

This causes the code to be more maintainable because you no longer need to modify this section of code when a new column is added or one is removed.

While you're iterating over the inputs already, you reset the value of the inputs to their default value (assuming they have the desired default value)

// build new table row from line item inputs
var newRow = "<tr>";
newRow += $(".line-items tfoot").find(":input").map(function(i,el) {
    var $el = $(el), val = $el.val();
    // clear line item input
    $el.val(el.defaultValue);
    return "<td>" + val + "</td>";
}).join("");
newRow += "</tr>";

// add the new row to the table
$('.line-items > tbody:last').append(newRow);

Other Thoughts:

  • Why does the enableLineItemEdit function exist? Why can't you just execute that code immediately? (this is likely where your "memory leak" is coming from, you keep binding the same event to the table, never removing it)
  • When wiring up your delegated events on $('.line-items'), you should use chaining so that you aren't repeatedly calling $('.line-items')
\$\endgroup\$
  • \$\begingroup\$ Kevin, I believe I created the enableLineItemEdit function because I couldn't get .one() to work correctly. So I used .on(click), wrapped it in a function and used .off() to disable the event listener after a cell was selected for edit. Otherwise clicking in the cell while in edit state would populate the input with the table cell contents <input> and multiple cells could be in edit mode simultaneously with no way to change them back to a normal table cell. Hence the blur even listener below it reenables the edit mode click listener. I know there must be a better way, but I'm not seeing it.. \$\endgroup\$ – Ted Jan 23 '14 at 21:34
  • \$\begingroup\$ My issue with that is the fact that your usage of .off in fact does not remove the event. $(this) is the clicked element, not the element that the event is bound to (which just so happens to be the table) \$\endgroup\$ – user400654 Jan 23 '14 at 21:35

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.