Simon Says: is this too complex?

Question

I took a class covering HTML, CSS, Javascript and for a final project, thinking of a previous community challenge, I created Simon Says.

Going through my code my professor deemed it overly complicated (or did he mean complex?), explicitly referring to my showSequence method in particular.

I'm just wondering what and how could I have simplified things?

function SimonButton(id, litColor, color) {
    this.color = color;
    this.id = id;
    this.litColor = litColor;
    this.highlight = function() {
        document.getElementById(id).style.background = litColor;
        setTimeout(function() {
            document.getElementById(id).style.background = color;
        }, 750);
    }
}

var buttons = [
    new SimonButton("alpha", "red", "darkred"),
    new SimonButton("beta", "yellow", "darkorange"),
    new SimonButton("gamma", "lightblue", "darkblue"),
    new SimonButton("delta", "lightgreen", "darkgreen")
];

var sequence = [];
var round = 0;
var sequenceIterator = 0;
var userTurn = false;
playing = false;

window.onload = function() {
    alert("Try to reproduce the sequence shown.\nClick start to begin!");

    document.getElementById("newGame").onclick = function() {
        document.getElementById("newGame").value = "New Game";
        reset();
        playing = true;
        computerTurn();
    }

    for (var i = 0; i < buttons.length; i++) {
        document.getElementById(buttons[i].id).onclick = function() {
            if (playing) {
                if (!userTurn) {
                    alert("Wait for the sequence to end before trying to reproduce.");
                } else {
                    if (buttons[sequence[sequenceIterator++]].id != this.id) {
                        gameOver();
                    }
                    if (sequenceIterator === sequence.length && playing) {
                        sequenceIterator = 0;
                        document.getElementById("counter").innerHTML = ++round;
                        computerTurn();
                    }
                }
            } else {
                alert("Click upper left button to start a New Game.");
            }
        }
    }
}

function computerTurn() {
    userTurn = false;
    extendSequence(1);
    showSequence();
}

function reset() {
    sequence = [];
    round = 0;
    sequenceIterator = 0;
    document.getElementById("counter").innerHTML = round;
}

function gameOver() {
    playing = false;
    alert("Game Over.\nClick New Game to restart.");
}

/* Adds number items to the sequence.
*   e.g. if the sequence is 1, 3, 2,
*   calling extendSequence(2) 
*   could result in a sequence 1, 3, 2, 0, 2
*/
function extendSequence(number) {
    for (i = 0; i < number; i++) {
       sequence.push(Math.floor(Math.random() * 4)); 
    }   
}

function showSequence() {
    var i = 0;
    sequenceLoop();

    function sequenceLoop() {
        setTimeout(function() {
            buttons[sequence[i++]].highlight();
            if (i < sequence.length) {
                sequenceLoop();
            } else {
                userTurn = true;
            }
        }, 1000);
    }
}

body {
	background-color: #272822;
}

input:focus {
	outline: none;
}

input#newGame {
	height: 3em;
	width: 7em;
	font-size: 15px;
	font-family: Arial;
	font-weight: bold;
	border-radius: 6px;
	text-decoration: none;
	background-color: #A6E22E;
}
input#newGame:hover {
	color: #fff;
}

div#counter {
	position: relative;
	font-weight: bold;
	font-size: 30px;
	font-family: Arial;
	color: #fff;
	left: 9em;
}

.simonButton {
	height: 20em;
	width: 20em;
	margin: .5em;
}
.simonButton:hover {
	box-shadow: 0 0 .5em #fff;
}

input#alpha {
	background-color: darkred;
	border-radius: 11em 2em 5em 2em;
}

input#alpha:active {
	background-color: red !important;
}

input#beta {
	background-color: darkorange;
	border-radius: 2em 11em 0em 5em;
}
input#beta:active {
	background-color: yellow !important;
}

input#gamma {
	background: darkblue;
	border-radius: 2em 5em 0em 11em;
}
input#gamma:active {
	background-color: lightblue !important;
}

input#delta {
	background-color: darkgreen;
	border-radius: 5em 2em 11em 0em;
}
input#delta:active {
	background-color: lightgreen !important;
}

<html>
	<head>
		<meta charset = "utf-8">
		<title>Simon Says</title>
		<script src="simon-script.js"></script>
		<link rel="stylesheet" type="text/css" href="simon-sheets.css" />
	</head>
	<body>
		<div>
			<input type='button' id='newGame' value='Start'/>
		</div>

		<div>
			<input type='button' id='alpha' class='simonButton'>
			<input type='button' id='beta'  class='simonButton'>
		</div>
		<div id='counter'>0</div>
		<div>
			<input type='button' id='gamma' class='simonButton'>
			<input type='button' id='delta' class='simonButton'>
		</div>

	</body>
</html>

Answer 1

You are missing a doctype for your HTML. The doctype tells the browser which version of HTML to parse it as, and the latest doctype is <!DOCTYPE HTML>, capitalization not required.

---

HTML input elements of type button need a value to show the text on the button. As you do not want text on your buttons, you can create them with a single space, like:

<input type='button' id='alpha' class='simonButton' value=' '>

The HTML validator at the W3C has no other complaints.

---

Your CSS also passes the W3C's validator.

---

When you declare your JS fields, playing is not defined with var, but the others are:

playing = false;

Is this intentional, or no?

---

This statement can be confusing: buttons[sequence[sequenceIterator++]].id. I would recommend not incrementing an item in a statement that also retrieves a value.

Answer 2

First off, your code looks very pretty and clean and was an ease to read.

JavaScript

---

Get all the document elements!

No.

You are often calling the method document.getElementById. You should try to minimize these calls as much as you possibly can. Just imagine how slow this function must be, having to traverse the entire DOM for a single element.

One place where I see a this method being called a lot is in your SimonButton class. Here is your highlight method:

    document.getElementById(id).style.background = litColor;
    setTimeout(function() {
        document.getElementById(id).style.background = color;
    }, 750);

You are finding the element twice in these 4 lines. Then, later:

for (var i = 0; i < buttons.length; i++) {
    document.getElementById(buttons[i].id).onclick = function() {

scream. You could really cut back on these calls.

A very simple and easy fix is to find the correct DOM element as soon as the SimonButton is instantiated. Then, whenever you need to access it, just access the property:

function SimonButton(id, litColor, color) {
    this.color = color;
    this.id = id;
    this.litColor = litColor;
    this.element = document.getElementById(id);

I'm not sure if you even still need the id property.

---

A sequence of sequence

I see a lot of sequences in your code...

var sequence = [];   <---
var round = 0;
var sequenceIterator = 0 <---

...

function extendSequence(number) {
    ...
}

function showSequence() {
    ...
}

Hmm. If only there was some way for this code to be more organized.

Oh wait - there is!

You've already done some grouping of logic with your SimonButtons; why not do it for these sequence operations, too?

Now, it's up to you whether this Sequence is going to be an object or a class. I don't personally know which would be a better OOP practice. All you need to do is move the sequence related field/methods into a single class.

As an extra, you may want to add another method that is used when checking if the user's guesses are correct because you would be doing an awful amount of property modification on a Sequence in that section if not.

---

Stop alerting me!

alerts are annoying. Their ugly and they make your browser interact with them in order for them to go away. And, during that time, you are practically exempt from any other browser interaction.

I think your UI would really benefit from not using them. You could try creating your own custom pop-ups that show up in the center of the Simon game thing and maybe grow and then fade when they are done showing.

It's up to you; just don't use alerts.

Answer 3

Your code is nice, but there's a few things you can improve on, such as:

---

onclick:

You should be using eventListeners instead:

document.getElementById("newGame").addEventListener(function () {
    document.getElementById("newGame").value = "New Game";
    reset();
    playing = true;
    computerTurn();
});

and then instead of calling the same element inside the function, you can use this:

document.getElementById("newGame").value

into:

this.value

---

prototype chain:

Instead of using these unlinked functions inside a big script, connect them all as a prototype chain:

Effectively looking something like this:

function SimonButton(id, litColor, color) {
    //...
}
SimonButton.prototype.computerTurn = function(){
    //...
}
SimonButton.prototype.playerTurn = function(){
    //...
}

and then similarly to before, you can assign this values inside the prototype chain, and call the functions like this.computerTurn().

Additionally, this allows you to use a constructor, in case you want to build different game instances, like:

var gameOne = new SimonButton(id, litColor, color);
var gameTwo = new SimonButton(idTwo, litColorTwo, colorTwo);

Answer 4

Your professor is right this is overly complex. I see two reasons with just a quick glance at this function:

• There is no reason to use recursion -- just use a the functionality of setInterval
• There is no reason to call a function that you define on the next line that is only onle line long (sequenceLoop) -- replace the function call with the single line.