4
\$\begingroup\$

I've written a simple JavaScript function to increment a counter after clicking on a button.

Here's my code:


function incrementValue() {

    span = document.getElementsByClassName("quantity")[0]

    let value = span.textContent;

    span.textContent = Number(value) + 1
}

Is my solution to what I want to do too simple or novice-like? How can I improve it without using a framework like jQuery, for example?

From the point of view of a more experienced JavaScript programmer, what are some points I could improve?

\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

You say

"How can I improve it without using a framework like jQuery, "

There is no need to use a framework. Modern DOM APIs on all modern browsers have all you need to unilaterally access and manipulate the DOM.

Style

  • Be consistent with style. If you use semicolons, use them everywhere where needed, if not then use them only where absolutely needed.

  • Name variables for what they represent, not for what they are.

  • For single element queries use Document.querySelector, for multiple element queries use Document.querySelectorAll

  • At some point during our education we stopped double lining our written work, why then must you double line your code. If you want more space between lines do this via the IDE setting don't force everyone to read double spaced code.

  • Always use constants for variables that do not change. Eg value and span should be constants.

Bugs

You have several bugs that can throw a number of errors.

Undeclared variable!

Never use an undeclared variable.

Example of declared and undeclared variables.

var b = 1; // declaration 
a = 1;     // undeclared

Why? Because JS will automatically declare a variable in the global scope meaning you can not trust the content of that named variable. For example if a 3rd party script on you page used the variable span and declared it as a constant your function would break.

Example without changing your function I can be broken as follows. Running the function will throw an error at the first line.

incrementValue();
function incrementValue() {
    span = document.getElementsByClassName("quantity")[0];
    let value = span.textContent;
    span.textContent = Number(value) + 1;
}
const span = "foo";

This is such a common mistake JS has a directive so that you can catch the error early in the development phase. Always use strict mode by adding "use strict"; to the top of the code

Ungarded

As you have not provided any HTML the access to the element with class "quantity" can not be assured. If it does not exist span will be assigned undefined and then will throw when you try to get undefined.textContent

You also do not check if the text content of span has a number, it may contain text that will parse to NaN (Not a Number) You should guard against this possibility.

  • Note You can use isNaN to check if a string is not a number.

Example guarded access.

"use strict";
function incrementValue() {
    const counter = document.querySelector(".quantity");
    if (counter) { 
        const value = Number(counter.textContent);
        if (isNaN(value)) {
            counter.textContent = "1";
        } else {
            counter.textContent = value + 1;
        }
    }
}

or

"use strict";
function incrementValue() {
    const counter = document.querySelector(".quantity");
    if (counter) { 
        const value = Number(counter.textContent);
        counter.textContent = (isNaN(value) ? 0 : value) + 1;
    }
}

  • Note that these guards are not needed if the HTML is known.

Rewrite

Assuming that the HTML is correct the function can be written without guards as follows.

  • Note that all the code is inside a IIFE (Immediately Invoked Function Expression) so that is isolated and protected from 3rd party code.

    To correctly isolate this anonymous function should also be started with a semicolon to protect it from code that does not use semicolons, and ended with a semicolon to ensure it will not throw if bundled.

;(() => {
    "use strict";
    function incrementValue() {
        const counter = document.querySelector(".quantity"); 
        counter.textContent = Number(counter.textContent) + 1;
    }
})();
\$\endgroup\$
4
\$\begingroup\$

From a short review;

  • incrementValue could be called incrementQuantity since you increment quantity

  • You only use the value of value once, so I would inline it

    span.textContent = Number(span.textContent) + 1     
    
  • You are not consistent in ending statements with ;, consider using a linter

  • It is better to give an id="quantity" to the span and then use getElementById

  • You could consider an optional value parameter that is defaulted to 1

//Global const
const quantity = document.getElementById('quantity');

//Event Listener
quantity.addEventListener("click", incrementQuantity);

//Do that thing
function incrementQuantity(value) {
  quantity.textContent = Number(quantity.textContent) + 1;
}
<span id="quantity">0</span>

\$\endgroup\$
3
\$\begingroup\$

It depends how general purpose you want it to be. At the moment it's not general purpose at all, meaning that you have to rewrite it each time. How about making it take the class name as an argument? I would actually make it take an ID; this is the normal way of passing element identity around.

Then you need to think about:

  • what will happen if there is no class / id with that name
  • what will happen if there are many (in the class case - one reason not to use class)
  • what will happen if the text there is not a number.

But even more deeply, I would not store numeric values in the DOM. They should be JS variables. Then when you update the variable you redraw the DOM. This is 'data binding' and it's at the heart of a lot of frameworks - which you say you don't want to use, but it's interesting to at least look at them to see how it looks from a programmer's viewpoint.

The point is that you can do things with a JS variable; you cannot do math with HTML, at least not efficiently.

You can do something like this, knowing that your counter to be updated is at ID1:

 ct1++;
 update(ct1, "ID1");

where update is:

 update = (v,id)=>{
    let el = getElementByID(id);
    if(el) el.textContent = v;
 }
\$\endgroup\$

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.