CODEWITHSUNDEEP
javascriptrefactoring
whoMe
whoMe

Reputation: 247

What is the best way to refactor this code?

What's a more effective way of writing the following code: 

<button onclick="changeRed()">red</button>
<button onclick="changeGray()">gray</button>
<button onclick="changeBlack()">black</button>    

myLoader = document.getElementsByClassName('preloader');
    function changeGray() {
        for (i = 0; i < myLoader.length; i++) {         
            myLoader[i].setAttribute("id", "gray");
        } 
    }

    function changeBlack() {
        for (i = 0; i < myLoader.length; i++) {
            myLoader[i].setAttribute("id", "black");
        }
    }

    function changeRed() {
        for (i = 0; i < myLoader.length; i++) {
            myLoader[i].setAttribute("id", "red");
        }
    }

I am using a for loop as a workaround for using multiple ID's on a single page (this is for a specific mockup - will not be used in production)

I am looking for a way to write this without using multiple change___() functions.

Upvotes: 1

Views: 57

Answers (2)

MKougiouris
MKougiouris

Reputation: 2861

Refactor so that u accept a color value on the function, and keep only one function to set the value

<button onclick="setColor('red')">red</button>
<button onclick="setColor('gray')">gray</button>
<button onclick="setColor('black')">black</button>    


myLoader = document.getElementsByClassName('preloader');
    function setColor(color) {
        for (i = 0; i < myLoader.length; i++) {         
            myLoader[i].setAttribute("id", color);
            myLoader[i].setAttribute("style", 'color:'+color);
        } 
    }

Upvotes: 1

alanboy
alanboy

Reputation: 379

You could have the color as an argument to the function:

    function changeColor(color) {
        for (i = 0; i < myLoader.length; i++) {
            myLoader[i].setAttribute("id", color);
        }
    }

Upvotes: 2

Related Questions