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:
- You don't have to wait for the document to load
- You don't have to handle the load event
- Your Javascript won't block other downloads
- 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>
iarei - 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 idsx_y. The neighbors then are even easier to compute based on x and y. \$\endgroup\$