0

I am still fairly new to JavaScript and am trying to deepen my understanding of it through mini projects.

In this counter project, I have managed to get the result what I want:

  • After clicking the "add" button, the counter increment would increase and change color to green.
  • After clicking the "subtract" button, the counter increment would decrease and change color to red.

Below would be my JavaScript code:

//create variables to hold the HTML selectors 
var counterDisplay = document.querySelector('.counter-display');
var counterMinus = document.querySelector('.counter-minus');
var counterPlus = document.querySelector('.counter-plus');
//create variable for the counter 
//what we'll use to add and subtract from 
var count = 0;


//create a function to avoid code redundancy 
function updateDisplay(){
  counterDisplay.innerHTML = count;
};

function toGreen(){
  document.querySelector('.counter-display').style.color = "green";
};
function toRed(){
  document.querySelector('.counter-display').style.color = "red";
};

/*-------------------------------------------*/

updateDisplay();

//EventListeners
counterPlus.addEventListener("click", () =>{
  count++;
  updateDisplay();
  toGreen();
});

counterMinus.addEventListener("click", () =>{
  count--;
  updateDisplay();
  toRed();
});

I separated the color functions but I feel like there's a cleaner way to write this code i.e conditionals like if statements, however, as I'm still learning I don't fully know how to implement this.

**As mentioned, I'm still learning so a thorough explanation/laymen's terms is greatly appreciated!

Please let me know for any clarifications or if more info is needed!

Thank you in advance!

EDIT: Thank you those who have taken the time to help me in sharing their solutions!

KO-d14
  • 115
  • 7
  • What's the question? Is it for a code-review, or is there a specific issue with the current code (eg. unexpected behavior) or some other clarification..? – user2864740 May 15 '21 at 01:54
  • I want to know if there is a better way to write a function to change the color of the counter: EX: //function changeColor() { if (counter increment goes up){ change color to green }else (counter decreases) { change color to red}; }; something like that – KO-d14 May 15 '21 at 02:06
  • There are definitely different ways. Imagine a function like this: `function incrCounter(step) { .. }` with the documentation "Changes the current counter and updates the display depending on movement direction." It might be called like `incrCounter(1)` and `incrCounter(-1)`, respectively. Inside it might look like: `count += step; counterDisplay.innerHTML = count; var color = step >= 0 ? "green" : "red"; counterDisplay.style.color = color;`. – user2864740 May 15 '21 at 02:09

3 Answers3

1

The code you wrote is quite long but it does the job

I'm not sure what do you want exactly, but here are few notes :

  • Use HTML onclick Event instead:
    Instead of adding event listener from javascript you can add it in the HTML code like so: <button onclick="myFunction()">Click me</button>, and whenever the button is clicked myFunction() will be called.
    You can also pass the button as a parameter, for example

function myFunction(element) {
  element.style.backgroundColor = "red";
}
<button onclick="myFunction(this)">Click me</button>
First dev
  • 495
  • 4
  • 17
  • Thank you for the tip and link! To confirm, would var not be acceptable if it is used for global purpose / acceptable as long as it's been hoisted? – KO-d14 May 17 '21 at 23:15
  • @KO-d14, using `var` is totally fine, it's up to you to decide which best, but in most cases, you don't need to access a variable from out of its block scope `{}`, in this case, `let` is better. – First dev May 18 '21 at 15:08
0

You need to make the following few changes:

  • Make use of let and const. If it doesn't change its value in the future then use const else let.
  • You can create an array of buttons (in the below example buttons) so that you can loop over and add event listener on all buttons. So that you don't need to add an event listener on each one of the buttons.
  • You can use data attribute to recognize which type of button it is.
  • Get rid of multiple functions toRed or toGreen, instead make a single function that will change the color of text with only one function changeTextColor.
  • If you just need to change the text of the HTML element then better to use textContent in updateDisplay function.

see innerText vs innerHTML vs label vs text vs textContent vs outerText

//create variables to hold the HTML selectors
const counterDisplay = document.querySelector(".counter-display");
const counterMinus = document.querySelector(".counter-minus");
const counterPlus = document.querySelector(".counter-plus");
//create variable for the counter
//what we'll use to add and subtract from
let count = 0;

//create a function to avoid code redundancy
function updateDisplay() {
  counterDisplay.textContent = count;
}

function changeTextColor(color) {
  counterDisplay.style.color = color;
}

/*-------------------------------------------*/

//EventListeners
const buttons = [counterMinus, counterPlus];
buttons.forEach((btn) => {
  btn.addEventListener("click", (e) => {
    const btnType = e.target.dataset.type;
    if (btnType === "sub") {
      count--;
      changeTextColor("red");
    } else {
      count++;
      changeTextColor("green");
    }
    updateDisplay();
  });
});
.counter-display {
  background-color: black;
  padding: 1rem;
  border-radius: 4px;
  margin-bottom: 1rem;
  font-size: 2rem;
  font-weight: 700;
  text-align: center;
  color: whitesmoke;
}

.button-container {
  display: flex;
  gap: 1rem;
  justify-content: center;
}

.button-container>* {
  padding: .75rem 3rem;
  font-size: 2rem;
  border: none;
  border-radius: 4px;
}

.counter-minus {
  background-color: red;
}

.counter-plus {
  background-color: green;
}
<div class="counter-display"> 0 </div>
<div class="button-container">
  <button class="counter-minus" data-type="sub">-</button>
  <button class="counter-plus" data-type="add">+</button>
</div>
DecPK
  • 24,537
  • 6
  • 26
  • 42
  • Thank you! This was the easiest for me to understand. I do like how the function for eventListener was complied into one for adding, subtracting, and changing color :) Another new thing I have learn from your code is the `data-type` (in html). I was reading it up on this site: https://css-tricks.com/a-complete-guide-to-data-attributes/ but to clarify, is `data-type` an attribute where you can make your own attributes? – KO-d14 May 17 '21 at 23:06
  • `data-*` is `data-attribute` where you can make your own custom `data-attribute`. https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes – DecPK May 17 '21 at 23:43
  • 1
    Noted! Thank you for clarifying. I will keep that in mind going forward. – KO-d14 May 18 '21 at 00:23
0

How about something more app based? You can break your whole thing down into a couple key components with a little glue get something that you can easily build on. Its not very different than what you already have, it just sprinkles in some tried and true patterns.

const widget = (element) => {
  const $minus = element.querySelector('.counter-minus');
  const $plus = element.querySelector('.counter-plus');
  const $display = element.querySelector('.counter-display');
  
  let state = {
    color: 'black',
    count: 0
  };
  
  const update = (next) => {
    state = next;
    render();
  };
  
  const render = () => {
     $display.innerText = state.count;
     $display.style.color = state.color;
  };
  
  const onMinusClick = () => {
    update({ color: 'red', count: state.count - 1 });
  };
  
  const onPlusClick = () => {
    update({ color: 'green', count: state.count + 1 });
  };
  
  $minus.addEventListener('click', onMinusClick);
  $plus.addEventListener('click', onPlusClick);
  
  render();
  
  return () => {
    $minus.removeEventListener('click', onMinusClick);
    $plus.removeEventListener('click', onPlusClick);
  };
};

widget(document.querySelector('#app1'));
widget(document.querySelector('#app2'));
widget(document.querySelector('#app3'));
div {
  margin-bottom: .5rem;
}
<div id="app1">
  <button class="counter-minus">-</button>
  <span class="counter-display"></span>
  <button class="counter-plus">+</button>
</div>

<div id="app2">
  <button class="counter-minus">-</button>
  <span class="counter-display"></span>
  <button class="counter-plus">+</button>
</div>

<div id="app3">
  <button class="counter-minus">-</button>
  <span class="counter-display"></span>
  <button class="counter-plus">+</button>
</div>
  1. First thing you probably notice is that i have multiple instance running of my app. I can do this because I dont rely on global state. You can see that when you call widget, it holds its own variables.

  2. I have state as an object and the only thing that can write to state is the update function. You had something very similar except i combined my state into a single variable and have 1 function in charge of writing to state, and then reacting to it i.e. rendering.

  3. Then I have some event handlers being attached that are just calling update with how they want the state to look. This of course triggers a render to keep the ui consistent with the state. This 1 way data flow is very important to keep code clear. The update function can be more sophistacted and do things like except partial state values and spread state = { ...state, ...next}. it can get pretty crazy.

  4. Lasty i return a function. You can return anything in here, you might want to return an api for a user to interact with you widget with. For example you have a calendar and you want to be able to set the date outside of the widget. You might return { setDate: (date) => update({ date }) } or something. However i return a function that will clean up after the widgets by removing event listeners so garbage collection can reclaim any memory i was using.

Brenden
  • 1,947
  • 1
  • 12
  • 10