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):



&lt;!DOCTYPE html5&gt;

&lt;head&gt;
  &lt;title&gt;Lights on&lt;/title&gt;
  &lt;script&gt;
    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 &lt;= 9; i++) {
        color_single_button(i);
      }
    }

    function randomize() {
      for (var i = 1; i &lt;= 9; i++) {
        document.getElementById(i).innerHTML = random_zero_or_one();
      }
    }

    function randomize_and_color() {
      randomize();
      color_following_numbers();
    }

    function invert_text(id) {
      button = document.getElementById(String(id));
      button.innerHTML = String(button.innerHTML) === "1" ? "0" : "1";
    }

    function invert_text_and_colour(id) {
      invert_text(id);
      color_single_button(id);
    }

    function switch_with_neightbors(current) {
      var neightbors = {
        1: [2, 4, 5],
        2: [1, 3, 4, 5, 6],
        3: [2, 5, 6],
        4: [1, 2, 5, 7, 8],
        5: [1, 2, 3, 4, 6, 7, 8, 9],
        6: [2, 3, 5, 8, 9],
        7: [4, 5, 8],
        8: [4, 5, 6, 7, 9],
        9: [5, 6, 8]
      };
      invert_text_and_colour(current);
      nears = neightbors[parseInt(current, 10)];
      for (var i in nears) {
        console.log(nears[i]);
        invert_text_and_colour(nears[i]);
      }
    }
  &lt;/script&gt;

&lt;/head&gt;
&lt;b&gt;Clicking a button inverts it and its neightbours.
You win when you switch on all the lights.&lt;/b&gt;


&lt;p&gt;
  &lt;button style="height:150px;width:150px" id=1 onclick="switch_with_neightbors(1)"&gt;0&lt;/button&gt;
  &lt;button style="height:150px;width:150px" id=2 onclick="switch_with_neightbors(2)"&gt;1&lt;/button&gt;
  &lt;button style="height:150px;width:150px" id=3 onclick="switch_with_neightbors(3)"&gt;0&lt;/button&gt;
&lt;/p&gt;

&lt;p&gt;
  &lt;button style="height:150px;width:150px" id=4 onclick="switch_with_neightbors(4)"&gt;1&lt;/button&gt;
  &lt;button style="height:150px;width:150px" id=5 onclick="switch_with_neightbors(5)"&gt;1&lt;/button&gt;
  &lt;button style="height:150px;width:150px" id=6 onclick="switch_with_neightbors(6)"&gt;1&lt;/button&gt;
&lt;/p&gt;

&lt;p&gt;
  &lt;button style="height:150px;width:150px" id=7 onclick="switch_with_neightbors(7)"&gt;1&lt;/button&gt;
  &lt;button style="height:150px;width:150px" id=8 onclick="switch_with_neightbors(8)"&gt;1&lt;/button&gt;
  &lt;button style="height:150px;width:150px" id=9 onclick="switch_with_neightbors(9)"&gt;0&lt;/button&gt;
&lt;/p&gt;

&lt;/body&gt;

&lt;/html&gt;




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 &lt;!DOCTYPE html&gt; - there's no 5 in the the doctype declaration.
You're missing a &lt;body&gt; 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:

&lt;style&gt;
  button {
    width: 150px;
    height: 150px;
  }
&lt;/style&gt;


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 polluting the global scope (i.e. accidentally overwriting an existing function, or risking other code doing that to you).

One gotcha is that you can't use onclick anymore, since you'd be referring to a function that's no longer in the global scope. There are ways to get around this of course, but again I'd rather not use onclick anyway.
You've got a stray console.log in there. It's not a big deal, but it's the sort of debug cruft that should be removed before you "ship" the code.


Questions


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


Yes and no. For larger projects, I'd say use separate files. But for a little experiment like this, it's fine. I'd still avoid the onclick attributes, though, in favor of a cleaner separation.

Note that you can also put your script tag at the end of the body element, in which case you won't need to listen for the DOMContentLoaded event, since all the above will of course have been loaded when the script's evaluated.


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


For the button size, see the bit about CSS above. As for creating the buttons, you can do that in JS without too much hassle. And if you do, you can also stick the elements in an array, so you don't need to go through the DOM to find them again. The basic principle would be something like:

var buttons = [];

for(var i = 0; i &lt; ROW_COUNT; i++) {
  var row = document.createElement("div");
  document.body.appendChild(row);

  for(var j = 0; j &lt; COLUMN_COUNT; j++) {
    var button = document.createElement("button");
    button.addEventListener("click", handleClick, false);
    row.appendChild(button);
    buttons.push(button);
  }
}


Maybe you'll want to use a 2D array for the buttons, so it matches the layout (you'll have less index arithmetic to deal with when finding neighbors), but either thing can work.


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


Yes, you seem to using it correctly. Edit: Actually, no. I wasn't paying close enough attention. If you had been iterating the neightbors object it'd be cool (and that's what I thought you were doing), but you're iterating the nears array, which is not a job for for..in.

It's common mistake to use the for..in loop for arrays, since other languages use their for..in that way. But JS's for..in is designed to iterate object properties, specifically; not array elements. It'll (seem to) work, since array elements are properties on the array, and thus get iterated. But your intention is not to iterate them as though they were properties; you want to treat them as array elements.

So: Iterating an object? Use for..in. Iterating an array? Use a regular for (or the forEach function).


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


Well, it doesn't change, so it might as well be outside the function. Any JavaScript runtime from the last decade will no doubt optimize that away, though, so it's not like it's a performance issue or anything (even if it wasn't optimized away, it still wouldn't make a dent in performance). But from a structural standpoint, it's a little iffy to have an essentially constant value be a local variable that's declared and defined on each function invocation.


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


Yes: All of 'em look unnecessary :)

JS is very loose when it comes to type (sometimes it's just plain weird, but that's another story). So getElementById(3) and getElementById("3") are equivalent. And in the case of your neightbors table, there's a twist: As mentioned, all JavaScript property names are strings internally, so when you say neightbors[parseInt(current, 10)]; you're taking a string, turning it into a number, and then JS turns it back into a string again to do the lookup in the neightbors object's properties. So you're doing the opposite of what's "natural" - and yet it still works :)

Similarly, all DOM attribute values are strings by default (since HTML is all text). In your case you've used single digits as IDs (note: as with object properties it's better to assume that IDs should work as plain variable names and follow those rules), but they're rendered as strings. Funny thing is that a string that's just a plain number will be silently coerced to a number when needed. So for instance, an array lookup like [1, 2, 3]["1"] will return 2 just fine, even though array indices are numbers. And similarly, as you've already seen, object lookups will coerce stuff to strings: ({ 42: "foo" })[42].

The most common source of type-confusion is probably mixing strings and numbers. For instance, 2 + "2" yields the string "22", not the number 4. So in such a case, it's a good idea to explicitly cast/convert your variables - either to achieve to desired outcome (e.g. numeric addition), or just to clarify your intention (e.g. yes, I do want string concatenation).


  Is my code modular enough?


It's nicely divided into functions, but you do have some cross-coupling with the DOM elements (i.e. onclick) and some magic numbers/hardcoding.

Don't know if I'd call it "modular" because there's really only one "module": The game. I'd rather ask if it's "well-factored", and to that I'd answer, yeah, it's not bad at all. But of course, I have also written a huge review (see all of the above) for you to chew on :)



Edit: As requested, here's a simple one-button example of using a class to toggle and keep state:



document.querySelector("button").addEventListener("click", function () {
  // Toggle the "active" class on click
  this.classList.toggle("active");
  
  // An example of how to test the button's state (check the console)
  if(this.classList.contains("active")) {
    console.log("Button is now on");
  } else {
    console.log("Button is now off");
  }
}, false);
/* base styling */
button {
  width: 100px;
  height: 100px;
  background-color: red;
  outline: none; /* hides the "halo" that otherwise appears when you click the button */
}

/* active/lit styling */
button.active {
  background-color: green;
}
&lt;button&gt;&lt;/button&gt;