How can I refactor this color change with multiple ids

Question

How can I make it simpler in vanilla JS? At the moment it's 4 times similar element:

function display_clear(){
    red = document.getElementById("red");
    red.style.backgroundColor = default_shadow;
    red.style.boxShadow = default_background;
    blue = document.getElementById("blue");
    blue.style.backgroundColor = default_shadow;
    blue.style.boxShadow = default_background;
    green = document.getElementById("green");
    green.style.backgroundColor = default_shadow;
    green.style.boxShadow = default_background;
    orange = document.getElementById("orange");
    orange.style.backgroundColor = default_shadow;
    orange.style.boxShadow = default_background;

Answer 1

Like @Bergi said: Use a function with a parameter, then call it multiple times.

The challenge is in identifying parts of the code that can be repeated as-is versus parts of the code that should be repeated for a parameter. The "as-is" parts can be moved into the function with said parameters. This decision can only be made based on the needs of your application.

If you are always going to set the same color and shadow to a list of elements, you can write:

function setDefaultAttributes(elementId){
    let box = document.getElementById(elementId);
    box.style.backgroundColor = default_background;
    box.style.boxShadow = default_shadow;
}

// Calling same function multiple times for each color
function display_clear(){
    setDefaultAttributes("red");
    setDefaultAttributes("blue");
    setDefaultAttributes("green");
    setDefaultAttributes("orange");
}

// Alternate: Calling same function for each element of the array
// using the forEach loop    
function display_clear(){
    ["red", "blue", "green", "orange"].forEach(function(element)){
        setDefaultAttributes(element);
    });
}

If you are always going to set the same color but not the shadow to all the box elements, the default_background from your code alone should be placed inside the function, with the shadow becoming a parameter.

Assuming the default_background variable is accessible to the function, you can write:

function setBoxAttributes(id, shadow){
    let box = document.getElementById(id);
    box.style.backgroundColor = default_background;
    box.style.boxShadow = shadow;
}

function display_clear(){
    setBoxAttributes("red", default_shadow);
    setBoxAttributes("blue", default_shadow);
    setBoxAttributes("green", default_shadow);
    setBoxAttributes("orange", orange_shadow);
} 

If you are going to set different color/shadow combinations under different cases, then you can write:

function setBoxAttributes(id, bgColor, shadow){
    let box = document.getElementById(id);
    box.style.backgroundColor = bgColor;
    box.style.boxShadow = shadow;
}

function display_clear(){
    setBoxAttributes("red", default_shadow, default_background);
    setBoxAttributes("blue", blue_shadow, default_background);
    setBoxAttributes("green", default_shadow, green_background);
    setBoxAttributes("orange", default_shadow, default_background);
}

Answer 2

Try this.

    function setStyle(element_id) {
       var element = document.getElementById(element_id);
       element.style.backgroundColor = default_shadow;
       element.style.boxShadow = default_background;
    }
    function display_clear(){
        ["red", "blue", "green","orange"].forEach(element_id=> setStyle(element_id))
    }

Answer 3

I tried this, but your guys solutions seems so much more neat:

function display_clear(){
  var colours = ["red", "green", "blue", "orange"];
  var x;
  for (x = 0; x <= colours.length; x++){
      colour = document.getElementById(colours[x]);
      colour.style.backgroundColor = default_background;
      colour.style.boxShadow = default_shadow;
  }

Answer 4

Try this:

function display_clear() {
  var colors = ['red', 'blue', 'green', 'orange'];
  var default_shadow = 'red';
  var default_background = 'yellow';
  colors.forEach((color) => {
    console.log(color);
    var element = document.getElementById(color);
    element.style.backgroundColor = default_shadow;
    element.style.boxShadow = default_background;
  });
}

Answer 5

When trying to refactor code that is doing the same thing over and over, you have to ask yourself this question: "what is the instruction I am repeating and what are the variables ?"

orange = document.getElementById("orange");
orange.style.backgroundColor = default_shadow;
orange.style.boxShadow = default_background;

In this particular case, this set of instructions is repeated, which means it can be isolated. Now you have to think on the variables. It seams that the only varying factor is the name of the color, as a string. You can then write a function that will completed this particular set of tasks, taking in account the varying factor:

function setBackground (color) {
  let element = document.getElementById(color);
  element.style.backgroundColor = default_shadow;
  element.style.boxShadow = default_background;
}

Then you can call your function 4 times, which will execute extactly the same code

setBackground("red");
setBackground("blue");
setBackground("green");
setBackground("orange");

However, I would consider this as a bad practice, since code is again repeated. If at some point in the future your colors would change, you woudl have to find the particular spot in your code where you call this function, and modify the color where it is called. This is considered as a bas practice in general. You want to isolate your data from the code using this data.

You have a list of colors. WHat sould come into your mind is: I could put those colors in an array ! And yes, in this case this would be the best solution I think.

// where you define the constants being used by your code
let colors = ["red", "blue", "green", "orange"];

// somewhere else, where you want to call setBackgound()
colors.forEach(color => {
  setBackground(color);
});

This way, if you ever need to add a color, or maybe change the logic : your now get the colors from a server, because this is how your app works now. The part using the data will remain unchanged: the logic will remain. You will only have to change the way you get your colors.

Answer 6

You can use a for Loop here by changing the elements

var elements = ["red","blue","green","orange"]; // make an array of elements name 
for(var i=0;i<elements.length;i++){
    var elem = document.getElementById(elements[i]);   // getting all elemennts one by one
    elem.style.backgroundColor = default_shadow;
    elem.style.boxShadow = default_background;
}

hope this help, you can also do this by using a function

function setElementStyle(id){
     var elem = document.getElementById(id);   // getting all elemennts one by one
    elem.style.backgroundColor = default_shadow;
    elem.style.boxShadow = default_background;
}

//call the function one by one
setElementStyle("red");
setElementStyle("blue");
setElementStyle("green");
setElementStyle("orange");

Answer 7

As it's been suggested in comments just use a function like that

function colorChange(elId, bg, shadow) {
}

Then you describe in function things you want to do with these values and then just invoke it as many time as you want.