Toggle based on checkbox

Question

I have a table which has tasks with several statuses, user can use filter check-boxes to show and hide rows based on these status filters.

 function toggleRows(checkBox, img) {
 var show = $("#" + checkBox).prop("checked");
 var rows = $('tr img[src="../_layouts/15/images/' + img + '.png"]').closest('tr')
 if (show) {
 rows.show();
 }
 else {
 rows.hide();
 }
}

Implementation can be,

toggleRows("taskFinishedCb", "finished");

I would put table's code too but I am not allowed to touch that and only code above needs to be improved.

• RoToRa: jQuery can show/hide an element based on a boolean variable with .toggle(). Instead of if (show) ... just use rows.toggle(show);

Answer 1

Personally, I don't like the first two lines of code, but I'm aware that you're only concerned with this function and I wouldn't know how to improve those two lines without seeing the calling function. However:

Do the checkbox and rows share a common (and close) parent?

Example markup:

<tr>
 <td>
 <img src="../layouts/15/images/finished.png" />
 </td>
 <td>
 <input type="checkbox" value="some value">
 </td>
</tr>

In this case, the img and input share the same tr. Meaning you could have this instead:

var $row = $('tr img[src="../_layouts/15/images/' + img + '.png"]').closest('tr');
var show = $('#' + checkBox, $row).prop('checked'); 

Specifically looking at this code: $('#' + checkBox, $row). Here we are looking for an element with the id specified within the context of $row. The advantage of this is that jQuery doesn't traverse the whole DOM looking for that input, it only looks within the tr. This won't make much of a difference in this case - but it's important to have this approach with performance in mind on client side. You may have also noticed that I renamed rows to $row? A lot of people will prefix a variable with a $ if the variable is a jQuery selector, this gives other developers a bit more visibility.

Ternary operator

Instead of your if/else here you could use a ternary operator to reduce the lines of code:

show ? rows.show() : rows.hide();

Naming variables

The function variable names are a bit ambiguous in my opinion. To me checkBox suggests I'm actually receiving a checkbox not the ID property. The same with img applies here. More suitable ones might be:

function toggleRows(checkboxId, imgName) 

Answer 2

Selecting the rows by the URL of the images is a terrible idea. Not only is it slow, it links two things that should be unrelated. Considering these are statuses and considering each table row directly represents a task item, you should be using classes on the table rows instead:

<tr class="finished"> ... </tr>
function toggleRows(checkBox, status) {
 var show = $("#" + checkBox).prop("checked");
 var rows = $('tr.' + status);
 rows.toggle(show);
}

---

Another thing: I assume there are multiple checkboxes? Then this won't work, because you'll have several checkbox checked at once, you should be filtering by all of those. How/when exactly do you trigger toggleRows()?

Answer 3

toggleRows should not take a checkbox id but a show variable to show/hide the row by doing so you can call this method from any where.

function toggleRows(show, img) {
 var rows = $('tr img[src="../_layouts/15/images/' + img + '.png"]').closest('tr')
 if (show) {
 rows.show();
 }
 else {
 rows.hide();
 }
}
function ShowOrHide() {
 var checkbox = $("#" + checkBox);
 var show = checkbox.prop("checked");
 toggleRows(show, "finished");
}

Might be a nitpicking on your code but that is what I have found

• RoToRa: What is the variable checkBox in the first line of ShowOrHide()?
• Paritosh: checkbox variable is the id of the checkbox .