Script for managing server IDs

Question

I have a jQuery script and I should explain it line by line. I already do that and I want to make sure that is correct. If someone has any remarks, I will be very appreciative.

//Here we use the jQuery selector ($) to select the servers_id which is located into  
//the delivers_id and we attaches a function to run when a change event occurs
$("#delivers #servers").change(function () {

    //Here we look if the servers_id value was changed and the value is different of 0

    if ($(this).val() != '0') {

        //Here we create a new variable sid and we stored the servers_id value in it
        var sid = $("#delivers #servers").val();

        //Here we use the Ajax $.get to get the sid value and send it by Ajax request then 
        //we set the data into the o_vmats_id html and empty the vmtas_id
        $.get("/deliverability/get_vmtas/" + sid,
            function (data) {
                $('#o_vmtas').html(data);
                $('#vmtas').html('');
            });
    } else {
        //Here the else statement, we select the vmtas_id and set the html content like in the code (value=0)
        //and empty the o_vmtas_id html content
        $('#vmtas').html('<option value="0">All Classes</option>');

        $('#o_vmtas').html('');
    }
});

Answer 1

Curious,

• $("#delivers #servers") does not do what you think it does, $("#delivers, #servers") might, UPDATE: unless you mean to look for #servers under #delivers in which case you should strive to only have one tag with the #servers id and then go for $("#servers")
• I don't understand why you at one point use $(this).val() and at another point $("#delivers #servers").val() if you say that they both point to servers_id
• Furthermore, you could simply use var sid = this.value
• You are performing many jQuery calls, those could be cached.
• Your comments are too much, you don't have to explain every line ;)
• On the other hand, what I would really want to know ( the difference between vmtas and o_vmtas is not commented )
• vmtas and o_vmtas are terrible names
• Your indenting is confusing
• Consider $.empty() over $.html('')

I took some guesses as to what you are trying to and incorporated my feedback into this:

var $dropdown = $('#vmtas')
var $deliverability = $('#o_vmtas')

//Listen to changes in server id
$("#delivers").change(function () {

    //What server id are we dealing with ?
    var sid = this.value;
    //Is it something we can get deliverability info on?
    if ( sid != '0') {
        $.get("/deliverability/get_vmtas/" + sid, function (data) {
            $deliverability.html(data);
            $dropdown.html('');
        });
    }
    else {
        $dropdown.html('<option value="0">All Classes</option>');
        $deliverability.html('');
    }
});