Advice needed in refactoring/cleaning up my javascript code

Question

I am fairly new to Javascript. I have written a website that works as designed. However, I have a strong feeling that my code could be heavily refactored. Can anyone see anything, besides line spacing, that I could do to clean up my code?

var i1 = document.createElement("img");
var i2 = document.createElement("img");
var i3 = document.createElement("img");
var i4 = document.createElement("img");
var i5 = document.createElement("img");
var i6 = document.createElement("img");
var i7 = document.createElement("img");
var i8 = document.createElement("img");
var i9 = document.createElement("img");
i1.type = "image";
i2.type = "image";
i3.type = "image";
i4.type = "image";
i5.type = "image";
i6.type = "image";
i7.type = "image";
i8.type = "image";
i9.type = "image";
var floatingButton1 = document.getElementById('floatingButton1');
var floatingButton2 = document.getElementById('floatingButton2');
var floatingButton3 = document.getElementById('floatingButton3');
var floatingButton4 = document.getElementById('floatingButton4');
var floatingButton5 = document.getElementById('floatingButton5');
var floatingButton6 = document.getElementById('floatingButton6');
var floatingButton7 = document.getElementById('floatingButton7');
var floatingButton8 = document.getElementById('floatingButton8');
var floatingButton9 = document.getElementById('floatingButton9');
floatingButton1.style.paddingLeft="35px";
floatingButton2.style.paddingLeft="35px";
floatingButton3.style.paddingLeft="35px";
floatingButton4.style.paddingLeft="35px";
floatingButton5.style.paddingLeft="35px";
floatingButton6.style.paddingLeft="35px";
floatingButton7.style.paddingLeft="35px";
floatingButton8.style.paddingLeft="35px";
floatingButton9.style.paddingLeft="35px";
floatingButton1.style.paddingTop="5px";
floatingButton2.style.paddingTop="5px";
floatingButton3.style.paddingTop="5px";
floatingButton4.style.paddingTop="5px";
floatingButton5.style.paddingTop="5px";
floatingButton6.style.paddingTop="5px";
floatingButton7.style.paddingTop="5px";
floatingButton8.style.paddingTop="5px";
floatingButton9.style.paddingTop="5px";

function clearButtons() {
    i = 1;
    while (i < 10) {
        var floatingButtonToClear = document.getElementById('floatingButton' + i);
        floatingButtonToClear.style.backgroundImage="url('Images/whitesquare.png')";     
        i = i + 1;
    }
}

function makeOptionButtons(a, b) {
    i = a;
    while (a <= b) {
        var floatingButtonOptionsClear = document.getElementById('floatingButton' + a);
        floatingButtonOptionsClear .style.backgroundImage="url('Images/buttonUnclicked.png')"; 
        a = a + 1;
    }
}

function clearThumbnails() {   


    if (floatingButton1.hasChildNodes())
    {
    floatingButton1.removeChild(i1);
    }
    if (floatingButton2.hasChildNodes())
    {
    floatingButton2.removeChild(i2);
    }
    if (floatingButton3.hasChildNodes())
    {
    floatingButton3.removeChild(i3);
    }
    if (floatingButton3.hasChildNodes())
    {
    floatingButton3.removeChild(i3);
    }
    if (floatingButton4.hasChildNodes())
    {
    floatingButton4.removeChild(i4);
    }
    if (floatingButton5.hasChildNodes())
    {
    floatingButton5.removeChild(i5);
    }
    if (floatingButton6.hasChildNodes())
    {
    floatingButton6.removeChild(i6);
    }
    if (floatingButton7.hasChildNodes())
    {
    floatingButton7.removeChild(i7);
    }
    if (floatingButton8.hasChildNodes())
    {
    floatingButton8.removeChild(i8);
    }
    if (floatingButton9.hasChildNodes())
    {
    floatingButton9.removeChild(i9);
    }
}



function showImageOnClick1(a) {
    clearThumbnails()
    clearButtons()
    document.getElementById(a).style.visibility = 'visible';
}

function showImageOnClick2(a, b) {
    clearThumbnails()
    clearButtons()
    document.getElementById(a).style.visibility = 'visible';
    document.getElementById(b).style.visibility = 'hidden';
}

function showImageOnClick3(a, b, c) {
    clearThumbnails()
    clearButtons()
    document.getElementById(a).style.visibility = 'visible';
    document.getElementById(b).style.visibility = 'hidden';
    document.getElementById(c).style.visibility = 'hidden';
}

function showImageOnClick4(a, b, c, d) {
    clearThumbnails()
    clearButtons()
    document.getElementById(a).style.visibility = 'visible';
    document.getElementById(b).style.visibility = 'hidden';
    document.getElementById(c).style.visibility = 'hidden';
    document.getElementById(d).style.visibility = 'hidden';
}

function Door() {
    clearThumbnails()
    clearButtons()
    makeOptionButtons(1, 4);    
    i1.src = "Images/tnBB1.png";
    floatingButton1.appendChild(i1);

    i2.src = "Images/tnBB1.png";
    floatingButton2.appendChild(i2);

    i3.src = "Images/tnBB1.png";
    floatingButton3.appendChild(i3);

    i4.src = "Images/tnBB1.png";
    floatingButton4.appendChild(i4);
    i1.onclick = function () {
        showImageOnClick4('Door1', 'Door2', 'Door3', 'Door4')
    };
    i2.onclick = function () {
        showImageOnClick4('Door2', 'Door1', 'Door3', 'Door4')
    };
    i3.onclick = function () {
        showImageOnClick4('Door3', 'Door1', 'Door2', 'Door4')
    };
    i4.onclick = function () {
        showImageOnClick4('Door4', 'Door1', 'Door2', 'Door3')
    };
}

function Trim() {
    clearThumbnails()
    clearButtons()
    makeOptionButtons(2, 4);
    i2.src = "Images/tnS1.png";
    i2.innerHTML = "1";
    floatingButton2.appendChild(i2);

    i3.src = "Images/tnS2.png";
    floatingButton3.appendChild(i3);

    i4.src = "Images/tnR3 - Copy.png";
    floatingButton4.appendChild(i4);
    i2.onclick = function () {
        showImageOnClick3('Trim1', 'Trim2', 'Trim3')
    };
    i3.onclick = function () {
        showImageOnClick3('Trim2', 'Trim1', 'Trim3')
    };
    i4.onclick = function () {
        showImageOnClick3('Trim3', 'Trim1', 'Trim2')
    };
}

function Roof() {
    clearThumbnails()
    clearButtons()
    makeOptionButtons(3, 5);



    i3.src = "Images/tnT3.png";
    floatingButton3.appendChild(i3);

    i4.src = "Images/tnBB3.png";
    floatingButton4.appendChild(i4);

    i5.src = "Images/tnBB1.png";
    floatingButton5.appendChild(i5);

    i3.onclick = function () {
        showImageOnClick3('Roof1', 'Roof2', 'Roof3')
    };
    i4.onclick = function () {
        showImageOnClick3('Roof2', 'Roof1', 'Roof3')
    };
    i5.onclick = function () {
        showImageOnClick3('Roof3', 'Roof1', 'Roof2')
    };
}

function Siding() {
    clearThumbnails()
    clearButtons()
    makeOptionButtons(4, 6);

    i4.src = "Images/tnBB2.png";
    floatingButton4.appendChild(i4);

    i5.src = "Images/tnBB311.png";
    floatingButton5.appendChild(i5);

    i6.src = "Images/tnBB112.png";
    floatingButton6.appendChild(i6);
    i4.onclick = function () {
        showImageOnClick3('Siding1', 'Siding2', 'Siding3')
    };
    i5.onclick = function () {
        showImageOnClick3('Siding2', 'Siding1', 'Siding3')
    };
    i6.onclick = function () {
        showImageOnClick3('Siding3', 'Siding1', 'Siding2')
    };
}

function Stone() {
    clearThumbnails()
    clearButtons()
    makeOptionButtons(5, 7);

    i5.src = "Images/tnBB1.png";
    floatingButton5.appendChild(i5);

    i6.src = "Images/tnR3 - Copy14.png";
    floatingButton6.appendChild(i6);

    i7.src = "Images/tnBB1.png";
    floatingButton7.appendChild(i7);
    i5.onclick = function () {
        showImageOnClick3('Stone1', 'Stone2', 'Stone3')
    };
    i6.onclick = function () {
        showImageOnClick3('Stone2', 'Stone1', 'Stone3')
    };
    i7.onclick = function () {
        showImageOnClick3('Stone3', 'Stone1', 'Stone2')
    };
}

function BB() {
    clearThumbnails()
    clearButtons()
    makeOptionButtons(6, 9);

    i6.src = "Images/tnBB1.png";
    floatingButton6.appendChild(i6);

    i7.src = "Images/tnBB117.png";
    floatingButton7.appendChild(i7);

    i8.src = "Images/tnBB218.png";
    floatingButton8.appendChild(i8);

    i9.src = "Images/tnBB319.png";
    i9.type = "image";
    floatingButton9.appendChild(i9);
    i6.onclick = function () {
        showImageOnClick4('BB1', 'BB2', 'BB3', 'BB4')
    };
    i7.onclick = function () {
        showImageOnClick4('BB2', 'BB1', 'BB3', 'BB4')
    };
    i8.onclick = function () {
        showImageOnClick4('BB3', 'BB1', 'BB2', 'BB4')
    };
    i9.onclick = function () {
        showImageOnClick4('BB4', 'BB1', 'BB2', 'BB3')
    };
}

All of my code can be seen at http://codepen.io/angstd/pen/ropca.

Could you provide some context on this code, since it's a lot? — Jamal, Commented Jul 23, 2014 at 2:19

Pimgd · Accepted Answer · 2014-07-23 09:41:24Z

I'm not so experienced in javascript, so beware syntax errors, but looking at it from what I can read...

This...

var i1 = document.createElement("img");
var i2 = document.createElement("img");
var i3 = document.createElement("img");
var i4 = document.createElement("img");
var i5 = document.createElement("img");
var i6 = document.createElement("img");
var i7 = document.createElement("img");
var i8 = document.createElement("img");
var i9 = document.createElement("img");
i1.type = "image";
i2.type = "image";
i3.type = "image";
i4.type = "image";
i5.type = "image";
i6.type = "image";
i7.type = "image";
i8.type = "image";
i9.type = "image";
var floatingButton1 = document.getElementById('floatingButton1');
var floatingButton2 = document.getElementById('floatingButton2');
var floatingButton3 = document.getElementById('floatingButton3');
var floatingButton4 = document.getElementById('floatingButton4');
var floatingButton5 = document.getElementById('floatingButton5');
var floatingButton6 = document.getElementById('floatingButton6');
var floatingButton7 = document.getElementById('floatingButton7');
var floatingButton8 = document.getElementById('floatingButton8');
var floatingButton9 = document.getElementById('floatingButton9');
floatingButton1.style.paddingLeft="35px";
floatingButton2.style.paddingLeft="35px";
floatingButton3.style.paddingLeft="35px";
floatingButton4.style.paddingLeft="35px";
floatingButton5.style.paddingLeft="35px";
floatingButton6.style.paddingLeft="35px";
floatingButton7.style.paddingLeft="35px";
floatingButton8.style.paddingLeft="35px";
floatingButton9.style.paddingLeft="35px";
floatingButton1.style.paddingTop="5px";
floatingButton2.style.paddingTop="5px";
floatingButton3.style.paddingTop="5px";
floatingButton4.style.paddingTop="5px";
floatingButton5.style.paddingTop="5px";
floatingButton6.style.paddingTop="5px";
floatingButton7.style.paddingTop="5px";
floatingButton8.style.paddingTop="5px";
floatingButton9.style.paddingTop="5px";

could be this:

var imageArray = [];
var floatingButtonArray = [];
for (i = 0; i < 9; i++) { 
    imageArray[i] = document.createElement("img");
    imageArray[i].type = "image";
    floatingButtonArray[i] = document.getElementById('floatingButton'+(i+1));
    floatingButtonArray[i].style.paddingLeft = "35px";
    floatingButtonArray[i].style.paddingTop = "5px";
}

Any time you do stuff like this...

function clearThumbnails() {   


    if (floatingButton1.hasChildNodes())
    {
    floatingButton1.removeChild(i1);
    }
    if (floatingButton2.hasChildNodes())
    {
    floatingButton2.removeChild(i2);
    }
    if (floatingButton3.hasChildNodes())
    {
    floatingButton3.removeChild(i3);
    }
    if (floatingButton3.hasChildNodes())
    {
    floatingButton3.removeChild(i3);
    }
    if (floatingButton4.hasChildNodes())
    {
    floatingButton4.removeChild(i4);
    }
    if (floatingButton5.hasChildNodes())
    {
    floatingButton5.removeChild(i5);
    }
    if (floatingButton6.hasChildNodes())
    {
    floatingButton6.removeChild(i6);
    }
    if (floatingButton7.hasChildNodes())
    {
    floatingButton7.removeChild(i7);
    }
    if (floatingButton8.hasChildNodes())
    {
    floatingButton8.removeChild(i8);
    }
    if (floatingButton9.hasChildNodes())
    {
    floatingButton9.removeChild(i9);
    }
}

You could be doing this instead:

function clearThumbnails(){
    for(i = 0; i < 9; i++){
        if(floatingButtonArray[i].hasChildNodes()){
            floatingButtonArray.removeChild(imageArray[i]);
        }
    }
}

This alone would shrink your code so much, you have no idea. For the examples given, you'd go from 90 lines to 17. However, it's not the number of lines that's important. What's important is the amount of repetition in your code.

"But wait, I have other images and they're called i101, i102 and i103!"

Put the image names in an array. Split the image arrays depending on meaning. If those i101 and so are special, put them in a seperate array. But in general, do your best to avoid repetition.

Ben · Accepted Answer · 2014-07-23 10:04:36Z

8

To add to what @Pimgd said, instead of using a while loop in clearButtons and makeOptionButtons and you should use a for loop.

Whenever you see this pattern:

var i = 1;
while (i < 10) {
    // ...
    i = i + 1;
}

replace it with this:

for (var i = 1; i < 10; i++) {
    // ...
}

As an aside, in Javascript unless you explicitly intend to create a global variable (which you nearly always should not), then you should use the var keyword. For example your i = 1; should be var i = 1;.

answered Jul 23, 2014 at 10:04

Ben

1812 bronze badges

\$\begingroup\$ Does your recommendation to use for instead of while in cases like this come from some established set of best practices or style guide? If so, do you have a reference? If not, is this just your personal opinion? I agree that using for instead of while in cases like this is more idiomatic, but for a beginner, I think the while syntax is much easier to understand/remember, and for is more of a shorthand. If it's easier for the OP to understand his own code with while, or the code just evolved that way, I see no reason to recommend changing it. \$\endgroup\$
– Dagg
Commented Jul 28, 2014 at 13:33

Add a comment |

janos · Accepted Answer · 2014-07-23 16:41:10Z

Adding to what others have already said.

Instead of these functions:

function showImageOnClick1(a) {
    clearThumbnails()
    clearButtons()
    document.getElementById(a).style.visibility = 'visible';
}
function showImageOnClick2(a, b) {
    // ...
}
function showImageOnClick3(a, b, c) {
    // ...
}
function showImageOnClick4(a, b, c, d) {
    clearThumbnails()
    clearButtons()
    document.getElementById(a).style.visibility = 'visible';
    document.getElementById(b).style.visibility = 'hidden';
    document.getElementById(c).style.visibility = 'hidden';
    document.getElementById(d).style.visibility = 'hidden';
}

You could simplify to use only one:

function showImageOnClick(a) {
    clearThumbnails()
    clearButtons()
    document.getElementById(a).style.visibility = 'visible';
    for (var i = 1; i < arguments.length; ++i) {
        var arg = arguments[i];
        document.getElementById(arg).style.visibility = 'hidden';
    }
}

You could replace all your calls to showImageOnClick1 .. showImageOnClick4 with this one, it will work for all of them even though only the first argument is named (a).

You could also simplify this kind of code:

i1.onclick = function () {
    showImageOnClick4('Door1', 'Door2', 'Door3', 'Door4')
};
i2.onclick = function () {
    showImageOnClick4('Door2', 'Door1', 'Door3', 'Door4')
};
i3.onclick = function () {
    showImageOnClick4('Door3', 'Door1', 'Door2', 'Door4')
};
i4.onclick = function () {
    showImageOnClick4('Door4', 'Door1', 'Door2', 'Door3')
};

You can create a helper function that will create a function:

function showImageOnClickMaker() {
    var args = arguments;
    return function() { showImageOnClick.apply(this, args); }
}

Which will simplify creating the onclick functions with a single line and less boilerplate code:

i1.onclick = showImageOnClickMaker('Door1', 'Door2', 'Door3', 'Door4');
i2.onclick = showImageOnClickMaker('Door2', 'Door1', 'Door3', 'Door4');
i3.onclick = showImageOnClickMaker('Door3', 'Door1', 'Door2', 'Door4');
i4.onclick = showImageOnClickMaker('Door4', 'Door1', 'Door2', 'Door3');

Mark Robbins · Accepted Answer · 2014-07-28 10:19:37Z

I am not going to refactor much code, but just offer some general advice.

The first thing as a beginner is you need to get used to separating your data from your functions. The simplest way to to this is to break down your data into a global obect (in this case). Think of the parts of your app and break it down like so..

(I'm going to wing it here but you will get the idea)

var appData={
  floatingButtons:{
    count:9
    ,style:{
      paddingLeft:'35px'
      ,paddingTop:'5px'
    }
    ,clearStyle:{
      backgroundImage:"url(blah de blah)"
    }
    ,unclickedStyle:{
      backgroundImage:"url(blah de blah)"
    }
    ,elements:{
    }
  }
  /// blah, blah, more like this
};


// now lets make some buttons
function makeFloatingButtons(){
  var buttons=appData.floatingButtons;
  function getButton(x){
    var rv=buttons.elements['floatingButton'+x]=document.getElementById('floatingButton'+x);
    return rv;
  }
  function styleButton(el){
    var styles=buttons.style;
    var o=el.style;
    for (var i in styles) {//see how this is magic? just declare new styles when you need them
      o[i]=styles[i];
    }
  }
  for (var x=0;x<buttons.count;x+=1) {
    var btn=getButton(x);
    styleButton(btn);
  }
}

function clearFloatingButtons(){
  var buttons=appData.floatingButtons;
  //easy
  for (var i in buttons.elements) {
    var style=buttons.elements[i].style;
    for (var j in buttons.clearStyle) {
      style[j]=buttons.clearStyle[j];// more magic
    }
  }
}

Concentrate on the structure of your data, and genericizing your functions, then eventually you will learn how to move your functions into that global object and then start thinking in an OOP manner (once you see how functions and data CAN live like brother and sister IF YOU ARE ORGANIZED).

Stack Exchange Network

Advice needed in refactoring/cleaning up my javascript code

4 Answers 4

Your Answer

Not the answer you're looking for? Browse other questions tagged
javascript
or ask your own question.

Hot Network Questions

Advice needed in refactoring/cleaning up my javascript code

4 Answers 4

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascript or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
javascript
or ask your own question.