How do I make this JavaScript shorter/better without frameworks?

Question

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();
};

Answer 1

EDIT

I agree with a lot of the other answers talking about storing your data in arrays, but instead of parallel arrays, I would use one array of objects:

var i, buttonData = [];
for(i = 1; i <= 5; i++)
   buttonData.push({ "enabled" : false, 
                     "button": document.getElementById("sectionButton" + i) });
buttonData[0].enabled = true;

And then:

for (i = 0; i < buttonData.length; i++) {
     setClassIfEnabled(buttonData[i].enabled, buttonData[i].button)
}

Or if you want to keep it simple, the original answer below will still chop a lot of code out of your original version:

---

Refactor duplicated code with a helper method

function setClassIfEnabled(enabled, button){
    if (enabled) {
          button.setAttribute("class", "sectionButtonEnabled");
    }
}

And then

function checkEnabled() {
     setClassIfEnabled(enabled1, sectionButton1);
     setClassIfEnabled(enabled2, sectionButton2);
     setClassIfEnabled(enabled3, sectionButton3);
     setClassIfEnabled(enabled4, sectionButton4);
     setClassIfEnabled(enabled5, sectionButton5);
}

Also

function setMouseOverIfEnabled(enabled, button) {
    button.onmouseover = function() {
        if (enabled) {
            button.setAttribute("class", "sectionButtonEnabled");
        }
    };
}

setMouseOverIfEnabled(enabled1, sectionButton1);
setMouseOverIfEnabled(enabled2, sectionButton2);
setMouseOverIfEnabled(enabled3, sectionButton3);
setMouseOverIfEnabled(enabled4, sectionButton4);
setMouseOverIfEnabled(enabled5, sectionButton5);

And of course do the same thing for mouseout

Also, you may want to consider using addEventListener to add your events

function setMouseOverIfEnabled(enabled, button) {
    button.addEventListener("mouseover", function() {
        if (enabled) {
            button.setAttribute("class", "sectionButtonEnabled");
        }
    });
}

Answer 2

Here's a condensed version. You definitely want to never repeat blocks of code with just different variables. Either use a loop or make a local function. In this case, since your IDs are sequential, a loop works well here:

function prepareEventHandlers()
{
    var button;
    var enabled = [true, false, false, false, false];
    for (var i = 0; i < enabled.length; i++) {
        button = document.getElementById("sectionButton" + (i + 1));
        button.buttonEnabled = enabled[i];
        if (button.buttonEnabled) {
           button.className = "sectionButtonEnabled";
        }
        button.onmouseover = function() {
            if (this.buttonEnabled) {
                this.className = "sectionButtonOver";
            }
        }
        button.onmouseout = function() {
            if (this.buttonEnabled) {
                this.className = "sectionButtonEnabled";
            }
        }
    }
}

This code also allows you to enable the button later in other code by manipulating the buttonEnabled property and/or className of the button and the event handlers will automatically do the right thing.

Answer 3

First off: Using onmouseover and onmouseout for styling buttons is some stuff from the 1990s. You can do the same thing with CSS now.

.sectionButtonEnabled       { regular styles here }
.sectionButtonEnabled:hover { mouseover styles here }

(Note, for IE, this requires "standards mode" -- read: have a doctype line -- and IE7 or later.)

Now, if you really want to do things the old and busted way...

function prepareEventHandlers() {
    var buttons = [
        "sectionButton1",
        "sectionButton2",
        "sectionButton3",
        "sectionButton4",
        "sectionButton5"
    ];
    var enabled = [ true, false, false, false, false ];

    for (var i = 0; i < buttons.length; ++i) {
        var elem = document.getElementById(buttons[i]);
        if (enabled[i]) {
            elem.className   = "sectionButtonEnabled";

            // Since you're only attaching the event handler to enabled buttons,
            // you already know that `enabled` is true. So you don't even need to
            // check, since there's no way to change your local variable.

            elem.onmouseover = function() {
                this.className="sectionButtonOver";
            };
            elem.onmouseout  = function() {
                this.className="sectionButtonEnabled";
            };
        }
    }
}

If you don't need the mouseover handlers, you can get rid of those and just use the CSS mentioned above.

Answer 4

Anytime you find yourself writing variable names like "foo1", "foo2", etc, and they all do more or less the same thing, you really need to stop, back up, and declare an array.

function prepareEventHandlers() {
    var sectionButtons = [];
    for (var i = 1; i <= 5; ++i)
      sectionButtons[i] = document.getElementById('sectionButton' + i);

    var enabled = [ true, false, false, false, false ];

    function checkEnabled() {
        for (var i = 1; i <= 5; ++i)
          if (enabled[i]) sectionButtons[i].className = 'sectionButtonEnabled';
    }

    checkEnabled();

    for (i = 1; i <= 5; ++i) {
      sectionButton[i].onmouseover = function(i) {
        return function() {
          if (enabled[i]) sectionButton[i].className = 'sectionButtonOver');
        }
      }(i);
      sectionButton[i].onmouseout = function(i) {
        return function() {
          if (enabled[i]) sectionButton[i].className = 'sectionButtonEnabled';
      }(i);
    }
}


window.onload = function() {
    prepareEventHandlers();
};

Now, two other things:

1. Don't set the "class" attribute with "setAttribute()". Instead, manipulate the "className" property of the DOM element.

2. Instead of setting the class directly to those strings, it's better to construct your own "addClass()" and "removeClass()" functions. Keep in mind that the class can be a list of class names, separated by spaces. Such functions would look something like this:

   function addClass(elem, c) {
     elem.className += ' ' + c;
   }
   
   function removeClass(elem, c) {
     elem.className = elem.className.replace(new RegExp('\\b' + c + '\\b', 'g'), ''));
   }