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.

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