Minimal solution: Move the state variable outside of the click handler's scope
Change your JavaScript to put the number variable outside of the click handler. Otherwise, you are resetting the number variable to 1 everytime the click handler is called.
var number = 1;
function plusOne() {
var count = document.getElementById('count');
number++;
count.textContent = number.toString();
}
Example: https://jsfiddle.net/hLymhak7/6/
Move the element reference outside the click handler's scope
It is also a good idea to keep the element reference outside of the click handler's scope if the element is never destroyed.
var number = 1;
var count = document.getElementById('count');
function plusOne() {
number++;
count.textContent = number.toString();
}
DOM query lookups are cheap nowadays, but a lot of them will negatively affect your app's performance.
Example: https://jsfiddle.net/hLymhak7/8/
Make the element dependency explicit
We can even pass the count element to the click handler to make it easier to test.
JavaScript
var number = 1;
function plusOne(count) {
number++;
count.textContent = number.toString();
}
HTML
<div>
<span id="count">1</span>
</div>
<div>
<button onclick="plusOne(count)">
+
</button>
</div>
The span element is actually assigned to a global variable which is within the scope of the button element just like the plusOne click handler. This means that in all examples, we could just as easily have used count or window.count to access the span element.
Example: https://jsfiddle.net/hLymhak7/12/
Best practice: Add as event listener
It is not recommended to bind the click handler by using the onclick attribute of the button element. One of the reasons is that we are only ever allowed to add one onclick handler, which is not the case with Element#addEventListener.
HTML
<div>
<span id="count">1</span>
</div>
<div>
<button id="incrementor">
+
</button>
</div>
JavaScript
var number = 1;
var count = document.getElementById('count');
var incrementor = document.getElementById('incrementor');
incrementor.addEventListener('click', plusOne);
function plusOne() {
number++;
count.textContent = number.toString();
}
See more discussions about onclick
Example: https://jsfiddle.net/hLymhak7/13/
Combine best practice with explicit element dependency
We can add a click listener that also passes the count element explicitly to the plusOne function.
var number = 1;
var count = document.getElementById('count');
var incrementor = document.getElementById('incrementor');
incrementor.addEventListener('click', function onClick() {
plusOne(count);
});
function plusOne(count) {
number++;
count.textContent = number.toString();
}
Now we are one step closer to maintainable code that is easily tested.
Example: https://jsfiddle.net/hLymhak7/14/
Final solution that is maintainable and easily tested
We can complete our solution by making the second dependency explicit, namely the number state variable.
When we pass this variable to the plusOne function, we now have a pure function which makes it easy to test and reason about.
HTML
<div>
<span id="count">1</span>
</div>
<div>
<button id="incrementor">
+
</button>
</div>
JavaScript
var number = 1;
var count = document.getElementById('count');
var incrementor = document.getElementById('incrementor');
incrementor.addEventListener('click', function onClick() {
number = plusOne(count, number);
});
function plusOne(count, number) {
number++;
count.textContent = number.toString();
return number;
}
While this is more verbose, the dependendencies are clear and the actual business logic, i.e. the plusOne function, can be extracted to a separate file and unit tested to verify that it does what it is supposed to.
Test suite
import { plusOne } from './plus-one';
describe('plusOne', () => {
let countElement;
let initialState;
let state;
beforeEach(() => {
initialState = 1;
state = initialState;
countElement = {
textContent: initialState.toString(),
};
})
it('returns an incremented state', () => {
state = plusOne(countElement, state);
expect(state).toBe(initialState + 1);
});
it('does not mutate the state', () => {
plusOne(countElement, state);
expect(state).toBe(initialState);
})
it('reflects the state in the count element', () => {
state = plusOne(countElement, state);
expect(countElement.textContent).toEqual(state.toString());
});
});
Example: https://jsfiddle.net/hLymhak7/15/
Test suite example: https://stackblitz.com/edit/stack-overflow-javascript-jquery-repeatedly-add-1-to-variable-o?file=src%2Fplus-one.spec.ts
Anti-pattern: Keep state in DOM
A lot of web apps keep the state in the DOM. While this is easy and we have less mutable state in our code, usually we want access to the state in multiple places of our apps.
Having to extract the state from the DOM in all places where we need it is not how it is supposed to be. We are supposed to keep our business logic in JavaScript and let the DOM reflect the state, not the other way around.
It also adds to a tight coupling to the DOM, making it more difficult to maintain and test.
// Keeping state in DOM is NOT recommended, but here we go...
var count = document.getElementById('count');
function plusOne() {
var number = Number(count.textContent);
number++;
count.textContent = number.toString();
}
Example: https://jsfiddle.net/hLymhak7/9/
