Simple cube that moves around the container with keys

Question

I made a basic cube that can move around of container and jump too. It's basic but I want to know if I can use better methods, better formatting, or shorten the code.

This my HTML:

<div class="container">
    <div class="cube">
        <div class="box">
    </div>
</div>

and this is my Javascript code with a little jQuery:

var velocity = 2;
var acceleration = 0.002;
var gravity = 2;
var key_left = false;
var key_right = false;
var key_up = false;
window.addEventListener('keydown', handleKeyDown)
window.addEventListener('keyup', handleKeyUp)


function handleKeyDown(event) {
if (event.keyCode == 65) {
    key_left = true;
} else if (event.keyCode == 68) {
    key_right = true;
} else if (event.keyCode == 87) {
    key_up = true;
}
};

function handleKeyUp(event) {
if (event.keyCode == 65) {
    key_left = false;
    velocity = 2;
    acceleration = 0.002;
} else if (event.keyCode == 68) {
    key_right = false;
    velocity = 2;
    acceleration = 0.002;
} else if (event.keyCode == 87) {
    key_up = false;
}
};

setInterval(function () {
var move = parseFloat($(".box").css("marginRight"));
var moveLimt = (parseFloat($(".cube").css("width")) - 100)

if (key_left === true && move < moveLimt) {
    $(".box").css("marginRight", function () {
        move += (velocity += acceleration);
        return move.toString() + "px";
    });
} else if (key_right === true && move > 0) {
    $(".box").css("marginRight", function () {
        move -= (velocity += acceleration);
        return move.toString() + "px";
    });
}

var jump = parseFloat($(".cube").css("height"));

if (key_up === true) {
    $(".cube").css("height", function () {
        jump += (gravity);
        if (jump < 402) {
            return jump.toString() + "px";
        }
    });
} else if (key_up === false) {
    $(".cube").css("height", function () {
        jump -= (gravity);
        if (jump > 98) {
            return jump.toString() + "px";
        }
    });
}
});

Answer 1

Review and alternative approaches

Micheal Zelensky has already provided a great review (Though I do not agree with all his points, no two coders ever will)

This review is to focus more on what your code does and how it does it. And some points the other answer missed.

Style

• Be consistent in your naming. You use snake_case for some variable names and camelCase for others. The JavaScript convention is camelCase. Swapping styles means you will be forever needing to remember what style you used for each variable name and this is how bugs creep in.

• JavaScript also uses UPPER_CASE_SNAKE when defining constants, mostly to define the many magic numbers and values your code needs. However it is not for all constants.

• You have lots of magic numbers in your code. Many are repeated, so if you wish to change a value you would have to find each one. It is both tedious, prone to error, and disincentivizes your will to tune your app to be just right.

  Using a set of constants defined in one place in the code makes changing values easy, reduces errors, and give semantic meaning in situation where all you have is a number.

JavaScript

• window is the default object, it is also the global scope. You don't need to use it. thus window.addEventListener('keydown', handleKeyDown) is the same as addEventListener('keydown', handleKeyDown)

• JavaScript has 3 types of variables. var scoped to the function, let scoped to a block { /*everything between the curlies is a block*/ }, and const also block scoped but can not be changed, a constant. It is important to learn which to use and when.

• Avoid repeated DOM queries for the same object, DOM queries are slow and as the code gets more complicated they will become a major bottleneck if you miss use DOM queries. Use query the DOM for .box and .cube each time you need them, this is bad.

  Locate the elements at the start of the program and assign it to a variables to be used when needed. It is much quicker and makes the code more readable.

• Also on the queries you should not be using class names to identify individual elements. Many elements can have the same class names. Use the element's Id which must be unique for each element on the page to locate and get a reference to the element.

• The keyboardEvent properties keyCode and charCode are depreciated properties and may disappear at any time (don't hold your breath though). Use keyboardEvent.code and/or keyboardEvent.key as they replace the depreciated properties and define the characters as named strings.

  Thus if q is pressed keyboardEvent.code === "KeyQ" and keyboardEvent.key === "q" See MDN https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent for more information

  See example of how to read keyboard states.

• When animating any DOM content don't use setInterval or setTimeout. Thay have a long list of problems in terms of creating good quality animation.

  Use requestAnimationFrame as it is synced to the display hardware and will ensure the best possible frame rate 60fps and will always be in perfect sync with the display refresh. (see example code for how to use)

Objects

Use objects to encapsulate related data and behaviours. Your code is animating a box and you have a variety of variable that are all related to the box in some way. As your code grows so will the number of variables and things will start to get very messy.

The best way to handle this is to put all related variables in a single object so that you can access them easily and dont end up with clashing variable names.

Objects can also define behaviours via function calls. Objects can serve as a template to make copies, define one and then create 1000's

JQuery

You don't need it, and the more you use the less you learn how the browser does it. I added two helper functions to the example. query takes a standard query string and return the first matching element. bounds takes a query string, or empty string and an element and returns the elements bounds object. top, left, right, bottom, width and height as numbers

Example

The example is quick rewrite and offered as a set of suggestions you may wish to familiarize yourself with.

I only made a close approximation of what your code did. If you have question use the comment and feel free to ask.

requestAnimationFrame(animationLoop); // start the main animation loop
function animationLoop(){
    box.update();  
    box.render();
    requestAnimationFrame(animationLoop); // request the next frame
}

// Helper functions to query the DOM qStr is standard query string
const query = qStr => document.querySelector(qStr); 
const bounds = (qStr, el = query(qStr)) => el.getBoundingClientRect(); 



// Set up all the magic numbers in one place as constants. Add comments to describe what the values mean, units, and safe ranges.
const MOVE_VELOCITY = 2;      // in pixels per frame (@60fps 2 pixel per frame is 120 pixels per second 
                              // At 120 pixels per second you can cross a HD 1920 screen in 16 seconds.
const GROUND_FRICTION = 0.1;  // fraction of x speed lost to friction per fame 
                              // Must be less than 1 and greater than 0
const BOUNCE = 1 / 2;         // fraction of speed that box will bounce when it hits the ground.
const GRAVITY = 2;            // in pixels                          
const BOX_ACCELERATION = 0.5; // in pixels per frame per frame x directions
const JUMP_POWER = -60;       // as a speed in pixels per frame
const ON_GROUND_SPEED = 0.1;  // below this speed box is on ground. In pixels per frame

// Keys are named keyboardEvent.code values REF https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code
const keys = {  // maps keyboard keys to box object control states
    KeyA(state) { box.controls.left = state },
    KeyD(state) { box.controls.right = state },
    KeyW(state) { box.controls.up = state },
    ArrowUp(state) { box.controls.up = state },
    ArrowLeft(state) { box.controls.left = state },
    ArrowRight(state) { box.controls.right = state },
}
function handleKeyEvents(event) {
    if(keys[event.code]) {
        keys[event.code](event.type === "keydown");  
        event.preventDefault(); 
    }
}
addEventListener('keydown', handleKeyEvents);
addEventListener('keyup', handleKeyEvents);
focus(); // get the keyboard events.


var boxBounds = bounds("#box");  
const box = {
    element : query("#box"),
    bounds : bounds("#cube"),    
    canJump : false, 
    x : boxBounds.left, 
    y : boxBounds.top,  
    width : boxBounds.width,
    height : boxBounds.height,
    dx : 0, // the box delta (AKA velocity)
    dy : 0,
    update() {
        if (box.controls.left) { box.dx -= BOX_ACCELERATION }
        if (box.controls.right) { box.dx += BOX_ACCELERATION }
        box.dy += GRAVITY; 
        if (box.controls.up && box.canJump) {
            box.dy -= JUMP_POWER;
            box.controls.up = false; 
        }
        box.x += box.dx;
        box.y += box.dy;
        if (box.x < box.bounds.left) {
            box.x = box.bounds.left;
            box.dx = 0;
        }
        if (box.x + box.width > box.bounds.right) {
            box.x = box.bounds.right - box.width;
            box.dx = 0;
        }
        if (box.y + box.height > box.bounds.bottom) {
            box.y = box.bounds.bottom - box.height;
            if (Math.abs(box.dy) < ON_GROUND_SPEED) {
                box.dy = 0; 
            } else {                
                box.dy = -Math.abs(box.dy) * BOUNCE; 
            }
            box.dx *= 1 - GROUND_FRICTION;
            box.canJump = true;
        } else {
            box.canJump = false;
        }
    },
    render() {
        box.element.style.top = box.y + "px";
        box.element.style.left = box.x + "px";
    },
    controls : {
        up : false,
        left : false,
        right : false,
    },
}
boxBounds = undefined; // no longer needed so dump it

#box {
   position : absolute;
   top : 40px;
   left : 40px;
   width : 60px;
   height : 60px;
   border: 4px solid #5D4;
   border-radius: 5px;
   background : #485;
}

#cube {
    position : absolute;
   top : 10px;
   left : 10px;
   bottom : 10px;
   right : 10px;
   border: 2px solid black;
}

<code>...Move A, W, D, or Up, Left, Right, arrows.</code>
<div id="cube"></div>
<div id="box"></div>

Answer 2

It looks not bad at all. However, it could look more professional, and to my not very picky eye there seem to be errors.

HTML

• There are no html-head-body tags, but every HTML document should have them. Nothing bad will happen, as modern browsers are really smart and will render your code correctly. These tags can be omitted by the HTML standards (e.g. WHATWG), but it is sort of a good style to have them always included. It will help whoever reads your code understand, that it is a document, not a part or template to be included somewhere else (if you didn't mean exactly this, of course).
• The third DIV element is not closed, this is an error that will cause your HTML render not correctly. Use traditional <div class="box"></div> or one-tag self-closing syntax: <div class="box"/>

Javascript

At the first glance, it is nice, but it doesn't look like Javascript. To look like Javascript, it should follow, or at least visually be close to one of coding conventions. Just a couple of things to be mentioned, that come to my mind:

• No header. A header really helps you (some time later), or another person, to understand what it is, what it does, who and when wrote it, what language it is written in, and many other things you may want to let the future generations know. This is just a /**/-comment block on the top of the script.

• Var-group. There are two main trends with defining the variables in JS - in the header / top of the script / block / function, or right before the place they are used. I personally prefer the first approach as more traditional, and besides that you assign initial values in your script, which makes it logic to do it as you did it. Var block can be grouped under the same var expression for better readability and more laconic and easier to transfer code. E.g.

  var velocity = 2, acceleration = 0.002, //... key_up = false;

• Separate the next logical paragraph (add listeners) with a blank line, don't keep it in the same paragraph with the "var" paragraph;

• Function body is not indented well. The indentation should start with the first line of the function body. The rest is fine. E.g.

  function example() { //first line indented //block opens: { //block indented //} }

• Good style is to put a block comment descriptor in front of every function to describe it.

• Use strict comparison, instead of type-independent (e.g. a === b instead of a == b). This will help you avoid some issues later.
• setInterval is missing the second argument - interval.

• Defining function for a callback argument in the function call expression is not really a good practice, as it decreases readability and mantainability of your code and may lead to a "Callback hell", so I would recommend first define a named function and then specify it is an argument for the setInterval method. E.g.

  function example() { //function body } setInterval(example, 1000);

And, in case if there are things that you don't know and which I mentined, but didn't reference here, you know how to search the web, right? =) Reading about coding conventions and a couple of articles about the good JS coding style may defenetely help.