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) {
    } 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>');

  • 2
    \$\begingroup\$ Your indentation is damn confusing \$\endgroup\$
    – megawac
    Commented Mar 5, 2014 at 12:13

1 Answer 1



  • $("#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) {
    else {
        $dropdown.html('<option value="0">All Classes</option>');

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.