Recording options selected from dropdown menu

Question

I am hoping someone can help me attain better performance and better ways of writing this script. Any help would be awesome!

Overall Goal: I have a dropdown that allows someone to select an option from it. It will add that record to the table below, and then the option will be removed. If they delete that row in the table, it then adds that option back to the dropdown.

$(function() {
$('#addResource').on('change', function(){
var $selectBox = $(this),
    oID   = $selectBox.val(),
    oText = $selectBox.children(':selected').text(),
    url   = '/admin/contacts/json',
    oSize = $selectBox.children('option').size()
    noResources = '<tr class="no-resources"><td colspan="5">No Resources.</td></tr>';

//if select box out of options lets hide it
if((oSize-1) == 1) {
    $selectBox.hide().parent().append('<div class="alert alert-info">No More Contacts Available</div>');
}

// lets remove the selected option
$selectBox.children(':selected').remove();  
$selectBox.children(':first').attr('selected','selected');

// lets get the contact to add to the table
$.post(url, { id: oID },function(data){
    var tableRow = "<tr class='resource_"+data.id+"'><input type='hidden' value='"+data.id+"' name='resouces[]'><td class='name'>"+data.name+"</td><td>"+data.phone+"</td><td>"+data.email+"</td><td>$"+data.rate+"</td><td><a href='#' title='Remove Resource' class='remove_"+data.id+"' data-id='"+data.id+"'><i class='icon-trash'></i></a></td></tr>",
        table = $("#resourcesTable tbody");


    table.find('tr.no-resources').remove();
    table.append(tableRow);

    cssClass = '.remove_'+data.id;
    table.on('click',cssClass, function(e){
        var $trash  = $(this),
            rowId  = $trash.attr('data-id');

        // open confirm modal window
        modalMsg('Are you sure you wish to delete '+data.name+' as a resource?','',function(result){
            // remove deleted row
            $(".resource_"+rowId).fadeOut(function(){
                var size = table.find('tr').size(),
                    name = $(this).find('.name').text();


                $selectBox.append("<option value='"+rowId+"'>"+name+"</option>"); // add option back to dropdown
                $(this).remove(); // remove the table row when deleted
                if((size-1) == 0 ){ // if no more talbe rows put back no resources message
                    table.append(noResources);
                }
            });

            // need to 
        })
        e.preventDefault();
     });
    // need to add input fields to post to save to database
}, "json" );
}); // end on change
});

Answer 1

• Your indentation is a little funky. I can understand not indenting after $(function () { (though I would do so myself), but the change handler's body really should be indented

• You've got everything in one function, though it'd probably be more readable to separate things. For instance, addRecord, removeRecord (with further specialized functions for adding/removing a row, and adding/removing an option.

• Use jQuery to build the HTML; don't type it out as a long string. Doing so is error-prone

• I don't know how modalMsg works, but I imagine that the result you receive in the callback can have at least 2 different values. What happens if the user says "No, I don't want to delete that resource"? It looks like it gets removed anyway...
  Moreover, the wording of that modal seems a bit severe considering you can just add the resource back in a moment later. The word "Delete" can scare the pants of people.

• I have to wonder why your ajax call is a POST. I'm guessing you're updating something server-side when a record is added to the table, but in that case, shouldn't there also be a POST or DELETE call when the user removes something from the table?

• You don't have any error handling, so if the ajax call should fail for any reason, the option is already gone, and the record isn't in the table either.

• It'd be easy to add a little bit of caching, just in case someone changes their mind twice (adds, removes, adds again)

• The hidden input should be in a td, rather than floating around inside a tr.

• There's no need to create unique class names for every row. The idea of class names is that they can be applied to several elements. If you want to uniquely identify something, use an ID attribute - it's what they're for. However, in this case, we don't even need that, as we can use closures instead.

• Rather than add and remove the no-resources row and the "No more contacts available" alert, it'd be easier to have these in the HTML already, and simply show/hide them as needed. The less HTML you have to build in JS, the better.

• There's the issue of sorting the records, too. Not so much in the table, but rather in the dropdown. If I were to select the 3rd option out of, say, 10, and then change my mind, I'd expect it to behave like "undo": That's the option is back in the dropdown, as the 3rd option. But tackling this is a bit beyond the scope of this review.

I'd do something like this (assuming that the 1st option - the do-nothing option - has its value attribute set to an empty string). I haven't looked at error handling because I don't know how you'd prefer to do that.

Haven't tested this, but the general idea should be apparent

$(function () {
  var dropdown = $("#addResource"),
      table = $("#resourcesTable tbody"),
      noResourcesRow = table.find(".no-resources"),
      noContactsAlert = dropdown.parent().find(".alert"),
      url = "/admin/contacts/json",
      recordsCache = {};

  function fetchRecord(id) {
    if(recordsCache[id]) {
      addTableRow(records[id]); // use cache if possible
    } else {
      $.post(url, {id: id}, function (record) {
        recordsCache[id] = record; // add to cache
        addTableRow(record);
      });
    }
  }

  function addTableRow(record) {
    var row = $("<tr></tr>").addClass("resource").data("record", record),
        removeLink = $("<a></a>", {
          "href": "#",
          "class": "remove-resource",
          "title": "Remove resource"
        }).append($("<i></i>", { "class": "icon-trash" }));

    row.append($("<td></td>").text(record.name)).append($("<input></input>", {
      type: "hidden",
      value: record.id
    }));

    // this could be done with a loop too
    row.append($("<td></td>").text(record.phone));
    row.append($("<td></td>").text(record.email));
    row.append($("<td></td>").text(record.rate));

    row.append($("<td></td>").append(removeLink));

    removeLink.on("click", function (event) {
      event.preventDefault();
      modalMsg("Are you sure you wish to remove " + record.name + " as a resource?", "", function (result) {
        // FIXME: Add proper checking of the result
        if(...) {
          removeTableRow();
        }
      });
    });

    table.append(row);
  }

  function removeTableRow(row) {
    var record = row.data("record");
    row.fadeOut(function () {
      row.remove();
      if(!table.children.length) {
        noResourcesRow.show(); // or fadeIn() maybe
      }
      addOption(record.id, record.name); // add the option back in
    }
  }

  function addOption(id, name) {
    dropdown.append($("<option></option>", { value: id }).text(name));
    noContactsAlert.hide();
    dropdown.show();
  }

  function removeOption(option) {
    option.remove();
    if(dropdown.children.length === 1) { // includes the do-nothing option
      dropdown.hide();
      noContactsAlert.show();
    }
  }

  dropdown.on("change", function (event) {
    if(!this.value) {
      return; // ignore event if the do-nothing option is selected
    }

    fetchRecord(this.value);
    removeOption(dropdown.children(":selected"));
    this.value = ""; // set the do-nothing option
  });
});