Note which colour button was clicked

Question

I have a script that changes the colour, it selects the colour by the id, then it listens what colour was clicked, and if the colour choose was black or whatever other colours, it will do the job.

However, there is a lot of repetition with the if statement. Is there a way to refactor this?

How about if color choosen ${colorClicked} then style bg with = the ${colorAssosiated} or something like that?

Can we re-factor this? But with ES5 best.

var black = document.getElementById("black");
var blue = document.getElementById("blue");
var red = document.getElementById("red");
//01. Listen for button click
var buttonClick = this.addEventListener ("click", function(e) {
 console.log(e.target.id + " button was clicked");
 var colorChosen = e.target.id;
 //02. Change background colour
 if (colorChosen == "black"){
 document.body.style.background = "black";
 console.log('You chose Black!');
 } else if (colorChosen == "blue") {
 document.body.style.background = "blue";
 console.log('You chose Blue!');
 } else if (colorChosen == "red") {
 document.body.style.background = "red";
 console.log('You chose Red!');
 }
});

• Gerardo Furtado: You don't need the if... else or even the template literals, if the colours are only those ones: jsfiddle.net/dcydzbw3. Also, your code doesn't seem to be complete. What are those elements at the beginning? And what is this in this context? When asking a question at CR please try to make the code "complete" for reviewing purposes.

Answer 1

I think in your case you can just do this:

document.body.style.background = colorChosen;

Avoiding all the ugly if else part.

A more robust solution will be using a lookup table to decouple the DOM ids from the proper colors string:

const colorFor = {
 "somethingRed": "red",
 "somethingBlue": "blue",
 "somethingBlack": "black"
};
...
document.body.style.background = colorFor[colorChosen];

With somethingBlaBla I mean a meaninful name that not contains the color. The purpose is to contain the changes in case you want a different color in the future or if you want that more options have the same color.

Another improvement is to have a guard condition to avoid errors or weird situations:

var buttonClick = this.addEventListener ("click", function(e) {
 console.log(e.target.id + " button was clicked");
 var colorChosen = e.target.id;
 if (!colorFor[colorChosen])
 return false;
 document.body.style.background = colorFor[colorChosen];
});

Instead of just return from the function you can log an error or pup up a dialog with a warning to inform the user. Or you can reset to a default color the background.

In the same way you can use a style instead of setting the color on the DOM element.

This could be better in some situation, depends on your app code convention.

Answer 2

I would recommend using data attributes on your buttons, something like:

<button data-color="black">Black!</button>

And then:

var buttonClick = this.addEventListener ("click", function(e) {
 var button = e.target;
 if (!button.hasAttribute('data-color'))
 return;
 document.body.style.background = button.getAttribute('data-color');
});

Also I'm not sure what this is set to here:

var buttonClick = this.addEventListener ("click", function(e) {

But in a global call this usually refers to the window and this will process clicks on any element. You should rather add the listener to the button's nearest container.

• Kruga: I think this answer is better, since it completely decouples the color from the id. This is exactly what data tags are meant to do. Hold additional data which is not meant for other tags. It's also easier to add more buttons, since you don't have to change anything in the code.