12
\$\begingroup\$

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

<!DOCTYPE html5>

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

    function randomize() {
      for (var i = 1; i <= 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]);
      }
    }
  </script>

</head>
<b>Clicking a button inverts it and its neightbours.
You win when you switch on all the lights.</b>


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

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

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

</body>

</html>

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?

\$\endgroup\$
  • \$\begingroup\$ a lot has been said already. one point: your array of neighbours is not necessary. the neighbours of a button i are i - 1, i + 1, i + WIDTH - 1, i + WIDTH + 1, i - WIDTH - 1, i - WIDTH + 1, filtered to keep only the values that are within the boundaries (0 to HEIGHT * WIDTH - 1). Moreover, you could use 2 coordinates, x,y and combine them to create clear ids x_y. The neighbors then are even easier to compute based on x and y. \$\endgroup\$ – njzk2 Sep 4 '15 at 3:41
  • \$\begingroup\$ @njzk2 that is a good suggestion to be put in an answer \$\endgroup\$ – Caridorc Sep 4 '15 at 7:08
15
\$\begingroup\$

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 <!DOCTYPE html> - there's no 5 in the the doctype declaration.

  • You're missing a <body> 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:

    <style>
      button {
        width: 150px;
        height: 150px;
      }
    </style>
    

    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 < ROW_COUNT; i++) {
  var row = document.createElement("div");
  document.body.appendChild(row);

  for(var j = 0; j < 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;
}
<button></button>

\$\endgroup\$
  • \$\begingroup\$ nears is an array. iterating with for (var i in nears) is not ok. \$\endgroup\$ – njzk2 Sep 4 '15 at 3:34
  • \$\begingroup\$ @njzk2 Oops, you're right. Will edit. \$\endgroup\$ – Flambino Sep 4 '15 at 3:35
  • \$\begingroup\$ Very nice and thought out answer. \$\endgroup\$ – The Angry Saxon Sep 4 '15 at 8:15
  • \$\begingroup\$ I am having trouble understanding 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. could you include a small example of that? \$\endgroup\$ – Caridorc Sep 4 '15 at 8:25
  • 1
    \$\begingroup\$ @Caridorc Added an example snippet to my answer \$\endgroup\$ – Flambino Sep 4 '15 at 9:17
10
\$\begingroup\$

HTML

In HTML, the property values should always be encased in ""s, even if the type is not necessarily intended to be a string. If needed, the JavaScript can parse the value into an actual number type.

In your HTML, you did not put ""s around the id. You should.

<button style="height:150px;width:150px" id="7" onclick="...

Inline styling

Inline styling in bad practice in HTML:

<button style="height:150px;width:150px"

In fact, there is another problem this: you are repeating the same style attribute for each and every button.

I recommend creating a CSS file and creating a rule for all buttons that sets these properties:

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

JavaScript

In your color_single_button, you are accessing whatever element has the id property of id two times in a row. The document.getElementById function is not very efficient, because it has to search the entire DOM. Your code would be a lot faster if you stored this value in a variable:

var button = document.getElementById(id);
var number = button.innerHTML;

...

In fact, let's take this one step further: let's store all the buttons in an array.

This is very easy to do with document.getElementsByTagName. This will return an array of all the elements that are of the specified tag:

var buttons = document.getElementsByTagName("button");

Then, instead of accessing the element by passing it's ID to getElementById, you use the ID - 1 as an indexer in this array to access an element.

This will significantly boost the efficiency of your code.

Note: I am not very good with HTML, but it may not be good practice to use the id in this case, but rather a custom data- property.


Questions

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

I would recommend moving the JavaScript to an external file. It will aid maintainability in the future.


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

Yes, you could, and there are two ways that I recommend

  • AngularJS

AngularJS can make generating multiple, slightly changing elements very easy using ng-repeat.

  • Vanilla JavaScript

In this method, you would use a for-loop and document.createElement to create a bunch of button elements and append them to the body of the code. I'm not sure if this is the best choice, though, as it is generally discouraged to generate HTML from JavaScript.

However, if you were to try this, it might look like this:

document.onload = {
    for(var i = 0; i < BUTTON_COUNT; i++) {
        var button = document.createElement("button");
        button.style = "...";
        button.id = i;
        button.onclick = "switch_with_neighbor(" + id + ")";
        document.body.appendChild(button);
    }
}

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

Yes, I think it is sensible: JavaScript's for/in loops are meant for looping through object constructs. However, I would not say:

var i in nears

Generally, in JavaScript, the singular version of the object being looped through is used as the indexing variable:

var near in nears

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

I don't really know if this has to do with practice, but it sure is weird: this object is the exact same thing every time.

It would make a lot more sense to put this outside of the function so it is loaded once and only once, rather than it being created and destroyed every time the function starts and stops.


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

Yes, there are a few spots where you are unnecessarily converting types.

nears = neightbors[parseInt(current, 10)];

In JavaScript, you can use a string to access the property of an object. For example, look at this:

var obj = {
    foo: "bar"
}

I could access it like this:

obj.foo

And:

obj["foo"]

You can apply this to your circumstance: current is already a string, so there is no point in calling parseInt.

var number = parseInt(document.getElementById(parseInt(id)).innerHTML);

The method document.getElementById is looking for a string parameter (but, of course, can take a number parameter). Therefore, you are just fine leaving out the parseInt(id)

\$\endgroup\$
  • \$\begingroup\$ Damn, I've got to stop writing my reviews in my editor. Your answer was 31 minutes ahead of mine, and I didn't see it until I posted. Sorry. It seems I've been repeating almost all of your points :-P \$\endgroup\$ – Flambino Sep 3 '15 at 23:28
  • \$\begingroup\$ @Flambino That's totally fine. You've provided many more very useful tips, too! \$\endgroup\$ – SirPython Sep 3 '15 at 23:37
  • \$\begingroup\$ This will significantly boost the efficiency of your code. but it will not matter. This game is user-interface driven, and feedback is instantaneous, I think that it does not need optimization \$\endgroup\$ – Caridorc Sep 4 '15 at 8:04
  • \$\begingroup\$ @SirPython you're right in that using a data-* attribute is more fitting here (or the value attribute on button). And you're right, Caridorc, it is a micro optimisation, however it does also better clarify your intent. \$\endgroup\$ – Dan Pantry Sep 4 '15 at 8:43
  • \$\begingroup\$ Angular is a dragons nest to a beginner. Tools like Knockout.js might be easier to grok and might suit this purpose much better than a full framework. \$\endgroup\$ – Seiyria Sep 4 '15 at 12:22
7
\$\begingroup\$

For a beginner, it isn't bad. There are some rough points to fix, but nothing serious!

Lets tackle by language:

HTML:

The first thing I've noticed is your doctype:

<!DOCTYPE html5>

That is an invalid doctype. Instead of html5, it should be just html, like this:

<!DOCTYPE html>

You have a closing <body>, but you forgot to open it right after your <head>. Watch out for that.


On your buttons, the id doesn't have quotes. ALL attributes must have quotes.


You should leave all elements on a pre-defined equal state. That is: turn them all off.


CSS:

Instead of a style attribute, try this stylesheet:

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

.off {
    background: #CC0000;
}

.on {
    background: #00FF00;
}

So much easier, right?

Instead of you re-writting the color code somewhere, and multiple instances of the size lost in the code... You have this little packed thingy which does it all for you, and you don't need to care about the button size and it's colors: just their class. And the changes affect all elements with the following tags/classes! 1 change, fixes many!

And later on, to change the class on Javascript, you just need to give a new value to the property className:

elem.className = 'on';

And this will receive the styles for the class .on.

Warning: An element can have multiple classes! Each class is separated by a space. For your code, you can trust this way since you aren't using more than 1 class. For other projects, with more classes, you can try the methods present here: https://stackoverflow.com/a/196038/2729937


Javascript + HTML:

Instead of having this:

<button style="height:150px;width:150px" id=7 onclick="switch_with_neightbors(7)">1</button>

Repeated over and over again, you could do this:

<button value="7">1</button>

The <button> tag has a value attribute, that can easily be accessed over Javascript.


Javascript:

First, I recommend building a grid with your buttons. This will speed-up everything else.

Here's a method I would use:

var BUTTONS = []; // global: will contain all buttons

// fetches all buttons, and convert the Nodelist into an Array
var tmp = Array.prototype.slice.call(document.getElementsByTagName('button'));

// global: calculates the size of the square, to allow for more sizes (3x3, 4x4, 5x5 ...)
var SIZE = Math.ceil(Math.sqrt(tmp.length));

// idea taken from https://stackoverflow.com/a/11318797/
while (tmp.length > 0) {
    BUTTONS.push(tmp.splice(0, SIZE));
}

tmp = null; // cleans the content

Your color_single_button() should be changed into this:

function color_single_button(button) {
    button.className = button.innerHTML == '1' ? 'on' : 'off';
}

Or:

function color_single_button(button) {
    var classes = ['off', 'on'];
    button.className = classes[button.innerHTML];
}

Basically, with the changes used on the CSS, you don't need to worry about what color is where or what.

All you need to know is that 1 is on and 0 is off.

And instead of passing an id, selecting it 3 times and doing what-not, you pass the button element to change the color and you're done!


I don't know if this is defined behaviour or not, but to win I only need to turn on or off all elements, and click on 5 to toggle.

That's pretty... easy... Instead, you could only toggle the ones above, bellow and to the sides. That should make some people mad.

Let's harden it up!

function switch_with_neightbors(current) {
    invert_text_and_colour(current);

    var value = current.value;
    var y = ((value - 1) % SIZE);
    var x = Math.floor((value - 1) / SIZE);

    if( x ) { // if we aren't all to the left
        invert_text_and_colour(BUTTONS[x - 1][y]);
    }
    if( x < SIZE - 1 ) { // if we aren't all to the right
        invert_text_and_colour(BUTTONS[x + 1][y]);
    }

    if( y ) { // if we aren't all to the top
        invert_text_and_colour(BUTTONS[x][y - 1]);
    }
    if( y < SIZE - 1 ) { // if we aren't all to the bottom
        invert_text_and_colour(BUTTONS[x][y + 1]);
    }
}

Now your game is hard as it can be!


And now, your randomize() function will look like this:

function randomize() {
    var size = SIZE;
    for (var i = 0; i < size; i++) {
        for(var j = 0; j < size; j++) {
            var state = Math.random().toFixed();
            var current = BUTTONS[i][j];
            if( current.innerHTML !== state ) {
                invert_text_and_colour(current);
            }
        }
    }
}

Basically, iterates over all buttons and change the status if it isn't equal to the randomized status.

You might be asking: Why Math.random().toFixed()?
The answer is: Because it is simpler and faster. Math.random() returns a floating-point number between 0 and 1 (excluding 1).
You can round it up when it is above or equal to 0.5, saving you some operations. The method .toFixed() does the rounding to 0 decimal digits. And also returns the number as a string, that then we can use the === (equivalent) and the !== (not equivalent) operators to check if it is the same value or not.


Your click handling is using bad practices. Instead of using an attribute, try this instead:

document.body.onclick = function(e) {
    if(e.target.tagName == 'BUTTON') {
        switch_with_neightbors(e.target);
    }
}

Basically: if the clicked element on the body is a button, pass it to switch_with_neightbors().


You could re-write your invert_text_and_colour() as:

function invert_text_and_colour(button) {
    button.innerHTML = (!parseInt(button.innerHTML, 10)) / 1;
    color_single_button(button);
}

Basically, it works like this:

  • convert the innerHTML to a number
  • use the ! (not) operator to invert the value (1 becomes false and 0 becomes true)
  • divide by 1 to convert it to a number (false becomes 0 and true becomes 1)
    • when a number is divided by 1, it returns itself
    • usually, it is written as +(!parseInt(button.innerHTML, 10))
  • sets the new innerHTML, automatically converted to a string
  • inverts the color

Easy, right?


Instead of this:

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

You can simply move your whole <script> to before the closing <body>.

This will have some advantages:

  1. You don't have to wait for the document to load
  2. You don't have to handle the load event
  3. Your Javascript won't block other downloads
  4. When the browser reaches here, you can be sure that the browser has, at least, the elements available on DOM

And that line of code will be transformed into:

randomize();

But the game will never stop as-is! We need to check a winner!

We check it by verifying if the HTML is 1:

function won() {
    var size = SIZE;
    for (var i = 0; i < size; i++) {
        for(var j = 0; j < size; j++) {
            if( BUTTONS[i][j].innerHTML !== '1' ) {
                return false;
            }
        }
    }
    return true;
}

And we handle it inside the function switch_with_neightbors().

If it wins, it can run a function like this:

function winner() {
    // creates a yes-no dialog
    if(confirm('You won!!!\r\nDo you want to play again?')) {
        randomize();
    } else {
        var size = SIZE;
        for (var i = 0; i < size; i++) {
            for(var j = 0; j < size; j++) {
                BUTTONS[i][j].disabled = true;
            }
        }
    }
}

My implementation:

var BUTTONS = []; // global: will contain all buttons

var CLASSES = ['off', 'on']; // global: classes for the elements

// fetches all buttons, and convert the Nodelist into an Array
var tmp = Array.prototype.slice.call(document.getElementsByTagName('button'));

// global: calculates the size of the square, to allow for more sizes (3x3, 4x4, 5x5 ...)
var SIZE = Math.ceil(Math.sqrt(tmp.length));

// idea taken from https://stackoverflow.com/a/11318797/
while (tmp.length > 0) {
	BUTTONS.push(tmp.splice(0, SIZE));
}

tmp = null; // cleans the content

function color_single_button(button) {
	button.className = CLASSES[button.innerHTML];
}

function switch_with_neightbors(current) {
	invert_text_and_colour(current);
	
	if( won() ) {
		return winner();
	}
	
	var value = current.value;
	var y = ((value - 1) % SIZE);
	var x = Math.floor((value - 1) / SIZE);
	
	if( x ) { // if we aren't all to the left
		invert_text_and_colour(BUTTONS[x - 1][y]);
	}
	if( x < SIZE - 1) { // if we aren't all to the right
		invert_text_and_colour(BUTTONS[x + 1][y]);
	}
	
	if( y ) { // if we aren't all to the top
		invert_text_and_colour(BUTTONS[x][y - 1]);
	}
	if( y < SIZE - 1) { // if we aren't all to the bottom
		invert_text_and_colour(BUTTONS[x][y + 1]);
	}
	
	// yes, we need to verify twice
	if( won() ) {
		winner();
	}
}

function invert_text_and_colour(button) {
	button.innerHTML = (!parseInt(button.innerHTML, 10)) / 1;
	color_single_button(button);
}

function randomize() {
	var size = SIZE;
	for (var i = 0; i < size; i++) {
		for(var j = 0; j < size; j++) {
			var state = Math.random().toFixed();
			var current = BUTTONS[i][j];
			if( current.innerHTML !== state ) {
				invert_text_and_colour(current);
			}
		}
	}
}

function winner() {
	// creates a yes-no dialog
	if(confirm('You won!!!\r\nDo you want to play again?')) {
		randomize();
	} else {
		var size = SIZE;
		for (var i = 0; i < size; i++) {
			for(var j = 0; j < size; j++) {
				BUTTONS[i][j].disabled = true;
			}
		}
	}
}

function won() {
	var size = SIZE;
	for (var i = 0; i < size; i++) {
		for(var j = 0; j < size; j++) {
			if( BUTTONS[i][j].innerHTML !== '1' ) {
				return false;
			}
		}
	}
	return true;
}

document.body.onclick = function(e) {
	if(e.target.tagName == 'BUTTON') {
		switch_with_neightbors(e.target);
	}
}

randomize();
button {
    height:150px;
    width:150px;
}

.off {
    background: #CC0000;
}

.on {
    background: #00FF00;
}
<b>Clicking a button inverts it and its neightbours.
You win when you switch on all the lights.</b>


<p>
	<button value="1" class="off">0</button>
	<button value="2" class="off">0</button>
	<button value="3" class="off">0</button>
</p>

<p>
	<button value="4" class="off">0</button>
	<button value="5" class="off">0</button>
	<button value="6" class="off">0</button>
</p>

<p>
	<button value="7" class="off">0</button>
	<button value="8" class="off">0</button>
	<button value="9" class="off">0</button>
</p>

\$\endgroup\$
  • \$\begingroup\$ @SirPython I hope it is better now. Any suggestions to improve the text? \$\endgroup\$ – Ismael Miguel Sep 4 '15 at 0:46
  • \$\begingroup\$ @SirPython Done. Sorry the slowlyness. IE crawls on itself when editing something with a snippet. \$\endgroup\$ – Ismael Miguel Sep 4 '15 at 0:53
  • \$\begingroup\$ @SirPython Thank you! If you see something I forgot or misspelled, please warn me. \$\endgroup\$ – Ismael Miguel Sep 4 '15 at 0:58
  • \$\begingroup\$ I appreciate the effort, but I fear you are over-complicating many things.. \$\endgroup\$ – Caridorc Sep 4 '15 at 8:13
  • 1
    \$\begingroup\$ @Caridorc If you are curious in trying, here's a 5x5 game: jsfiddle.net/xc437q6w (notice that the only changes were in the HTML). And thank you for your vote. \$\endgroup\$ – Ismael Miguel Sep 4 '15 at 8:26

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.