Can somebody review my Simon game JS core code please?

Can you give me some constructive criticism on this Simon game? App works fine, but with no design extras, only functionality.

So just to name a few questions: Too many/too few variables/functions? Too many/few loops? Some bad practices? Weak var names? Too many/few/unnecessary comments? Is it understadable for others? Unnecessary lines of code? What could be done here to make it more efficient? etc. etc…

Those functions were tricky, got to tell.

HTML:

<body>
	<div class="container">
		<div class="board-container">
			<div class="quarter" id="quarter1"></div>
			<div class="quarter" id="quarter2"></div>
			<div class="quarter" id="quarter3"></div>
			<div class="quarter" id="quarter4"></div>			
		</div>
	</div>
</body>

JS:

window.onload = function() {

	var original = []; //create table for adding new elements
	var playerGuess = []; //create table for adding player guesses
	var audio = { //create object with parameter names equal to div ids and values equal to proper sounds
		quarter1: new Audio('beep-1.wav'),
		quarter2: new Audio('beep-2.wav'),
		quarter3: new Audio('beep-3.wav'),
		quarter4: new Audio('beep-4.wav')
	};
	var guess = 0;	//create var what will be used for validation loop;

	nextRound(); //on window load execute nextRound() function

	function nextRound() { //beginning of the game
		var random = Math.floor(Math.random()*4 + 1); // random number (1-4)
		original.push("quarter" + random); //adding random element to the original table
		for (var i = 0; i < original.length; i++) { //loop for playing sounds from orignal table
			playSeries(i);		
		}
		click(); //go to click() function
	}

	function playSeries(i) { //executes playing sounds 
		setTimeout(function() {
			audio[original[i]].play();
		}, (i+1) + "000");
	}

	function click() { //made for handling click event
		for (var i=0; i<4; i++) {
			document.getElementsByClassName('quarter')[i].onclick = validation;
		} //after click on whichever of 4 divs execute validation() function
	}	

	function validation(){		
		playerGuess.push(this.getAttribute("id")); //add element (clicked div id) to playerGuess table
		audio[this.getAttribute("id")].play(); //play this divs sound
		if (playerGuess[guess] === original[guess]) { //check if player guess is correct
			guess++;
			if (guess === original.length) { //check if player guessed every element in original table
				playerGuess = []; //reset playerGuess table
				guess = 0; //reset guess var
				nextRound(); //go back to the beginning
			} else click(); //go back to click event handler
		} else {
			playerGuess = []; //reset playerGuess table
			guess = 0; //reset guess var
			click(); //go back to click event handler
		}
	}
};

I cleaned up your code.
You need to use triple backticks to post code to the forum.
See this post for details.

I’ll list what I would do as tedious as possible.

Change literal expression ‘4’ to audio.length because that’s what it actually stands for.

Apparently, playSeries() does not play series. It only plays the audio once per call. So I would put the loop associated with playSeries() within the function.

Change (i+1) + “000” to simple arithmetic because it’s inefficient; although, it won’t affect performance it’s wasteful operation.

Make reset function.

Change names: ‘original’, ‘validation’, ‘click’, because they don’t stand for themselves.

Assign ‘quarter’ as a constant variable so I can use that name consistently across the program.

Remove unnecessary, ambiguous, or rhetorical comments:
//create var what will be used for validation loop;
//on window load execute nextRound() function
//adding random element to the original table
//loop for playing sounds from orignal table
//go to click() function
//made for handling click event
//add element (clicked div id) to playerGuess table

1 Like

Thank you, gunhoo93, great advices