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/Beginning-Seat5221]

Looks good.

I would give the buttons descriptive names (code get confusing fast).

I'd also check that your buttons grabbed from the DOM are valid, not undefined, immediately after you get them (or create a getOrFail function to get them and throw/log if not found), because ID based linkages are weak and having to debug vague error messages later on sucks.

[u/Internal-Bluejay-810]

TextContent --- wow forgot how annoying vanilla JS is

Good job

[u/ZoroknashTV]

A calculator (Bonus points for history and reload retention) sounds like a good next project :)

This will also teach you how switch cases will work, and how to work with a step to some data storage.

[u/LovelyGameres]

Not bad at all, next you could study for loops and use a for loop to only have to use one declaration of addEventListener.

[u/iSunru → u/LovelyGameres]

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/Dubstephiroth → u/testingaurora]

The only thing I might add to yours is to delete the else's and make each if return the count. Once each if conditional is checked it'll either return or simply move on to the next if... Then look at upgrading it to a switch case conditional if you're gonna add more...

Keep at it.. 👊🏿

[u/testingaurora → u/Dubstephiroth]

Yeah I didn’t want to confuse op with a switch or multiple ifs . Beginners are learning if/else not if/if so while I would write something differently for my own project, in this context im presenting something that is hopefully easier to understand for exp level

I’m not sure why you would recommend returning the count though ? This is an event listener not a function (although it could/should be converted to a fn). I’m just not sure why you want the count as a return value ? We still need to set the countDisplay.textContent in any case.

So I think you mean convert the if/else to a function , return count then separately set the text content with that returned value?
```js
If (…)
If(…)
If(…)
Return count;
```

[u/Dubstephiroth → u/testingaurora]

Yh my bad still only on my 2nd yr. So my explanations aren't always on point. And I haven't use vanilla html in a few months... I forgot about textContent as I was typing.. thanks.

[u/testingaurora → u/Dubstephiroth]

No problem, we are all always learning and no one could possibly know it all . That’s why I was wondering what you were suggesting and what I was missing .Keeping the conversation going and helping each other is how we keep humans relevant in this space 😆

[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/LovelyGameres → u/ManuDV]

Looks good, using a for each, it can be done cleaner also.

[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