3
\$\begingroup\$

I am in the middle of working on this game and I still need to add some things (e.g. score, coin counters, etc.) but I really feel like there is way too much unneeded code. Could you help me simplify this so I can keep adding things?

<!DOCTYPE html>
<html>
<head>
<title>Game</title>
<style>
html, body {
background: linear-gradient(to bottom,#7EC0EE,#C1E5FF);
height: 100%;
width: 100%;
margin: 0;
}
#player {
background: #AF1C1C;
position: fixed;
height: 20px;
bottom: 0px;
width: 8px;
z-index: 2;
left: 0px;
top: 0px;
}
#head {
background: #F2CE8C;
position: absolute;
height: 6px;
width: 8px;
left: 0px;
top: 0px;
}
#hair {
background: #000000;
position: absolute;
height: 2px;
width: 8px;
left: 0px;
top: 0px;
}
#hair-back {
background: #000000;
position: absolute;
height: 6px;
width: 2px;
left: 0px;
top: 0px;
}
#arm {
background: #F2CE8C;
position: absolute;
height: 5px;
width: 5px;
left: 0px;
top: 8px;
}
#feet {
background: #000000;
position: absolute;
bottom: 0px;
height: 4px;
width: 8px;
left: 0px;
}
#jetpack {
background: linear-gradient(to bottom,#FFF,transparent);
position: fixed;
height: 16px;
width: 8px;
opacity: 0;
z-index: 1;
}
#ground {
background: green;
position: fixed;
height: 10px;
width: 100%;
bottom: 0px;
z-index: 3;
left: 0px;
}
#platform {
background: #915921;
position: fixed;
height: 10px;
width: 15%;
z-index: 3;
}
.coin {
animation: coinflip 6s linear infinite;
transform: rotate(-90deg);
border: 2px inset #D69E10;
background: #EFB112;
position: fixed;
height: 8px;
width: 8px;
z-index: 0;
}
@keyframes coinflip {
0% {transform: rotate(-90deg);filter: brightness(100%);}
40% {transform: rotate(-90deg);filter: brightness(100%);}
50% {transform: rotate(-90deg) rotateX(180deg);filter: brightness(60%);}
60% {transform: rotate(-90deg);filter: brightness(100%);}
100% {transform: rotate(-90deg);filter: brightness(100%);}
}
</style>
</head>
<body onkeydown="getkey()" onkeyup="stopkey()">
<div id="player">
 <div id="head"></div>
 <div id="hair"></div>
 <div id="hair-back"></div>
 <div id="arm"></div>
 <div id="feet"></div>
</div>
<div id="jetpack"></div>
<div class="object" id="ground"></div>
<script>
var collision = false;
var player = document.getElementById("player");
var jetpack = document.getElementById("jetpack");
var ground = document.getElementById("ground");
var object = document.getElementsByClassName("object");
var platform = document.getElementsByClassName("platform");
var coin = document.getElementsByClassName("coin");
var up = 0;
var key = 0;
var counter = 0;
var addcounter = setInterval(createPlatform,1);
setInterval(collisionDetection,1);
setInterval(gravity,1);
setInterval(controller,1);
setInterval(followplayer,1);

function createPlatform() {
if (counter == 0) {
if (2 * Math.round((window.innerHeight / 15) / 2) < player.offsetHeight) {
var newPlatform = document.createElement("DIV");
newPlatform.setAttribute("class", "object platform");
newPlatform.setAttribute("id", "platform");
newPlatform.style.top = player.offsetHeight + 2 + "px";
newPlatform.style.left = "0px";
document.body.appendChild(newPlatform);
counter++;
}
else {
var newPlatform = document.createElement("DIV");
newPlatform.setAttribute("class", "object platform");
newPlatform.setAttribute("id", "platform");
newPlatform.style.top = 2 * Math.round((window.innerHeight / 15) / 2) + "px";
newPlatform.style.left = "0px";
document.body.appendChild(newPlatform);
counter++;
}
}
if (counter > 0 && counter < 5) {
var newPlatform = document.createElement("DIV");
newPlatform.setAttribute("class", "object platform");
newPlatform.setAttribute("id", "platform");
newPlatform.style.left = (window.innerWidth / 5) * counter + "px";
newPlatform.style.top = 2 * Math.round(((window.innerHeight / 5) * counter + 4) / 2) + "px";
document.body.appendChild(newPlatform);
for (var c = 0; c < 5; c++) {
var newCoin = document.createElement("DIV");
newCoin.setAttribute("class", "coin");
newCoin.style.top = 2 * Math.round((newPlatform.offsetTop - 18) / 2) + "px";
newCoin.style.left = newPlatform.offsetLeft + (((window.innerWidth / 31) + 4.5) * c) + "px";
document.body.appendChild(newCoin);
}
counter++;
}
if (counter > 5) {
var newPlatform = document.createElement("DIV");
newPlatform.setAttribute("class", "object platform");
newPlatform.setAttribute("id", "platform");
newPlatform.style.left = (window.innerWidth / 5) * counter + "px";
newPlatform.style.top = 2 * Math.round(((window.innerHeight / 5) * counter + 4) / 2) + "px";
document.body.appendChild(newPlatform);
for (var c = 0; c < 5; c++) {
var newCoin = document.createElement("DIV");
newCoin.setAttribute("class", "coin");
newCoin.style.top = newPlatform.offsetTop - 18 + "px";
newCoin.style.left = newPlatform.offsetLeft + (((window.innerWidth / 31) + 4.5) * c) + "px";
document.body.appendChild(newCoin);
}
clearInterval(addcounter);
}
}

function gravity() {
if (collision === false) {
player.style.top = player.offsetTop + 1 - up + "px";
}
}

function collisionDetection() {
for (i = 0; i < object.length; i++) { 
// Player Right to Object Left
if (player.offsetLeft + player.offsetWidth <= object[i].offsetLeft + 2 &&
    player.offsetLeft + player.offsetWidth >= object[i].offsetLeft &&
    player.offsetTop < object[i].offsetTop + object[i].offsetHeight &&
    player.offsetHeight + player.offsetTop > object[i].offsetTop) {
player.style.left = object[i].offsetLeft - player.offsetWidth + "px";  
collision = true; 
}
// Player Left to Object Right
if (player.offsetLeft >= object[i].offsetLeft + object[i].offsetWidth - 2 &&
    player.offsetLeft <= object[i].offsetLeft + object[i].offsetWidth &&
    player.offsetTop < object[i].offsetTop + object[i].offsetHeight &&
    player.offsetHeight + player.offsetTop > object[i].offsetTop) {
player.style.left = object[i].offsetLeft + object[i].offsetWidth + "px"; 
collision =  true;  
}
// Player Bottom to Object Top
if (player.offsetLeft < object[i].offsetLeft + object[i].offsetWidth &&
    player.offsetLeft + player.offsetWidth > object[i].offsetLeft &&
    player.offsetTop + player.offsetHeight >= object[i].offsetTop &&
    player.offsetTop + player.offsetHeight <= object[i].offsetTop + 2) {
player.style.top = object[i].offsetTop - player.offsetHeight + "px";   
collision = true;
}
// Player Top to Object Bottom
if (player.offsetLeft < object[i].offsetLeft + object[i].offsetWidth &&
    player.offsetLeft + player.offsetWidth > object[i].offsetLeft &&
    player.offsetTop <= object[i].offsetTop + object[i].offsetHeight &&
    player.offsetTop >= object[i].offsetTop + object[i].offsetHeight - 2) {
player.style.top = object[i].offsetTop + object[i].offsetHeight + "px";   
collision = true;
}
else {
collision = false;
}
}
}

function controller() {
// Left
if (key == 37) {
player.style.left = player.offsetLeft - 1 + "px";
up = 0;
jetpack.style.opacity = "0";
}
// Right
if (key == 39) {
player.style.left = player.offsetLeft + 1 + "px";
up = 0;
jetpack.style.opacity = "0";
}
// Up
if (key == 38) {
up = 2;
jetpack.style.opacity = "1";
}
else {
up = 0;
jetpack.style.opacity = "0";
}
}

function followplayer() {
jetpack.style.top = player.offsetTop + player.offsetHeight + "px";
jetpack.style.left = player.offsetLeft + "px";
}

function getkey() {
key = event.keyCode;
}

function stopkey() {
key = 0;
}
</script>
</body>
</html>

\$\endgroup\$
  • \$\begingroup\$ What happened to the indentation? Could you repaste the code? It's hard to read. \$\endgroup\$ – t3chb0t Jun 7 '18 at 6:25
3
\$\begingroup\$

Right off the bat, using multiple calls to setInterval is a bad idea. setInterval is already kind of unreliable and not very well optimized, plus using multiple intervals separately can easily lead to your update operations falling out of sync.

Try reading up on game loops and requestAnimationFrame. MDN has a pretty decent article, but you can also find plenty of materials online if you google it.

https://developer.mozilla.org/en-US/docs/Games/Anatomy

Basically, in the end you'll want to turn this:

setInterval(createPlatform,1);
setInterval(collisionDetection,1);
setInterval(gravity,1);
setInterval(controller,1);
setInterval(followplayer,1);

Into something like this:

let lastTick = performance.now();

function tick(now) {
    window.requestAnimationFrame(tick);

    // Get the elapsed time since the last frame in seconds
    let delta = (now - lastTick) / 1000;

    controller(delta);
    gravity(delta);
    followPlayer(delta);
    createPlatform(delta);
    collisionDetection(delta);

    lastTick = performance.now();
}

window.requestAnimationFrame(tick);
\$\endgroup\$
  • \$\begingroup\$ Sorry for being so late to respond but I've been away from my computer. I did this and it works but it is extremely slow, is there any way to stop this? \$\endgroup\$ – Josh Berger Jun 10 '18 at 4:32
  • \$\begingroup\$ You probably need to redesign your update logic somewhat. Using requestAnimationFrame will throttle your game to 60fps at most. The reason we calculate the delta value in the tick() function is so we know how much time has elapsed since the last frame, and you need to update the game world accordingly. For example, if you want your player to move at a speed of 100 pixels per second, you add 100 * delta to its position each frame. \$\endgroup\$ – Máté Safranka Jun 10 '18 at 8:29
  • \$\begingroup\$ Thanks for all the help. I ended up keeping mainly what I had originally but I called the gravity function and the collisionDetection function in another function that runs every millisecond because of your input. It works great so thanks again. \$\endgroup\$ – Josh Berger Jun 17 '18 at 15:55
2
\$\begingroup\$

Feedback

The code caches the DOM lookups (e.g. player, jetpack, etc.) - which is wise. Also, wow that is a nice rendering of a person and spinning coins using CSS.

Suggestions

The advice below is not an exhaustive list, but hopefully a start in the right direction.

id attributes are supposed to be unique

It appears that every time createPlatform() is called and it makes a platform element, it sets the id attribute to platform, but the value must be unique in the whole document1. You could utilize counter to create unique values for that attribute.

Indentation

As was mentioned in comments, the indentiation is lacking - both in the CSS and JavaScript. That is a really important in terms of readability. The amount to indent can vary depending on developer/team opninions, but generally 2 or 4 spaces are standard. Doug Crockford reccomends 4 spaces2

Don't Repeat Yourself

createPlatform() has a lot of repeated lines. The common lines within the if and else blocks can be moved outside:

function createPlatform() {
  if (counter == 0) {
    var newPlatform = document.createElement("DIV");
    newPlatform.setAttribute("class", "object platform");
    newPlatform.setAttribute("id", "platform");
    if (2 * Math.round((window.innerHeight / 15) / 2) < player.offsetHeight) {
      newPlatform.style.top = player.offsetHeight + 2 + "px";
    } else {
      newPlatform.style.top = 2 * Math.round((window.innerHeight / 15) / 2) + "px";
    }    
    newPlatform.style.left = "0px";
    document.body.appendChild(newPlatform);
    counter++;
  }

Perhaps it would be best to make a function for those first three lines of the element creation and attribute setting, which could also be used later on in other places.

Also, in collisionDetection there appears to be a lot of redundancy in the conditions:

if (player.offsetLeft + player.offsetWidth <= object[i].offsetLeft + 2 &&
    player.offsetLeft + player.offsetWidth >= object[i].offsetLeft &&
    player.offsetTop < object[i].offsetTop + object[i].offsetHeight &&
    player.offsetHeight + player.offsetTop > object[i].offsetTop) {
player.style.left = object[i].offsetLeft - player.offsetWidth + "px";  
collision = true; 
}
// Player Left to Object Right
if (player.offsetLeft >= object[i].offsetLeft + object[i].offsetWidth - 2 &&
    player.offsetLeft <= object[i].offsetLeft + object[i].offsetWidth &&
    player.offsetTop < object[i].offsetTop + object[i].offsetHeight &&
    player.offsetHeight + player.offsetTop > object[i].offsetTop) {
player.style.left = object[i].offsetLeft + object[i].offsetWidth + "px"; 
collision =  true;  
}
// Player Bottom to Object Top
if (player.offsetLeft < object[i].offsetLeft + object[i].offsetWidth &&
    player.offsetLeft + player.offsetWidth > object[i].offsetLeft &&
    player.offsetTop + player.offsetHeight >= object[i].offsetTop &&
    player.offsetTop + player.offsetHeight <= object[i].offsetTop + 2) {
player.style.top = object[i].offsetTop - player.offsetHeight + "px";   
collision = true;
}
// Player Top to Object Bottom
if (player.offsetLeft < object[i].offsetLeft + object[i].offsetWidth &&
    player.offsetLeft + player.offsetWidth > object[i].offsetLeft &&
    player.offsetTop <= object[i].offsetTop + object[i].offsetHeight &&
    player.offsetTop >= object[i].offsetTop + object[i].offsetHeight - 2) {
player.style.top = object[i].offsetTop + object[i].offsetHeight + "px";   
collision = true;
}

This could be cleaned up by setting collision to false at the start of the function and then abstracting the common lines:

collision = false; //set initially
for (i = 0; i < object.length; i++) {
    if (player.offsetTop < object[i].offsetTop + object[i].offsetHeight &&
      player.offsetHeight + player.offsetTop > object[i].offsetTop) {
        // Player Right to Object Left
        if (player.offsetLeft + player.offsetWidth <= object[i].offsetLeft + 2 &&
          player.offsetLeft + player.offsetWidth >= object[i].offsetLeft) {
            player.style.left = object[i].offsetLeft - player.offsetWidth + "px";
            collision = true;
        }
        // Player Left to Object Right
        if (player.offsetLeft >= object[i].offsetLeft + object[i].offsetWidth - 2 &&
          player.offsetLeft <= object[i].offsetLeft + object[i].offsetWidth) {
            player.style.left = object[i].offsetLeft + object[i].offsetWidth + "px";
            collision = true;
        }
    }
    if (player.offsetLeft < object[i].offsetLeft + object[i].offsetWidth &&
      player.offsetLeft + player.offsetWidth > object[i].offsetLeft) {
        // Player Bottom to Object Top
        if (player.offsetTop + player.offsetHeight >= object[i].offsetTop &&
          player.offsetTop + player.offsetHeight <= object[i].offsetTop + 2) {
            player.style.top = object[i].offsetTop - player.offsetHeight + "px";
            collision = true;
        }
        // Player Top to Object Bottom
        if (player.offsetTop <= object[i].offsetTop + object[i].offsetHeight &&
          player.offsetTop >= object[i].offsetTop + object[i].offsetHeight - 2) {
            player.style.top = object[i].offsetTop + object[i].offsetHeight + "px";
            collision = true;
        }
    }
}   

Minimize reflows from appending elements

It might be wise to consider using a DocumentFragment or at least a container element to add the platforms and coins to, and then add that element to the body to minimize reflows. For more information on this and similar topics, check out this article Stop Writing Slow Javascript. I know it is a few years old but still has some relevant information.


1https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id 2https://crockford.com/javascript/code.html#indentation

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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