Lights on: playing with buttons in Javascript

First play with the game a little bit, and it is quite fun (a little hard but very satisfying when you win, be sure to put it full page):





Lights on

document.addEventListener('DOMContentLoaded', randomize_and_color, false);

function random_zero_or_one() {
return Math.floor(Math.random() * 2);
}

function color_single_button(id) {
var number = parseInt(document.getElementById(parseInt(id)).innerHTML);
if (number == 0) {
document.getElementById(id).style.background = '#CC0000';
} else {
document.getElementById(id).style.background = '#00FF00';
}
}

function color_following_numbers() {
for (var i = 1; i

Clicking a button inverts it and its neightbours.
You win when you switch on all the lights.

0
1
0

1
1
1

1
1
0





Ok, now the bad news:

-
I put HTML and JavaScript together. Would separating them improve readability?

-
I generate 9 buttons by hand, and I copy-paste the size for each one. Can I avoid so much repetition?

-
I am confused by the looping constructs of JavaScript. Is my use of the for .. in sensible?

-
I put the neighbours hash inside a function. Is this standard practice? Is this weird?

-
I cast variables to different types all the time. Can you help me spot unnecessary conversions?

-
Is my code modular enough?

I'll just start by looking at what you have, and then I'll get to your questions. (Edit: Seems I'm retreading a lot of what SirPython already covered. Didn't see that answer before I posted. Apologies for boring the reader, and no plagiarism of SirPython intended. Don't mess with a knighted snake!)

The HTML

-
The doctype for HTML5 is just ` - there's no 5 in the the doctype declaration.

-
You're missing a opening tag.

-
I'd recommend you consistently use quotes around your attribute values. Right now you have id=3 with no quotes. It's perfectly valid to skip them (provided the attribute value doesn't have spaces in it), but more often than not, you end up needing the quotes around other values anyway, so for the sake of consistency: Always use quotes.

-
I'm not keen on using p elements for the button rows. Semantically speaking they're not really paragraphs, per se. I'd use divs, since they're semantically neutral and more geared toward pure layout concerns.

-
Avoid style attributes in the HTML. Especially here, as it all repeats verbatim for each button. I'd suggest putting CSS in a style element in the head:

  button {
    width: 150px;
    height: 150px;
  }

That'll take care of all of them.

-
Avoid onclick attributes. They're sooo last century. But seriously, it's generally considered better form to use addEventListener to attach event handlers via JavaScript, rather than having little bits of JS scattered all over the HTML.

If you do use onclick know that you can pass this to the function, i.e. onclick="switch_with_neighbors(this)". In an attribute, this refers to the button itself, so you don't have to go find it again in the function (or repeat the id attribute's value in the onclick attribute).

The JavaScript

-
The JS naming convention is camelCase, not snake_case. Right now, you end up having to mix the two, since the DOM already uses camelCase (e.g. getElementById) so consistency suffers.

-
You have a typo that seems to have been repeated (I'm guessing your editor auto-completed it for you): "neightbors". It's also in the HTML.

-
You have a couple of unintentionally global variables. If you neglect the var keyword, an variable assignment/initialization like foo = bar is interpreted as window.foo = bar, i.e. the variable becomes a property on the window object (which in browser JavaScript means it's a global variable).

The ones I spotted were button in invert_text() and nears in switch_with_neightbors().

-
The way you track and set a button's state is somewhat brittle, relying on its innerHTML, some type conversion, and manually changing its background color. I'd recommend using classes instead. You can check the presence of a class with someElement.classList.contains("someClass"), and use classList.add/classList.remove or simply classList.toggle to toggle the class on and off.

So for instance you could have a lit class, or more conventionally (though less domain-specific) an active class. The class would define background color, so adding/removing it from the classlist would cause all the effects you want.

Edit: I've added an example at the end of the answer.

-
There's are a few too many hardcoded bits in the code for my liking. Several hardcoded 9s, and of course the table of neighbors. I'd extract the 9 into a "constant", just so it's not a magic number floating around. As for the neighbors-table, you can work that out at runtime. You don't even need element IDs; a getElementsByTagName or querySelectorAll will return the elements in their document order. So you'll have an array (well, a NodeList instance but for our purposes it's the same) and with a little bit of index arithmetic you can work out the neighbors. It's more code though, so I maybe wouldn't bother too much. Still, the neighbors table just irks me a little.

-
Speaking of the neightbors (sic) table, note that object properties are always considered strings (more on that later), so using a number literal as a property name is suspect, e.g. { 3: someValue }. In fact, I wouldn't be surprised if some browsers choke on it, since it seems to be bordering on a syntax error (not that I know the ECMA spec by heart).

Basically, you can access object properties with dot notation - i.e. obj.property - or with subscripting like an array - i.e. obj[property]. The first option becomes impossible if you just have a number as the property name, since obj.3 is a syntax error.

It's often better for property names to follow the same rules as variable names, i.e. can't start with a number, have spaces, or just be a number. So e.g. button3 would be nicer.

-
For bonus points, shove everything inside an IIFE (immediately-invoked function expression):

(function () {
  // all your code
})();

This causes all of your functions to be defined in a local scope, rather than the global window` scope. Thus you avoid pol

StackExchange Code Review Q#102728, answer score: 14