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>

  • 2
    Is intended that I can already start clicking while the pattern is still being played? – skiwi Dec 16 '15 at 16:47
  • 1
    Wow, it's something entirely unconsidered. I realize now you can 'click-along' which does ruin the spirit of the game. Definitely something to amend in the next revision. Thank you for that. – Legato Dec 16 '15 at 16:55

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.

  • when you leave out the var keyword, it tries to find the variable on a higher level of layering, and eventually goes up and up and up and becomes a global at runtime level. – Quill Dec 16 '15 at 17:07

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.

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

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.
  • 2
    Would you provide a working example? I structured it recursively because a loop would not achieve what was desired, e.g. It would setInterval for them all, which destroys the intended effect and triggers them all rather simultaneously. I attempted a work-around by multiplying the interval period by the iterator but that didn't work either so I came up with this. I do recognize the potential for overflow though so I am very interested in an alternative. – Legato Dec 16 '15 at 17:48
  • @Legato -- really? you don't know how to make a loop here? Post the loop code that did not work -- I disbelieve. – Hogan Dec 16 '15 at 18:15
  • 2
    @Hogan -- I'm not seeing how you'd use a loop here either. setTimeout takes a function as input. How would you, within your loop, pass a function to setTimeout that runs the rest of your loop? – CardboardBox Dec 16 '15 at 22:25
  • Recursion is necessary, because of the asynchronous nature of making each button flash for one second. A loop would make all the buttons in the sequence flash simultaneously. – Greg Burghardt Dec 16 '15 at 23:38
  • 2
    @CardboardBox - Now that I think about it, If setInterval is used I'm not seeing how a loop or recursion is needed. setInterval will keep calling every interval unless told to stop. – Hogan Dec 17 '15 at 0:12

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.