5
\$\begingroup\$
function prepareEventHandlers() {
    var sectionButton1 = document.getElementById("sectionButton1");
    var sectionButton2 = document.getElementById("sectionButton2");
    var sectionButton3 = document.getElementById("sectionButton3");
    var sectionButton4 = document.getElementById("sectionButton4");
    var sectionButton5 = document.getElementById("sectionButton5");

    var enabled1 = true;
    var enabled2 = false;
    var enabled3 = false;
    var enabled4 = false;
    var enabled5 = false;


    function checkEnabled() {
        if (enabled1) {
            sectionButton1.setAttribute("class", "sectionButtonEnabled");
        }
        if (enabled2) {
            sectionButton2.setAttribute("class", "sectionButtonEnabled");
        }
        if (enabled3) {
            sectionButton3.setAttribute("class", "sectionButtonEnabled");
        }
        if (enabled4) {
            sectionButton4.setAttribute("class", "sectionButtonEnabled");
        }
        if (enabled5) {
            sectionButton5.setAttribute("class", "sectionButtonEnabled");
        }

    }

    checkEnabled();
    sectionButton1.onmouseover = function() {
        if (enabled1) {
            sectionButton1.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton1.onmouseout = function() {
        if (enabled1) {
            sectionButton1.setAttribute("class", "sectionButtonEnabled");
        }
    };
    sectionButton2.onmouseover = function() {
        if (enabled2) {
            sectionButton2.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton2.onmouseout = function() {
        if (enabled2) {
            sectionButton2.setAttribute("class", "sectionButtonEnabled");
        }
    };
    sectionButton3.onmouseover = function() {
        if (enabled3) {
            sectionButton3.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton3.onmouseout = function() {
        if (enabled3) {
            sectionButton3.setAttribute("class", "sectionButtonEnabled");
        }
    };
    sectionButton4.onmouseover = function() {
        if (enabled4) {
            sectionButton4.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton4.onmouseout = function() {
        if (enabled4) {
            sectionButton4.setAttribute("class", "sectionButtonEnabled");
        }
    };
    sectionButton5.onmouseover = function() {
        if (enabled5) {
            sectionButton5.setAttribute("class", "sectionButtonOver");
        }
    };
    sectionButton5.onmouseout = function() {
        if (enabled5) {
            sectionButton5.setAttribute("class", "sectionButtonEnabled");
        }
    };
}


window.onload = function() {
    prepareEventHandlers();
};
\$\endgroup\$
7
\$\begingroup\$

Well first you should think about refactoring your code. The following

if (enabled1) {
    sectionButton1.setAttribute("class", "sectionButtonEnabled");
}

is repeated a few times with just 2 differences. The enabled flag and the button. So this could easily go into a new function

function SetClassIfEnabled(enabled, btn) {
    if (enabled) {
        btn.setAttribute("class", "sectionButtonEnabled");
    }
}

The following is also repeated:

sectionButton1.onmouseover = function() {
    if (enabled1) {
        sectionButton1.setAttribute("class", "sectionButtonOver");
    }
};
sectionButton1.onmouseout = function() {
    if (enabled1) {
        sectionButton1.setAttribute("class", "sectionButtonEnabled");
    }
};

so this could easily go into another function as well. But I would change one thing. You are adding the mouse events to every button but they only do something if they are enabled. So I would check that first and then add the mouse events.

function AddHoverIfEnabled(enabled, btn) {
     if (enabled) {
        btn.onmouseover = function () {
            btn.setAttribute("class", "sectionButtonOver");
        };
        btn.onmouseout = function () {
            btn.setAttribute("class", "sectionButtonEnabled");
        };
    }
}

Doing just the above will shorten your code quite a bit because you can then call the functions like this

SetClassIfEnabled(enabled1, sectionButton1);
SetClassIfEnabled(enabled2, sectionButton2);
...
AddHoverIfEnabled(enabled1, sectionButton1);
AddHoverIfEnabled(enabled2, sectionButton2);
...

But I took it a step farther. By putting the buttons and the enabled flags into an array you can just iterate over each and do the work if necessary. Doing it this way will make it extremely easy to add another button in the future. The following code can replace all the above mentioned changes.

function prepareEventHandlers() {

    var btnArray = [];
    btnArray[0] = [document.getElementById("sectionButton1"), true];
    btnArray[1] = [document.getElementById("sectionButton2"), false];
    btnArray[2] = [document.getElementById("sectionButton3"), false];
    btnArray[3] = [document.getElementById("sectionButton4"), false];
    btnArray[4] = [document.getElementById("sectionButton5"), false];


    for (i = 0; i < btnArray.length; i++) {
        if (btnArray[i][1] == true) { // section/button is enabled

            var btn = btnArray[i][0];
            btn.setAttribute("class", "sectionButtonEnabled"); // set default class

            // Add Hover Events
            btn.onmouseover = function () {
                this.setAttribute("class", "sectionButtonOver");
            };
            btn.onmouseout = function () {
                this.setAttribute("class", "sectionButtonEnabled");
            };
        }
    }
} 

I would actually take the array out of this function and pass it in as an argument but you get the point.

I'm also assuming the enabled flags may change for any of the buttons. If not and it is only one (as you are showing) you could just eliminate all the code that has nothing to do with button1 because none of the others are enabled so they are not manipulated at all.

\$\endgroup\$
  • \$\begingroup\$ The only quibble I have is that your AddHoverIfEnabled still instantiates multiple identical functions. Creating one function to apply in multiple cases would be better. \$\endgroup\$ – kojiro Dec 3 '11 at 2:24
3
\$\begingroup\$

You don't have to use setAttribute and "class" - the JavaScript property is className. What's more, this can be reduced to a loop, because all five elements have exactly the same general procedure. Here's what I would rewrite it as:

// The "enabled" states of each item:
var enabled = [true, false, false, false, false];

// Set up a function to prepare a section button:
function prepareSectionButton(i) {
    // Fetch the section button:
    var sectionButton = document.getElementById('sectionButton' + i);

    // Assign event handlers:
    sectionButton.onmouseover = function() {
        // Only change if enabled:
        if(enabled[i]) {
            sectionButton.className = 'sectionButtonOver';
        }
    };
    sectionButton.onmouseout = function() {
        // Only change if enabled:
        if(enabled[i]) {
            sectionButton.className = 'sectionButtonEnabled';
        }
    };

    // Check if this one is enabled:
    if(enabled[i]) {
        this.className = 'sectionButtonEnabled';
    }
}

// Finally, prepare our five section buttons:
for(var i = 1; i <= 5; i++) {
    prepareSectionButton(i);
}
\$\endgroup\$
1
\$\begingroup\$

and maybe could also make a wrapper:
function $(id){return document.getElementById(id);}

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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