3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ jQuery can show/hide an element based on a boolean variable with .toggle(). Instead of if (show) ... just use rows.toggle(show); \$\endgroup\$ – RoToRa Nov 9 '15 at 14:44
1
\$\begingroup\$

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) 
\$\endgroup\$
0
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ What is the variable checkBox in the first line of ShowOrHide()? \$\endgroup\$ – RoToRa Nov 9 '15 at 14:48
  • \$\begingroup\$ checkbox variable is the id of the checkbox . \$\endgroup\$ – paritosh Nov 10 '15 at 3:32
0
\$\begingroup\$

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()?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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