Partial page refresh Ajax call on button click

Question

function RequestValidation() {
 $(".RequestTrigger").on("click", function () {
 var selectedButton = $(this);
 var arr = [];
 if ($(this).data('multiple') == true) {
 var ids = $('input:checkbox:checked.check-box').map(function () {
 var value = $(this).data('entityid');
 if (!isNaN(value)) {
 arr.push(value);
 }
 });
 } else {
 var value = selectedButton.data('entityid');
 if (!isNaN(value)) {
 arr.push(value);
 }
 }
 var data = {
 ids: arr
 };
 $.ajax({
 url: selectedButton.data('url'),
 data: data,
 type: 'post',
 success: function (result) {
 $('#largeModal').modal('hide');
 $('#ApprovalTable').html(result);
 checkBox();
 accountRequestValidation();
 }
 });
 });
};

This functions binds to buttons with .RequestTrigger If the button has a data-attribute called multiple with a value of true the functions gets the entity-id of every checkbox and passes the values to the ajax request or if the value of multiple is false then it just gets the entity-id of the button clicked

• Simon Forsberg: Why did you tag your question with C# ? Does the code work as you expect it to?
• Luke Osborne: @SimonAndréForsberg The Ajax request calls a C# function. The code works but I was wondering where I could improve it as I'm not great at jQuery.

Answer 1

• By calling $(".RequestTrigger").on("click", ...); you are creating a jQuery collection of all elements matching that selector, then attaching click handlers directly to those elements. Instead, you can use event delegation:

  $(document).on("click", ".RequestTrigger", function () {
   // ...
  });
  

  If you dynamically add new .RequestTrigger elements to the page, you don't have to rerun this function to attach those event handlers, since the click event was attached to the document object. Additionally, you only have one event handler for the whole page. The more .RequestTrigger elements exist, the slower the page load will be since jQuery has to look at the whole document for those elements, loop over them and attach click handlers to them. You also don't have to worry about memory leaks due to removing elements from the DOM without detaching event handlers.

• Your are declaring a named function using the function FunctionName() { ... } syntax, which does not require a semi colon at the end:

  function RequestValidation() {
   // ...
  }
  

• The value variable is being declared twice, once in each branch of your if/else statement. Variable declarations should all go at the top of the function so other programmers can see at a glance what data is being used:

  function RequestValidation() {
   $(document).on("click", ".RequestTrigger", function () {
   var selectedButton = $(this),
   arr = [],
   value = null;
  

• The ids variable doesn't appear to be used at all. Furthermore, you are calling jQuery.map, which takes an array and maps it to an array of different objects. You really only need to call each. It also looks like an unnecessary pseudo class is being used in your selector:

  $('input[type=checkbox]:checked.check-box').each(function () {
  

• The jQuery.data function is really just a pass-through to HTMLElement.getAttribute. Under the hood, the jQuery.data function is going over a loop. Just use the native DOM method:

  function RequestValidation() {
   $(document).on("click", ".RequestTrigger", function () {
   var selectedButton = this,
   arr = [],
   value = null;
   if (this.getAttribute('data-multiple') == true) {
   $('input[type=checkbox]:checked.check-box').each(function () {
   value = this.getAttribute('data-entityid');
   // ...
   });
   } else {
   value = selectedButton.getAttribute("data-entityid");
   // ...
   }
   var data = {
   ids: arr
   };
   $.ajax({
   url: this.getAttribute('data-url'),
  

  The same holds try for the selectedButton variable. You gain nothing by using a jQuery object over the native DOM methods.

• You could parameterize the selector used to bind the event handler to make this function more flexible, while still providing an intelligent default:

  function RequestValidation(selector) {
   $(document).on("click", selector || ".RequestTrigger", function () {
  

The things I've pointed out are either nitpicks or stylistic issues. No major issues stand out.

• Gary Storey: $(document).on(".RequestTrigger", "click", function () {}); is not the correct format. It should be $(document).on("click",".RequestTrigger", function () {});.
• Greg Burghardt: @GaryStorey: Thanks for pointing that out. I updated my answer.

Answer 2

I would suggest to:

• I would recommend to use camelCase naming style, so instead of ApprovalTable you could use approvalTable

• you are using a function to define a handler for an event, the name is not very consistent with what this function does, you could rename it to enableRequestValidationHandler

• It's often a good idea to cache selectors, if you plan to use them more than once, so instead of using directly $('#largeModal') you could use somewhere:

  var largeModal = $('#largeModal');

then use it in the ajax response handler via variable success: function (result) {...

The reason to do that, is because jQuery dive in html everytime you ask him to find something in html

• you already defined var selectedButton = $(this); but still use $(this) in your code, you could use instead the selectedButton variable. I usually call it: self

These lessons will help you to understand fast and easy how jquery works

Answer 3

I was wondering where I could improve it as I'm not great at jQuery.

IMO there is not much matter to specially criticize jQuery aspect of your code. In the other hand here are some points I would suggest a slightly different way to go.

1. Structurally speaking I don't see why you're currently wrapping
   $(".RequestTrigger").on("click",function() {...}
   inside of the RequestValidation() function: anyway you're forced to write RequestValidation(); somewhere else in the main part of your code.
   So a simpler way could be to merely write
   $(".RequestTrigger").on("click",RequestValidation)
   in the main part, and have your RequestValidation() function directly beginning with the actual true contents at var selectedButton = $(this);.
   Also note that, in its current state, your code doesn't need the ; at the very end of your function.

2. You first define var selectedButton, probably to get more explicit in the following code, and I agree it can be useful. But then you're using $(this) again 2 times, so this can be normalized.

3. The if (selectedButton.data('multiple') == true) could be expressed in this shorter way: if (!!selectedButton.data('multiple'))

4. In both branches of the if (selectedButton.data('multiple') == true) condition you're using this block:
   var value = selectedButton.data('entityid'); if (!isNaN(value)) { arr.push(value); }
   You could rather make a function for it.

5. Before invoking $.ajax() you define var data to contain what will be then passed as data value of its arg object: this is useless, and IMO should be avoided since both:

   • it consumes one more variable, and more code
   • and it stresses readability about data word, already (and mandatory) used for data attribute elsewhere
     So I suggest directly write inside of $.ajax() like this:
     data: {ids: arr},

Answer 4

I took everybody's advice on board and this is the solution I came up with.

function RequestValidation() {
var self = $(this),
value = null,
arr = [];
if (self.data('multiple') == true) {
 $('input:checkbox:checked.check-box').each(function () {
 var value = self.data('identifierid');
 arr.push(checkForValue(value));
 });
} else {
 value = self.data('identifierid');
 arr.push(checkForValue(value));
}
$.ajax({
 url: self.data('url'),
 data: { ids: arr },
 type: 'post',
 success: function (result) {
 $('#largeModal').modal('hide');
 if (self.data('table') == "accountgroupadmin") {
 $('#GroupAdminTable').html(result);
 } else if (self.data('table') == "accountrequestadmin") {
 $('#ApprovalTable').html(result);
 }
 checkBox();
 $(".RequestTrigger").on("click", RequestValidation);
 }
});
};
function checkForValue(value) {
return (!isNaN(value) ? value : null);
}

• Rodislav Moldovan: please consider the fact that in js: [var myVar] --> is local, and [myOtherVar] is global, in your example value and arr are in the global space, I think you want them to be only inside your function, right ?