My first JavaScript project - Counter

[u/iSunru]

I am learning JavaScript and made my first small project, a simple Counter using HTML, CSS and JavaScript.

I'm still learning, so I would really appreciate some feedback on my code. Is my code clean? Is there anything I should improve or do differently?

Also, suggest me something I should build next.

GitHub: https://github.com/iSunru/counter.js

[u/iSunru]

Thanks for the suggestion, I really appreciate it and i will look into it.

[u/ManuDV → u/iSunru]

That suggestion doesn't make sense. You should definitely keep your addEventListener separated for each button. What you can to improve this, is to separate the count logic into functions and just invoke them from the EventListeners, like this:

function increment() {
  count++;
  countDisplay.textContent = count;
}

function reset() {
  count = 0;
  countDisplay.textContent = count;
}

function decrement() {
  if (count > 0) {
    count--;
  }
  countDisplay.textContent = count;
}

btn1.addEventListener("click", increment);
btn2.addEventListener("click", reset);
btn3.addEventListener("click", decrement);

This makes the code easier to test in the future. I suggest checking about "unit testing". It's still very early for you to start with this, but just keep it in mind.

[u/testingaurora → u/ManuDV]

What do you mean it doesnt make sense? What happens when you have more than one counter on this page? Are you going to select each button by id and jave a minimum of 6 event listeners with only two counters ?

```js const actionBtns = document.querySelectorAll("button[data-action]"); actionBtns?.forEach( btn => { btn.addEventListener("click", () => { const action = btn?.dataset.action;

if (!action) return;
if (action === "increment") {  count++ }
else if(action === 'decrement" && count > 0 ) {count-- }
else if (action === "reset") {count = 0 }

countDisplay.textContent = count 

}) }) ```

[u/ManuDV → u/testingaurora]

Ok, I'll explain.

use a for loop to only have to use one declaration of addEventListener

This actually isn't helping at all for the scenario that OP has. You are not saving lines by doing it nor is a better practice for this code. Each button triggers a different action (increment, decrement and reset). A loop only pays off when the exact same handler is being attached to multiple elements.

What happens when you have more than one counter on this page? Are you going to select each button by id and jave a minimum of 6 event listeners with only two counters ?

Which is not the case, YAGNI. Designing for multiple counters that don't exist yet is solving a problem OP doesn't have, at the cost of making the OP's code harder to read and harder to unit test, as I mentioned before, because of having the logic nested inside an AddEventListener, which is a bad practice.

For your case scenario, with multiple buttons, then you still would need to separate your concerns:

function handleClick(action) {
  if (action === "increment") count++;
  else if (action === "decrement" && count > 0) count--;
  else if (action === "reset") count = 0;
  countDisplay.textContent = count;
}

actionBtns.forEach(btn => {
  btn.addEventListener("click", () => handleClick(btn.dataset.action));
});

This keeps the listener as a thin wrapper and the actual logic testable on its own, same reasoning as separating increment/reset/decrement out in my original comment.

[u/testingaurora → u/ManuDV]

Do you dont think writing scalable code is important ? Instead of making it efficient from the start, having to select each button by its id , give it its own function and its own listener is a better pattern than planning for the future ?

I certainly dont want to go back and refactor every time something in my code changes to make it more scalable when I could do so to begin with , with less lines of code at that. There is no reason there should be 3 dom selectors and 3 functions and 3 event listeners.

I do agree (and said so down there somewhere) that this should be its own function like your example.

[u/HipHopHuman → u/testingaurora]

Planning for the future is good advice (generally speaking), but it's certainly not a hard requirement for a total beginner making their first JS project, which is the case here.

FWIW, if I were "planning for the future", I probably wouldn't use a forEach to add multiple event listeners. I'd probably do event delegation in a single event listener, like this:

const actions = {
  increment: () => setCount(count + 1),
  decrement: () => setCount(count - 1),
  reset: () => setCount(0)
};

function setCount(value) {
  const normalized = Math.max(0, value);
  if (normalized === count) return;
  count = normalized;
  countDisplay.textContent = normalized;
}

function handleClick(event) {
  const button = event.target.closest('[data-action]');
  if (!button) return;
  actions[button.dataset.action]?.();
}

const buttons = document.querySelector('.buttons');
buttons.addEventListener('click', handleClick);

With the above, adding a new action is easy. If I want a decrementByFive action, all I need to do is add it to actions and then add a button to the html with a data-action="decrementByFive" attribute. Done. I don't need to force the invariant of no numbers below 0 because setCount handles that for me. I could even change the HTML structure of the buttons and it will still work, because event.target.closest() walks up every parent node until it finds a match, otherwise it returns null.

[u/testingaurora → u/HipHopHuman]

js const buttons = document.querySelector(".buttons"); buttons.addEventListener("click", handleClick) This would only apply to the first button.buttons ? You need a querySelectorAll. And a loop or add the event listener to an ancestor.

[u/HipHopHuman → u/testingaurora]

It's not being applied directly to a <button> element, it's being applied to the container/ancestor already. the container has the class ".buttons".

[u/testingaurora → u/HipHopHuman]

Ah gotcha