4
\$\begingroup\$

I have my HTML, CSS and JavaScript below and I would like to know as many bad things I did as possible.


window.onload = function () {
	// Fade in the body
	document.querySelector('body').style.opacity = 0;

	setTimeout(() => {
		document.querySelector('body').style.opacity = 1;
	});


	const body = document.querySelector('body'),
		tilesNumber = document.querySelector('#settings p span'),
		tilesInput = document.querySelector('#settings input'),
		startBtn = document.querySelector('#settings button[name=start]'),
		resetBtn = document.querySelector('#settings button[name=reset]'),
		playArea = document.querySelector('#play-area');


	tilesInput.addEventListener('input', () => {
		tilesNumber.innerHTML = tilesInput.value;
	});

	resetBtn.addEventListener('click', () => {
		startBtn.removeAttribute('disabled', '');
		tilesInput.removeAttribute('disabled', '');
		playArea.innerHTML = '';
	});

	startBtn.addEventListener('click', () => {
		startBtn.setAttribute('disabled', '');
		tilesInput.setAttribute('disabled', '');

		// Create tiles
		for (let i = 0; i < tilesInput.value; i++) {
			playArea.innerHTML += '<div class="tile"><p></p></div>';
		}

		// Push to `numsToAssign` all the numbers from 0 to `tilesInput / 2`
		var numsToAssign = [];

		for (let i = 0; i < tilesInput.value / 2; i++) {
			numsToAssign.push(i, i);
		}

		// Assign each number of `numsToAssign` to each tile randomly
		var randomNum = Math.floor(Math.random() * tilesInput.value);
		var randomTile = document.querySelectorAll('.tile p')[randomNum];

		for (let i = 0; i < tilesInput.value; i++) {
			do {
				if (!randomTile.innerHTML) {
					randomTile.innerHTML = numsToAssign.shift();
				}

				randomNum = Math.floor(Math.random() * tilesInput.value);
				randomTile = document.querySelectorAll('.tile p')[randomNum];
			} while (randomTile.innerHTML && numsToAssign.length > 0)
		}

		startGame();
	});


	function startGame() {
		let tiles = document.querySelectorAll('.tile');
		let firstClickedTile;
		let secondClickedTile;

		// Add click event for every tile
		for (let i = 0; i < tiles.length; i++) {
			tiles[i].addEventListener('click', matching);
		}

		function matching(e) {
			if (!firstClickedTile) {
				firstClickedTile = e.target.parentNode;
				firstClickedTile.firstChild.style.filter = 'blur(0px)';
			} else if (e.target.parentNode !== firstClickedTile) {
				secondClickedTile = e.target.parentNode;
				secondClickedTile.firstChild.style.filter = 'blur(0px)'

				/*
				If the tiles match:
				 - add 'matched' class to the matched tiles;
				 - remove the click event for the matched tiles;
				 - remove blur for the matched tiles;
				Else keep the blur.
				*/

				if (firstClickedTile.firstChild.innerText == secondClickedTile.firstChild.innerText) {
					firstClickedTile.classList.add('matched');
					secondClickedTile.classList.add('matched');

					firstClickedTile.removeEventListener('click', matching);
					secondClickedTile.removeEventListener('click', matching);

					firstClickedTile.firstChild.style.filter = 'blur(0px)';
					secondClickedTile.firstChild.style.filter = 'blur(0px)';

					tilesClickDelayAndWinCheck();
				} else {
					setTimeout(() => {
						firstClickedTile.firstChild.style.filter = 'blur(15px)';
						secondClickedTile.firstChild.style.filter = 'blur(15px)';
					}, 400);

					tilesClickDelayAndWinCheck();
				}

				setTimeout(() => {
					firstClickedTile = undefined;
				}, 400);
			}
		}

		// Removes click events on tiles to be able to see the second tile, then adds events back.
		function tilesClickDelayAndWinCheck() {
			let notMatchedTiles = 0;

			for (let i = 0; i < tiles.length; i++) {
				tiles[i].removeEventListener('click', matching);

				if (!tiles[i].classList.contains('matched')) {
					notMatchedTiles++;
				}
			}

			if (notMatchedTiles === 0) {
				console.log('You won!');
				winMenu();
				return;
			}

			setTimeout(() => {
				for (let i = 0; i < tiles.length; i++) {
					tiles[i].addEventListener('click', matching);
				}
			}, 400);
		}

		function winMenu() {
			body.innerHTML += '<div id="winScreen"></div>';
			document.querySelector('#winScreen').innerHTML = '<p>You won!</p>';

			setTimeout(() => {
				document.querySelector('#winScreen').style.background = 'rgba(0, 0, 0, .7)';
			}, 100);
			
			setTimeout(() => {
				body.removeChild(document.querySelector('#winScreen'));
			}, 2000);
		}
		
	}
}
body {
	margin: 0;
	background: rgb(15%, 15%, 15%);
	font-family: sans-serif;
	transition: .6s;
}

#winScreen {
	width: 100vw;
	height: 100vh;
	position: absolute;
	top: 0;
	transition: background .4s;
	overflow-y: hidden;
	display: flex;
	justify-content: center;
	align-items: center;
}

#winScreen p {
	color: white;
	font-size: 60px;
	transform: translateY(-70vh);
	user-select: none;
	text-shadow: 2px 2px 0 black;
	animation: textDrop 2s;
}

#intro, #game {
	width: 95%;
	text-align: center;
	margin: 10px auto 0 auto;
	border: 1px solid black;
	border-radius: 20px;
	background: linear-gradient(rgba(70%, 70%, 70%, .2), rgba(70%, 70%, 70%, .3));
	box-shadow: 2px 2px 1px gray;
}

#intro span {
	color: brown;
	text-shadow:
		1px 1px black,
		1px -1px black,
		-1px 1px black,
		-1px -1px black;
}

#game {
	min-height: 450px;
	margin-bottom: 10px;
}

#settings {
	font-size: 22px;
	background: rgba(0, 0, 0, .2);
	width: 300px;
	margin: 20px auto;
	padding: 10px;
	border-radius: 20px;
}

#settings button {
	width: 60px;
	height: 26px;
	cursor: pointer;
}

#play-area {
	display: flex;
	flex-wrap: wrap;
	justify-content: center;
}

.tile {
	height: 110px;
	width: 110px;
	margin: 20px;
	border: 1px solid;
	background: linear-gradient(rgba(255, 0, 0, .6), brown);
	cursor: pointer;
}

.tile p {
	font-size: 30px;
	margin: 0;
	padding: 35%;
	position: relative;
	top: 50%;
	transform: translateY(-50%);
	filter: blur(20px);
	user-select: none;
}

.matched {
	border-radius: 50%;
	transition: 1s;
	background: blue;
}

.matched p {
	filter: blur(0px);
}


@keyframes textDrop {
	0% {
		transform: translateY(-70vh);
	}
	10% {
		transform: translateY(-12vh);
	}
	90% {
		transform: translateY(12vh);
	}
	100% {
		transform: translateY(110vh);
	}
}
<!DOCTYPE html>
<html>

<head>
	<title>Memory Game</title>
	<link type="text/css" rel="stylesheet" href="style.css">
</head>

<body>
	<div id="container">
		<div id="intro">
			<h1>Welcome to <span>Memory Game</span>!</h1>
		</div>
		<div id="game">
			<div id="settings">
				<p>Number of tiles (<span>10</span>)</p>
				<input type="range" value="10" min="10" max="30" step="2">
				<button name="reset">Reset</button>
				<button name="start">Start</button>
			</div>
			
			<div id="play-area"></div>
		</div>
	</div>

	<script src="script.js"></script>
</body>

</html>

\$\endgroup\$
4
3
\$\begingroup\$

This code is already quite good, just some pointers:

  • You can use addEventListener for onload as well
  • You can put the Fade in code after your var, then you can re-use body instead of document.querySelector('body')
  • Your indentation is huge, I would advise 4 or even 2 spaces, give that a shot
  • it would probably be better to randomize numsToAssign and then do a simple loop to assign button[x] the value of numstoAssign[x]
  • Not a big fan of mixing let and var, if you are using var then you should that declare on the top of the block/scope to reflect what is happening from a hoisting perspective
  • Probably you could make a helper function that addresses 1 tile, and then call it twice

                firstClickedTile.classList.add('matched');
                secondClickedTile.classList.add('matched');
    
                firstClickedTile.removeEventListener('click', matching);
                secondClickedTile.removeEventListener('click', matching);
    
                firstClickedTile.firstChild.style.filter = 'blur(0px)';
                secondClickedTile.firstChild.style.filter = 'blur(0px)';
    
  • I still don't understand why you need to remove all the listeners in tilesClickDelayAndWinCheck

The bigger take away is that for the next level, you should read up on Model View Controller, and try to write your game using that approach.

Also, as an exercise, a player should be able to play this game more than once. Which means you will need to be a bit more careful with state (you cannot keep adding <div id="winScreen"></div> every time the player wins.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ "Your indentation is huge, I would advise 4 or even 2 spaces, give that a shot" - it seems OP is using tabs for indentation. So, it's not "huge", it's normal - one tab is one indentation level. You can set your editor to show tabs smaller or larger, as needed. Sort of related - the stack snippets show tabs as big as two spaces when editing but then makes them REALLY big when displaying the code. I would guess 8 spaces wide but I'm not sure. It's too much in any case. \$\endgroup\$ – VLAZ Feb 10 '19 at 15:54

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