Simple Counter Program

Question

It is my first program in Javascript. Is there any lack or a thing to be improved? It simply increases, decreases, or reset the counter either by buttons or arrow keys.

HTML,

<h1 id="number"></h1>
<button class="btn" id="incr">Increase</button>
<button class="btn" id="reset">Reset</button>
<button class="btn" id="decr">Decrease</button>

JS,

let number = 0
const getNumber = document.getElementById(`number`)
const incrButton = document.getElementById(`incr`)
const decrButton = document.getElementById(`decr`)
const resetButton = document.getElementById(`reset`)
getNumber.textContent = number
const incrFunc = function () {
 ++number
 getNumber.textContent = number
 if (number > 0) {
 getNumber.style.color = `green`
 }
}
const resetFunc = function () {
 number = 0
 getNumber.textContent = number
 getNumber.style.color = `gray`
}
const decrFunc = function () {
 --number
 getNumber.textContent = number
 if (number < 0) {
 getNumber.style.color = `red`
 }
}
incrButton.addEventListener(`keyup`, function (e) {
 e.stopPropagation()
 //console.log(e.target === document.body)
 if (e.code === `ArrowUp`) {
 incrFunc()
 }
})
document.addEventListener(`keyup`, function (e) {
 //console.log(e.target === document.body)
 if (e.code === `ArrowUp`) {
 incrFunc()
 }
})
document.addEventListener(`keyup`, function (e) {
 if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
 resetFunc()
 }
})
resetButton.addEventListener(`keyup`, function (e) {
 if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
 resetFunc()
 }
})
document.addEventListener(`keyup`, function (e) {
 if (e.code === `ArrowDown`) {
 decrFunc()
 }
})
decr.addEventListener(`keyup`, function (e) {
 if (e.code === `ArrowDown`) {
 decrFunc()
 }
})
incrButton.addEventListener(`click`, incrFunc)
resetButton.addEventListener(`click`, resetFunc)
decrButton.addEventListener(`click`, decrFunc)

• Drew Reese: May just be a copy/paste typo, but decr.addEventListener(keyup, ... looks like it should probably be decrButton.addEventListener(keyup, ....

Answer 1

My first advice would be to reformat your code with a linter. I won't go into this in more detail because I assume you're mostly after implementation advice, but as it is it's quite jarring to read, similar to text written in your own language but with foreign capitalization and punctuation.

Some of your functions could use destructuring to make their bodies more terse.

For instance, this:

document.addEventListener(`keyup`, function (e) {
 if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
 resetFunc()
 }
})

Could be rewritten as this, since you're only using the code property of the argument:

document.addEventListener(`keyup`, function ({ code }) {
 if (code === `ArrowRight` || code === `ArrowLeft`) {
 resetFunc()
 }
})

A more controversial bit of advice might be to write this at the top of your script:

const $ = document.querySelector.bind(document);

This allows you to replace this:

const getNumber = document.getElementById(`number`)
const incrButton = document.getElementById(`incr`)
const decrButton = document.getElementById(`decr`)
const resetButton = document.getElementById(`reset`)

With something much less verbose:

const getNumber = $(`#number`)
const incrButton = $(`#incr`)
const decrButton = $(`#decr`)
const resetButton = $(`#reset`)

Answer 2

You should be using a semicolon at the end of lines.

There is no need to have keyup event on the buttons, the document will catch the event. You also only need a single event listener on the document.

document.addEventListener(`keyup`, function (e) {
 if (e.code === `ArrowUp`) {
 incrFunc();
 }
 else if (e.code === `ArrowDown`) {
 decrFunc();
 }
 else if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
 resetFunc();
 }
})

Your functions for increasing, deceasing, and resetting are similar enough that they can be combined, so you don't have to repeat the same code. This way you also have a way set the number to any value or change it by any amount if you need that later.

function setNumber(value) {
 number = value;
 getNumber.textContent = number
 if (number < 0) {
 getNumber.style.color = `red`
 }
 else if (number > 0) {
 getNumber.style.color = `green`
 }
 else {
 getNumber.style.color = `gray`
 }
}
function changeNumber(change) {
 setNumber(number + change);
}

You can set the event listeners with pre-set arguments with bind. The first argument is to set the this keyword, which we don't use, so we can just set it to null.

incr.addEventListener(`click`, changeNumber.bind(null, 1));
reset.addEventListener(`click`, setNumber.bind(null, 0));
decr.addEventListener(`click`, changeNumber.bind(null, -1));

While saving an element to a variable when it's reference multiple times is a good practice, there's no need to when it's only used once.

Lastly, getNumber is not a good descriptive name.

The final code.

const numberDisplay = document.getElementById(`number`);
let number = 0;
setNumber(number);
function setNumber(value) {
 number = value;
 numberDisplay.textContent = number;
 if (number < 0) {
 numberDisplay.style.color = `red`;
 }
 else if (number > 0) {
 numberDisplay.style.color = `green`;
 }
 else {
 numberDisplay.style.color = `gray`;
 }
}
function changeNumber(change) {
 setNumber(number + change);
}
document.addEventListener(`keyup`, function (e) {
 if (e.code === `ArrowUp`) {
 changeNumber(1);
 }
 else if (e.code === `ArrowDown`) {
 changeNumber(-1);
 }
 else if (e.code === `ArrowRight` || e.code === `ArrowLeft`) {
 setNumber(0);
 }
})
document.getElementById(`incr`).addEventListener(`click`, changeNumber.bind(null, 1));
document.getElementById(`reset`).addEventListener(`click`, setNumber.bind(null, 0));
document.getElementById(`decr`).addEventListener(`click`, changeNumber.bind(null, -1));

<h1 id="number"></h1>
<button class="btn" id="incr">Increase</button>
<button class="btn" id="reset">Reset</button>
<button class="btn" id="decr">Decrease</button>

Answer 3

Maybe you could map every key code with a function and avoid to check the keyCode multiple times

const map = {
 'ArrowUp': incrFunc,
 'ArrowRight': resetFunc,
 'ArrowLeft': resetFunc,
 'ArrowDown': decrFunc,
}
document.addEventListener(`keyup`, function (e) {
 if(map[e.code]) {
 map[e.code]();
 }
})

Answer 4

Don't use template strings (`number`) if you are not actually using templates. Use normal single or double quotes: 'number' or "number".

• konijn: My favourite style guide agrees with you, but why though?
• RoToRa: Performance: The JS engine will try to find placeholders where there are none. Readability: The reader may expect that something special is happening here when there is nothing. Writability(?): On some keyboard layouts the back tick can be tricky to type. (These are all my personal thoughts, not based on any style guide).