1
\$\begingroup\$

This is my code:

function search() {
    $("#myInput").keyup(function () {
        var value = this.value;

        $("table").find("tr").each(function (index) {
            if (index === 0) return;
            var id = $(this).find("td").first().text();
            $(this).toggle(id.indexOf(value) !== -1);
        });
    });
}

$('body').on('click', '#searchFonts', function () {
    var value = "Font"
    $("table").find("tr").each(function (index) {
        if (index === 0) return;
        var id = $(this).find("td").first().text();
        $(this).toggle(id.indexOf(value) !== -1);
    });
});

$('body').on('click', '#searchScripts ', function () {
    var value = "script"
    $("table").find("tr").each(function (index) {
        if (index === 0) return;
        var id = $(this).find("td").first().text();
        $(this).toggle(id.indexOf(value) !== -1);
    });
});

I'm looking for ways to make this run more effectively, as I'm sure there is a more efficient way to search for the specific value's seen below such as 'Font' and 'Script'.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

D.R.Y.

There is a widely accepted principle amongst developers: Don't Repeat Yourself. The following block appears three times (with varying spacing):

$("table").find("tr").each(function(index) {
    if (index === 0) return;
    var id = $(this).find("td").first().text();
    $(this).toggle(id.indexOf(value) !== -1);
});

That can be abstracted into a function (which could accept a parameter for the value) which could then be called in place of that repeated block.

For example:

function toggleRowsWithKeyword(value) { 
    $("table").find("tr").each(function(index) {
        if (index === 0) return;
        var id = $(this).find("td").first().text();
        $(this).toggle(id.indexOf(value) !== -1);
    });
}

And usage:

function search() {
  $("#myInput").keyup(function() {
      toggleRowsWithKeyword(this.value);        
  });
}
$('body').on('click','#searchFonts',function(){
    toggleRowsWithKeyword("Font");
});
$('body').on('click','#searchScripts ',function(){
    toggleRowsWithKeyword("script");
});

Cache DOM lookups

Even with abstracting that code into a function, it would still be performing a query on the DOM.

$("table").find("tr")

That can be stored in a variable (or constant if ES-6's const is used):

var rows = $("table").find("tr");

To be on the safe side, that assignment can be placed inside a DOM-loaded callback (e.g. a function wrapped to the jQuery callback wrapper: $()):

$(function() {  //DOM is ready
    var rows = $("table").find("tr");
    //Define  toggleRowsWithKeyword(), add click handlers, etc.
}); 

Then the function can utilize rows instead of querying for the rows:

rows.each(function(index) {...

CSS Selector for all except first row

The CSS Selector:

var rows = $("table").find("tr")

Could be simplified to:

var rows = $("tr")

And the check for the first row (i.e. if (index === 0) return;) could be eliminated using the :not pseudo-class along with :first-child:

var rows =  $("tr:not(:first-child)")

More tips in article

For more tips about improving Javascript that interacts with the DOM, see this post: Stop Writing Slow Javascript. I know it bashes jQuery initially but it has some very helpful tips (and nice quotes).

\$\endgroup\$
2
  • \$\begingroup\$ Careful with initalizing rows on "document ready". This leads to rows being undefined, if the user clicks on one of the links before "document ready" triggers (which theoretically can happen). To avoid that add the event handler in the "document ready" handler, too. \$\endgroup\$
    – RoToRa
    Dec 19, 2017 at 10:41
  • \$\begingroup\$ Thanks- I have updated the advice to hopefully convey that style \$\endgroup\$ Dec 19, 2017 at 13:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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