How could this be written better/simpler?

user_14315629 0
Viewed 64

I'm practising my JavaScript skills a came up with an idea of a button that changes through an array of colours every time I click it.

const button = document.querySelector('button');
const colours = ['red', 'yellow', 'green', 'blue'];
let cycle = 1;
button.style.backgroundColor = colours[0];
button.addEventListener('click', e => {

    button.style.backgroundColor = colours[cycle];
    cycle++;
    if (cycle == 4) {
        cycle = 0;
    };
});
javascript
4 Answers

You could make use of post-incrementation and modulo operator. Also, in the event callback, you can use e.target instead of button variable directly:

const button = document.querySelector('button');
const colours = ['red', 'yellow', 'green', 'blue'];
let cycle = 1;
button.style.backgroundColor = colours[0];
button.addEventListener('click', e => {
    e.target.style.backgroundColor = colours[cycle++ % colours.length];
});
<button>M</button>

user_70805480

Your solution is pretty nice and clear, I can just show you how to do it without using cycle variable and use only the colours array and array functions to get the current position and increment it. I'm using the remainer of division by colours array lenght to stay in array size

const button = document.querySelector('button');
const colours = ['red', 'yellow', 'green', 'blue'];
button.style.backgroundColor = colours[0]
button.addEventListener('click', e => {
    button.style.backgroundColor = colours[(colours.indexOf(button.style.backgroundColor)+1)%colours.length];
});
<button>Click me!</button>

user_111297680

Taking a different and more functional programming approach - you could create a cycleColours function, which uses a closure and returns a function to cycle over the colours, which returns the next in the array each time it is called.

This way, you can re-use this function elsewhere, and abstracts the logic from the click handler against the button.

const cycleColours = (coloursArray = []) => {
  const colours = coloursArray;
  let index = 0;

  const incrementIndex = () => {
    index++;
    if (index >= colours.length) index = 0;
  };

  return () => {
    const colour = colours[index];
    incrementIndex();
    return colour;
  };
};

const coloursButtonOne = ['red', 'yellow', 'green', 'blue']
const getButtonOneColour = cycleColours(coloursButtonOne);

const coloursButtonTwo = ['yellow', 'pink', 'purple', 'white']
const getButtonTwoColour = cycleColours(coloursButtonTwo);

const buttonOne = document.querySelector('button#one');
const buttonTwo = document.querySelector('button#two');

buttonOne.style.backgroundColor = getButtonOneColour();
buttonTwo.style.backgroundColor = getButtonTwoColour();

buttonOne.addEventListener('click', e => {
    e.target.style.backgroundColor = getButtonOneColour();
});

buttonTwo.addEventListener('click', e => {
    e.target.style.backgroundColor = getButtonTwoColour();
});
<button id="one">Click Me!</button>
<button id="two">Click Me!</button>

user_88002540

These are my five cent:

My strategy is

  1. Style as much as you can with CSS. Try to prevent inline styles or setting style with JavaScript.
  2. Don't directly add EventListeners to an element. Add EventListeners to the document instead. Use the event bubbling.
  3. Encapsulate variables names (that's what the IIFE is for)

// IIFE to encapsulate variable names
(function(w, d) {
  const className = 'color-cycle';
  const selector = `.${className}`;
  const cycles = 4

  // add EventListener to document not the element
  d.addEventListener('click', e => {
    // check the target of the event, if it is inside the button
    const button = e.target.closest(selector);
    if (button) {
      // Use `data-` attribute to store cycle
      button.dataset.cycle = (parseInt(button.dataset.cycle || 0) + 1) % cycles
    }
  });
}(window, document));

// For demonstration purposes
(function(d) {
  const selector = `#add-button`;
  const className = 'color-cycle';

  d.addEventListener('click', e => {
    const button = e.target.closest(selector);
    if (button) {
      const newButton = d.createElement('button');
      newButton.classList.add(className);
      newButton.textContent = 'Button'
      d.body.appendChild(newButton);
      d.body.appendChild(d.createTextNode("\n"))
    }
  })
}(document));
button {
  padding: 10px;
  border-radius: 10px;
  border: 1px solid gray;
  outline: none;
}

button:hover,
button:focus {
  border-color: black;
  border-width: 2px;
  padding: 9px;
}

button.color-cycle {
  display: inline-block;
  margin: 2px;
}
button.color-cycle,
button.color-cycle[data-cycle="0"] {
  background-color: red;
}

button.color-cycle[data-cycle="1"] {
  background-color: yellow;
}

button.color-cycle[data-cycle="2"] {
  background-color: green;
}

button.color-cycle[data-cycle="3"] {
  background-color: blue;
}
<button id="add-button">Add Button</button><br><br>

<button class="color-cycle">Button 1</button>
<button class="color-cycle">Button 2</button>
<button class="color-cycle">Button 3</button>
<button class="color-cycle">Button 4</button>

user_4769510
Related