HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Mouseover effects for five buttons which may be enabled or disabled

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
enabledmayfiveeffectsdisabledbuttonsmouseoverforwhich

Problem

```
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.set

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#6487, answer score: 7

Revisions (0)

No revisions yet.