$("#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:
Is my IDE on to something here, can I write this in a better way?
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).
var colorsAdded = document.getElementById("colors_added"); colors_added.innerHTML = parseFloat(colors_added.innerHTML) + 1; but not enough jQuery.
\$\endgroup\$
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.
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.
