1
\$\begingroup\$

I'm looking for suggestions on how to clean up JavaScript/jQuery code that looks like the example in the Before snippet.

I'm a backend developer so some of the constructs in JavaScript are foreign to me. I took a stab at it in the After block but I'm not sure if there is a name for that design pattern.

Most of the JS code we have are click handlers and DOM table manipulation. There are also situations where we need to keep state like rows visible, etc.

In order to easily toggle console logging, I added a log() function that checks the debug flag.

Before

$(function() {

  // Delete button handler
  $('.delete-button').click(function(e) {
    e.preventDefault();
    var id = $(this).closest('tr').data('id');
    var name = $(this).prop('name');
    var modal = $('#delete-modal');
    modal.find('.modal-body').html("You've chosen to delete <strong>" + name + "</strong>");
    modal.data('id', id);
    modal.modal('show');
    console.log('Showing modal');
  });

  // Confirm delete button handler
  $('#confirm-delete-button').click(function(e) {
    var modal = $('#delete-modal');
    var id = modal.data('id');

    modal.modal('hide');

    console.log('Deleting id ' + id);

    // Build form for submission
    var $form = $('<form action="" method="POST"></form>');
    $form.append('<input type="hidden" name="id" value="' + id + '">');
    $form.append('<input type="hidden" name="reqType" value="Delete">');
    $(document.body).append($form);
    $($form).submit();

  });
});

After

var App = {

  rowCount: 0,
  visibleCount: 0,
  debug: true;

  onReady: function () {
    App.initHandlers();
  },

  log: function (message) {
    if (App.debug) {
      console.log(message);
    }
  },

  initHandlers: function () {
    $('.button-delete').click(App.showDeleteModal);
    $('#button-confirm-delete').click(App.deleteRow);        
  },

  showDeleteModal: function (event) {
    event.preventDefault();
    var id = $(this).closest('tr').data('id');
    var name = $(this).prop('name');
    var modal = $('#delete-modal');
    modal.find('.modal-body').html("You've chosen to delete <strong>" + name + "</strong>");
    modal.data('id', id);
    modal.modal('show');
    App.log('Showing modal');
  },

  deleteRow: function () {
    var modal = $('#delete-modal');
    var id = modal.data('id');

    modal.modal('hide');

    App.log('Deleting id ' + id);

    // Build form for submission
    var $form = $('<form action="" method="POST"></form>');
    $form.append('<input type="hidden" name="id" value="' + id + '">');
    $form.append('<input type="hidden" name="reqType" value="Delete">');
    $(document.body).append($form);
    $($form).submit();
  },

  updateToolbar: function () {
    App.visibleCount = $('.product-row:visible').length;
    App.rowCount = $('.product-row').length;

    if (App.visibleCount === App.rowCount) {
      $('div.toolbar').html('<h2><span class="label label-default">Showing ' + App.visibleCount + ' of ' + App.rowCount + ' rows</span></h2>');
    } else {
     $('div.toolbar').html('<h2><span class="label label-primary">Showing ' + App.visibleCount + ' of ' + App.rowCount + ' rows</span></h2>');
    }
  },
};

$(document).ready(App.onReady);
\$\endgroup\$
  • 2
    \$\begingroup\$ And your code does exactly what? You need to add a description and also the title should say what the code is doing and not what your concerns about it are. \$\endgroup\$ – t3chb0t Jul 6 '17 at 16:28
  • 1
    \$\begingroup\$ @t3chb0t: I saw this on SoftwareEngineering.SE. I updated the title with a better description. \$\endgroup\$ – Greg Burghardt Jul 6 '17 at 16:36
  • \$\begingroup\$ @GregBurghardt The new title helps, but the OP should propably also add a short paragraph of description in the question body, maybe detailing a bit more how the two approaches differ conceptually. \$\endgroup\$ – Graipher Jul 7 '17 at 10:17
3
\$\begingroup\$

The main benefit to your refactoring job is to clarify what the code is doing. The original version lacks names for things, so does clicking the .button-delete element delete the item? After further reading the code, "oh, it appears to be showing a modal dialog. Now where to we actually delete the item..." - so your intent here is very good. :)


  1. Use $(...) instead of $(document).ready(...) as this is more idiomatic for jQuery.

    So $(document).ready(App.onReady); becomes $(App.onReady);

  2. No need to App.onReady, just use an anonymous function:

    $(function() {
      App.initHandlers();
    });
    

    Remember that the context of what this points to changes based on how you call a function in JavaScript. Using the code above means the this variable in initHandlers refers to the App object, instead of the window object.

  3. Don't maintain state if it is not necessary

    The rowCount and visibleCount properties are only referenced in the updateToolbar. Use local variables instead of properties, since you overwrite the values of those properties on every invocation of updateToolbar

  4. Cleaning up repeated HTML code

    The HTML code to generate the <h2> tag in the toolbar has only one minor difference, which is the HTML class name. Use a local variable for that class name and then have one hard coded HTML string for the heading tag:

    var headingClass = App.visibleCount === App.rowCount
        ? "label-default"
        : "label-primary";
    
    var headingMarkup = '<h2><span class="label ' + headingClass + '>Showing ' + App.visibleCount + ' of ' + App.rowCount + ' rows</span></h2>';
    
    $('div.toolbar').html(headingMarkup);
    
  5. The name App is pretty generic. Methods like "initHandlers" and "deleteRow" are also too generic. This class could end up expanding way beyond what it should. A more focused approach will prevent this class from becoming an unmaintainable monster later on.

    Based on the naming of things in the code, it looks like something that is deleting items from a "shopping cart" table. A more appropriate name would be ShoppingCartTableController instead of App.

    Now names like "initHandlers" and "deleteRow" have a little context, and a clear focus.

    Calling this a "controller" is appropriate here, because it is responding to events in the View, which is precisely what a "Controller" does in the MVC design pattern.

  6. In the deleteRow method, a <form> tag is created, then two hidden inputs are appended. There is a hidden cost to this. HTML is being parsed 3 times, and the document tree on the page is being modified 3 times. With a little rearranging you can parse the HTML once, and only append to the document tree once:

    // Build form for submission
    var html = '<form action="" method="POST">'
             +      '<input type="hidden" name="id" value="' + id + '">'
             +      '<input type="hidden" name="reqType" value="Delete">'
             + '</form>';
    var $form = $(html);
    
    $(document.body).append($form);
    
    $form.submit();
    

    Second of all, passing $form into the $ function is completely unnecessary, since $form is already a jQuery object. Just call $form.submit() directly.

Refactoring this into an object isn't really gaining you anything. Instead, you could opt for a named function that much more clearly makes your intent known:

function ShoppingCartTableController() {
  var showDeleteModal = function (e) {
    // ...
  },
  deleteRow = function (e) {
    // ...
  },
  log = function (message) {
    if (ShoppingCartTableController.debug) {
      console.log(message);
    }
  };

  $('.delete-button').click(showDeleteModal);
  $('#confirm-delete-button').click(deleteRow);
}

ShoppingCartTableController.debug = true;

$(ShoppingCartTableController);
\$\endgroup\$
  • \$\begingroup\$ Awesome info thanks! I appreciate the time you took to review my code. \$\endgroup\$ – A_B Jul 6 '17 at 17:55

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.