Confirmation to delete items using a jQuery modal dialog

Question

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);

Answer 1

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);