JoshWeinstein JoshWeinstein - 5 months ago 9
jQuery Question

Code review? Is there a way to write this better?

I have some code.

$(this).find('.change--button__approve').click(function(){
if ($(this).hasClass('disabled')) {
return false;
} else {
$(thisChange).attr('data-type', 'approved').attr('data-changeRule', 'once');
$(this).parent().parent().next().find('p').attr('class','approved').addTemporaryClass('bounceIn', 500);
$(this).parent().parent().next().find('.changeStatus').text('approved for this document');
}
});
$(this).find('.change--button__approveAlways').click(function(){
if ($(this).hasClass('disabled')) {
return false;
} else {
$(thisChange).attr('data-type', 'approved').attr('data-changeRule', 'always');
$(this).parent().parent().next().find('p').attr('class','approved').addTemporaryClass('bounceIn', 500);
$(this).parent().parent().next().find('.changeStatus').text('approved for every document');
}
});;
$(this).find('.change--button__reject').click(function(){
if ($(this).hasClass('disabled')) {
return false;
} else {
$(thisChange).attr('data-type', 'rejected').attr('data-changeRule', 'once');
$(this).parent().parent().next().find('p').attr('class','rejected').addTemporaryClass('bounceIn', 500);
$(this).parent().parent().next().find('.changeStatus').text('rejected for this document');
}
});;
$(this).find('.change--button__rejectAlways').click(function(){
if ($(this).hasClass('disabled')) {
return false;
} else {
$(thisChange).attr('data-type', 'rejected').attr('data-changeRule', 'always');
$(this).parent().parent().next().find('p').attr('class','rejected').addTemporaryClass('bounceIn', 500);
$(this).parent().parent().next().find('.changeStatus').text('rejected for every document');
}
});;


Because of the repetitive nature of the code, I figured there must be a better way to write this. I'm still a js beginner so I'd love to know how to write this cleaner/better.

Thanks!

Answer

Here is an optimized code :

var _this = $(this);

_this.find('.change--button__approve').click(function() {
  var sibling, _this = $(this);

  if (_this.hasClass('disabled')) {
    return false;
  } else {
    $(thisChange).attr('data-type', 'approved').attr('data-changeRule', 'once');
    sibling = _this.parent().parent().next();
    sibling.find('p').attr('class','approved').addTemporaryClass('bounceIn', 500);
    sibling.find('.changeStatus').text('approved for this document');
  }
});

_this.find('.change--button__approveAlways').click(function() {
  var sibling, _this = $(this);

  if (_this.hasClass('disabled')) {
    return false;
  } else {
    $(thisChange).attr('data-type', 'approved').attr('data-changeRule', 'always');
    sibling = _this.parent().parent().next();
    sibling.find('p').attr('class','approved').addTemporaryClass('bounceIn', 500);
    sibling.find('.changeStatus').text('approved for every document');
  }
});

_this.find('.change--button__reject').click(function() {
  var sibling, _this = $(this);

  if (_this.hasClass('disabled')) {
    return false;
  } else {
    $(thisChange).attr('data-type', 'rejected').attr('data-changeRule', 'once');
    sibling = _this.parent().parent().next();
    sibling.find('p').attr('class','rejected').addTemporaryClass('bounceIn', 500);
    sibling.find('.changeStatus').text('rejected for this document');
  }
});

_this.find('.change--button__rejectAlways').click(function() {
  var sibling, _this = $(this);

  if (_this.hasClass('disabled')) {
    return false;
  } else {
    $(thisChange).attr('data-type', 'rejected').attr('data-changeRule', 'always');
    // Store into a variable to optimize execution speed
    sibling = _this.parent().parent().next();
    sibling.find('p').attr('class','rejected').addTemporaryClass('bounceIn', 500);
    sibling.find('.changeStatus').text('rejected for every document');
  }
});

As you see, the main problem is the using of $(this).parent().parent().next() on two simultaneous lines, which cause speed issue. By storing it in a local variable, this function chain is not runned two times, and the local variable is deleted after usage.

The second problem (less important) is the repetitive usage of $(this) which needs to create a jQuery instance each time you use it. By storing it in a local variable _this you can use at as often as you want without causing a instanciation that takes time.

EDIT : Another more complex but better way is this one :

var _this = $(this);

var details = {
  'approve': ['once', 'approved', 'approved for this document'],
  'approveAlways': ['always', 'approved', 'approved for every document'],
  'reject': ['once', 'rejected', 'rejected for this document'],
  'rejectAlways': ['always', 'rejected', 'rejected for every document']
}, keys = Object.keys(details);

for(var i = 0; i < keys.length; i++)
  // Create a lambda function to use local variable 'scope'
  (function(scope) {
    // Here is the content of 'scope' :
    // scope[0] : value of 'data-changeRule' ('once' or 'always')
    // scope[1] : value of 'data-type' and 'class' ('approved' or 'rejected')
    // scope[2] : message to display 'approved for this document'...
    _this.find('.change--button__' + scope[3]).click(function() {
      var sibling, _this = $(this);

      if (_this.hasClass('disabled')) {
        return false;
      } else {
        $(thisChange).attr('data-type', scope[1]).attr('data-changeRule', scope[0]);
        sibling = _this.parent().parent().next();
        sibling.find('p').attr('class', scope[1]).addTemporaryClass('bounceIn', 500);
        sibling.find('.changeStatus').text(scope[2]);
      }
    });
  }).call(this, details[keys[i]].concat([keys[i]])); // Call the lambda function with its informations

Please note that this code is not optimized as best I can but if I optimize it it will be ugly.

EDIT 2 : This is, I think, the faster and better way to do your stuff :

var _this = $(this),
    keys = ['approve', 'approveAlways', 'reject', 'rejectAlways'];

for(var i = 0; i < keys.length; i++)
  // Create a lambda function to use local variable 'scope'
  (function(name) {
    _this.find('.change--button__' + name).click(function() {
      var sibling, _this = $(this),
          approve = (name.indexOf('approve') !== -1 ? 'approved' : 'rejected'), // Is the button an approving or rejecting button ?
          always  = (name.indexOf('Always') !== -1); // Is the button setting always or only for THIS document ?

      if (_this.hasClass('disabled')) {
        return false;
      } else {
        $(thisChange).attr('data-type', approve).attr('data-changeRule', (always ? 'always' : 'once'));
        sibling = _this.parent().parent().next();
        sibling.find('p').attr('class', approve).addTemporaryClass('bounceIn', 500);
        sibling.find('.changeStatus').text(approve + ' for ' + (always ? 'every' : 'this') + ' document');
      }
    });
  }).call(this, keys[i]); // Call the lambda function with its informations

Only a few strings are stored and your function _this.find('.change--button__...').click( is writed one time only and runned by the loop multiple times. If you want to add buttons, you can simply add their name in the array keys.

I hope I helped you :)