7
\$\begingroup\$
$("#colors_added").html(parseFloat($("#colors_added").html()) + 1);

This JS line makes my IDE go Duplicated jQuery selector with the following explanation:

Checks that jQuery selectors are used in an efficient way. It suggests to split descendant selectors which are prefaced with ID selector and warns about duplicated selectors which could be cached.

I've made a screenshot of it:

image of IDE

Is my IDE on to something here, can I write this in a better way?

\$\endgroup\$

3 Answers 3

13
\$\begingroup\$

You should cache your selector in a variable:

var $addedColors = $("#colors_added");
$addedColors.html(parseFloat($addedColors.html()) + 1);

Reason: placing selectors in variables is recommended, since overusing selectors can result in poor performance.

Everytime you would call a function on $("#colors_added"), the whole DOM has to be parsed by the browser. This is not the case when you store it in a variable.

Also, it's a general principle to not repeat yourself in code, also called the DRY principle (as mentioned by Peter Rader in the comments).

\$\endgroup\$
14
  • 2
    \$\begingroup\$ ... why? This will make the warning go away, I think, thus solving the "problem", but it doesn't explain why this is good to do so. It's not a "review". \$\endgroup\$
    – Pimgd
    Nov 19, 2014 at 13:10
  • \$\begingroup\$ I had no idea caching was even a problem with this kind of javascript. \$\endgroup\$ Nov 19, 2014 at 13:13
  • 1
    \$\begingroup\$ @Abbas I have upvoted it, because now I can see why something I thought to be a constant is actually a really expensive object to create. \$\endgroup\$
    – Pimgd
    Nov 19, 2014 at 13:50
  • 2
    \$\begingroup\$ Arguably this would be better: var colorsAdded = document.getElementById("colors_added"); colors_added.innerHTML = parseFloat(colors_added.innerHTML) + 1; but not enough jQuery. \$\endgroup\$
    – Daniel
    Nov 19, 2014 at 18:04
  • 1
    \$\begingroup\$ @DanielCook: Once you've already taken the performance hit of loading a library, why bother avoiding JQuery? In any case, .innerHTML parses the expression for DOM trees, so .textContent is the faster and safer method in this case. \$\endgroup\$ Nov 19, 2014 at 20:52
2
\$\begingroup\$

One way to eliminate this duplication is to use the callback form of .html():

$("#colors_added").html( function(idx, html) {
    return parseFloat(html) + 1;
} );

One advantage of using this style is that, unlike your original code, it will work correctly (i.e. apply the callback once to each element) even if the selector can match multiple elements.


Ps. Since you're not actually manipulating HTML code, but just the text content of the element, I'd really prefer to use .text() instead of .html():

$("#colors_added").text( function(idx, text) {
    return parseFloat(text) + 1;
} );

or, using Abbas' style:

var $colors = $("#colors_added");
$colors.text( parseFloat( $colors.text() ) + 1 );

In this particular case, the difference should not matter much, but it's a good idea to develop the habit of always using .text() to manipulate element content, unless you actually need to manipulate raw HTML (and are aware of the potential security issues in doing so). This habit will make you less likely to get bitten by XSS bugs down the road.

\$\endgroup\$
0
\$\begingroup\$

In this specific instance, it does not really matter. Here is my rationale:

In any case, in Sizzle (the selector library used by jQuery), a lookup for a simple #identifier selector (e.g. $("#identifier")) is probably the fastest around, since apart from error checking and browser compatibility check (apparently, some browsers, like older versions of IE, will not consider dynamically-added id attributes). it results in little more than simply translating it to a document.getElementById(selector) call. That DOM selection method is the fastest, since browsers will hash the identifiers for quick lookups, because they are required by specification to be unique inside a

Although it is good practice, in this case the performance gain is probably very negligible, when compared to a much more complex selector such as element > .class:not-first:has(element.class2) + li. And according to this answer the most recently used identifiers are cached anyway.

Bottom-line: in your situation, disregard the performance gains, they are negligible. Premature optimization is the root of all evil. Just consider the overall cleanliness of code: arguably, repeating the same selector call in a single line of code is not as clean as referencing the selector in a variable (DRY), which has the added benefit of being easily replaceable, or even parameterizable.

\$\endgroup\$
1
  • \$\begingroup\$ Lookup by ID is pretty fast, but you still have the overhead of creating an entire jQuery object. And jQuery really likes to take its time... \$\endgroup\$ Nov 19, 2014 at 17:24

Your Answer

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

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