4
\$\begingroup\$

I wrote a simple breakout game in javascript. I was wondering if anyone could give me some advice on improving my code. I wrote somethings a bit oddly so I am more then welcome to criticism (constructive would be preferred however.) 

let canvas = document.getElementById("mainCanv");
let ctx = canvas.getContext("2d");
let brickLength = 14;
let score = 0;
let paddle = {
	w:80,
	h:15,
	x:180,
	y:canvas.height-40,		
}
let ball = {
	w:20,
	h:20,
	x:200,
	y:200,	
	speedY:Math.sin(0.6),
	speedX:Math.cos(0.6),
	speed:5	
}

let keys = {
	Left:false,
	Right:false
}


let bricks = initBricks(brickLength);


function update(){
	//Draw everything
	ctx.fillStyle = 'black'
	ctx.fillRect(0,0,canvas.width, canvas.height)
	ctx.fillStyle = "green"
	ctx.fillRect(paddle.x, paddle.y, paddle.w, paddle.h)
	ctx.fillStyle = "red"
	ctx.fillRect(ball.x, ball.y, ball.w, ball.h)
	ctx.fillStyle = "white";
	ctx.fillText("Score: " + score, 20,20)
	for(let i = 0; i < brickLength; i++){
		if(bricks[i].showing){
			ctx.fillStyle = "blue";
			ctx.fillRect(bricks[i].x, bricks[i].y, bricks[i].w, bricks[i].h);
			if(ball.y<bricks[i].y+bricks[i].h && ball.y > bricks[i].y-bricks[i].h&& ball.x > bricks[i].x - bricks[i].w && ball.x < bricks[i].x +bricks[i].w){
				bricks[i].showing = false;
				ball.speedY *= 1;
				ball.speedX *= -1;
				score += 10;
			}
		}
	}

	//Do score calculations
	if(score % 13 == 0 && score >= 130){
		bricks = initBricks(brickLength);
		ball.speed += 2;
	}

	//Realtime movement
	if(keys.Right) {
		paddleMove(-5);
	}
	if(keys.Left) {
		paddleMove(5);
	}

	ballMove(ball, 5, 5)

	//Check out of bounds
	if(ball.y>480 || ball.y > paddle.y){
		ball.y = canvas.height/2
		ball.x = canvas.width/2
		ball.speed = 5;	
		score = 0;
		bricks = initBricks(brickLength);
	}
}

//Key checking
window.addEventListener("keydown", function(event){
	switch(event.keyCode){
		case 37:
			keys.Right = true;
			keys.Left = false;
			break;
		case 39:
			keys.Right = false;
			keys.Left = true;
			break;
	}

})
window.addEventListener("keyup", function(event){
	if(event.keyCode == 37 || event.keyCode == 39){
		keys.Right = false;
		keys.Left = false;
	}
})


//Movement functions
function paddleMove(dir){
	paddle.x += dir;
	if(paddle.x<0||paddle.x>canvas.width-paddle.w){
		paddle.x -= dir;
	}
}

function ballMove(ball){

	ball.x+=ball.speedX*ball.speed;
	ball.y+=ball.speedY*ball.speed;

	if(ball.y<0 || (ball.y > paddle.y-paddle.h && ball.x < paddle.x+paddle.w && ball.x > paddle.x-paddle.w)){
		ball.speedY *= -1;
	}

	if(ball.x<0||ball.x>480 ){
		ball.speedX *= -1;
	}
}

//Initialise bricks
function initBricks(length){
	let bricks = [length];
	for(let i = 0; i < length; i++){
		bricks.push({
			x:0+i*40,
			y:60,
			w:20,
			h:20,
			showing:true
		})
	}
	return bricks
}

setInterval(update, 1000/60);
<!DOCTYPE html>
<html>
<head>
	<title>Breakout</title>
</head>
<body>
<canvas id="mainCanv" width="500" height="500" style="background-color:black"></canvas>
<script type="text/javascript" src="game.js"></script>
</body>
</html>

\$\endgroup\$

1 Answer 1

Reset to default
5
\$\begingroup\$

Code review

Not bad, code is nice and clean, and naming is good.

Game play has some logic flaws in the way you handle the keyboard and collisions.

Some code style points.

  • Use constants for variables that do not change.

  • Learn to add semicolon to the end of lines. Don't rely on automatic semicolon insertion as that has some traps.

  • Separate operators with spaces to make the codes easier to read. EG if(ball.x<0||ball.x>480 ){ should be if (ball.x < 0 || ball.x > 480) {

  • KeyboardEvent.keyCode is a depreciated property and could be removed at any time. Use KeyboardEvent.key or KeyboardEvent.code

  • Never use setInterval to time rendering as it is not synced to the display refresh. Use requestAnimationFrame instead.

  • Add "use strict"; as the first line of the JavaScript to tell JavaScript to run in a stricter and more performant mode.

  • You have let bricks = [length]; this does not create an array of length, it creates an array with the first item length. To set an array's length use the length property bricks.length = length;

  • window is the default object, there is really no reason for you to reference it. You don't use it for window.document, window.setInterval, or any of the variables you declare in global scope so why use it for window.addEventListener

Code design and logic

See the rewrite for more details regarding the points below.

  • The update function has taken on too much responsibility. Try to limit what a function does to one thing only.

  • Keyboard logic is incorrect. You assume only one control key to be down at any one time. Players will sometimes hold one direction and tap the other. Your code does not allow this giving the illusion that the first key down no longer works. It is better to just hold the key state of each key.

  • Your paddle/ball collision has some logic flaws. A general rule of game collisions is to NEVER allow the game state to enter an impossible state. In this case the ball and paddle overlapping.

    As you have the code the ball will on occasion get stuck to the paddle.

    When you do a collision remember that it happens over a period of time between the previous and current frame. You need to work out when the ball hit during that frame and where it will be at the start of the next frame (usually some distance from the collision as it is already moving away)

  • Objects can also hold functions. It is easier to manage the various parts of a game if you encapsulate functionality into the game objects as well. In the rewrite you will see the main object, ball, paddle and bricks all contain the functions they need to draw, update, init/reset and various other functions as needed.

  • You can inherit properties of existing types using Object.assign, or the spread operator ... allowing you to setup properties and functions that are the same for many different objects. The bricks are an Array with properties added to draw, etc. Each array item is a brick with its own properties and functions.

  • Rather than have a semaphore to indicate if a brick is showing or not you can remove the brick from the array and that will save you iterating many bricks that are not showing.

  • Having many magic numbers spread in your code you can create a settings object to hold everything in one place. This makes it much easier to change the game settings, saving the hassle of going over the code to find every instance of a setting. In the rewrite the object game has everything needed to set up the game.

  • The order of actions for a game is usually move everything , test collisions and adjust position of everything, then draw everything

The rewrite.

The rewrite is as an example only. There are many ways to organised the code for an app, each has it pros and cons. These will vary with the individual, with the best style being one that best suits your way of thinking. Don't let code dogma spoil the fun of writing a game.

The rewrite has added some things.

I needed to test if it was all working, and being lazy, playing the game to find problems was too much effort, so I added an autoplay that plays the game at many time normal speed so that I could make sure all was OK. Check the game.autoPlay to turn off.

I also added a few more bricks.

I have change all the collision logic to a more robust form that takes in account the object's travel during the previous frame. As there are 4 types of collision Ball / Paddle, Ball / Wall, Ball / Brick, and Paddle/Wall each somewhat different in needs, thus the code base has grown somewhat.

I was tempted to add ball hit deviation depending on where the ball hit the paddle, but left that out in the end.

"use strict";

requestAnimationFrame(mainLoop); // request first frame. This will only happen after all the code below has been evaluated

// I am using direct reference to the canvas by its ID
//const canvas = document.getElementById("mainCanv");
const ctx = canvas.getContext("2d");

var score = 0;

/*===========================================================================================
  all game constants and settings
  ============================================================================================*/
const game = {
    backgroundColor : "black",
    autoPlay : true,
    auto : {
        playSpeed : 10, // Number of autoplay frames per 60th second Note one extra normal frame is also rendered
        follow : 0.7, // how well auto play ball follows 1 is perfect, 0.8 will always hit, < 0.8 ocational miss to 0 wont move
    },
    paddle : {  // details for the paddle
        init : {  // this is added directly to the paddle object
            w : 80,
            h : 15,
            color : "green",
        },
        fromBottom : 40, // these are added programmatic 
        speed : 5,
    },
    ball : {
        init : { // this is added directly to the ball object
            w: 20,
            h: 20,
            x: 200,
            y: 200,	      
            color : "red"
        },
        dir : 0.6, // these are added programmatic 
        speed : 4,        
    },
    brick : {
        init : { // this is added directly to each brick object
            w : 20,
            h : 20,
            color : "blue",
        },
        spacingX : 40, // these are added programmatic 
        spacingY : 40,
        top : 60,
        rowCount : 14,
        count : 14 * 4,
        value : 10, // points its worth
    },
    score : { // where to put the score and its color
        x : 20,
        y : 20,
        color : "white",
    },
}
        



/*===========================================================================================
  helper Function 
  ============================================================================================*/
function doRectsOverlap(rectA, rectB){ // both rectA, and rectB must have properties x,y,w,h
    return ! (
        rectA.y > rectB.y + rectB.h || 
        rectA.y + rectA.h < rectB.y || 
        rectA.x + rectA.w < rectB.x || 
        rectA.x > rectB.x + rectB.w
    );
}
/*===========================================================================================
  Common object properties and functions
  ============================================================================================*/
const rectangle = {
    draw() {
        ctx.fillStyle = this.color;
        ctx.fillRect(this.x, this.y, this.w, this.h);
    }    
}

/*===========================================================================================
  The main animation loop
  ============================================================================================*/
function mainLoop() {
	ctx.fillStyle = game.backgroundColor;
	ctx.fillRect(0, 0, canvas.width, canvas.height)
    inPlay();
    requestAnimationFrame(mainLoop); // request next frame
}

/*===========================================================================================
  When in play
  ============================================================================================*/
function inPlay() {
    if (ball.outOfBounds) {
        reset(); // reset game when out of bounds
    }
    // auto play loops over main game draws everything but bricks
    // at a faded alpha level
    if (game.autoPlay) {
        ctx.globalAlpha = 1 / (game.auto.playSpeed /4);
        for (let i = 0; i < game.auto.playSpeed; i++) {
            ball.update();
            paddle.update();
            bricks.update();
            
            paddle.testBall(ball);
            paddle.draw();
            ball.draw();
            
        }
        ctx.globalAlpha = 1;
        if (keys.any) {
            game.autoPlay = false;
        }
    }
    // main render for normal play at alpha = 1;
    ball.update();
    paddle.update();
    bricks.update();
    
    paddle.testBall(ball);
    
    paddle.draw();
    ball.draw();
    bricks.draw();
    
    if (bricks.length === 0 && paddle.hitBall) { bricks.init() }
    paddle.hitBall = false;
    drawScore();
}

function reset(){
    ball.reset();
    bricks.init();
    paddle.reset();
    keys.clear();
    score = 0;
}

function drawScore(){ // does the score thing
	ctx.fillStyle = game.score.color;
	ctx.fillText("Score: " + score, game.score.x, game.score.y)    
}

/*===========================================================================================
  Keyboard state and event handler 
  ============================================================================================*/
const keys = {
    ArrowLeft : false,
    ArrowRight : false,
    any : false,
    event(event) {
        if (keys[event.code] !== undefined) {
            keys[event.code] = event.type === "keydown";
        }
        keys.any = event.type === "keydown" ? true : keys.any;
    },
    clear(){
        for(const key of Object.keys(keys)) {
            if (typeof keys[key] !== "function") { keys[key] = false }
        }
    }
};
addEventListener("keydown", keys.event);
addEventListener("keyup", keys.event);


/*===========================================================================================
  Brick related code 
  ============================================================================================*/
// Assign some functions to an array of bricks
const bricks = Object.assign([], { // The array to get some more properties 
    init() {
        this.length = 0; // zero length
        for (let i = 0; i < game.brick.count; i++) {
            this.push({
                ...rectangle,
                ...game.brick.init,
                x: (i % game.brick.rowCount) * game.brick.spacingX,
                y: game.brick.top + (i / game.brick.rowCount | 0)  * game.brick.spacingY,
            })
        }        
    }, 
    draw() {
        for (const brick of this) { brick.draw() }
    },
    update() {
        var i;
        for (i = 0; i < this.length; i ++) {
            const brick = this[i];
            if (ball.testBrick(brick)) { // check ball against brick remove brick if hit
                this.splice(i--, 1); // remove brick
                score += game.brick.value;
            }
        }
    }
});


/*===========================================================================================
  Ball related code 
  ============================================================================================*/
const ball = {
    ...rectangle,  // assign common
    ...game.ball.init,  // assign ball init values
    dx : 0, // common name for speed is delta so I use deltaX, and deltaY shortened to dx,dy
    dy : 0,    
    outOfBounds : true, // this forces reset at start of game
    reset() {
        Object.assign(this, 
            game.ball.init, {
                dx : Math.cos(game.ball.dir) * game.ball.speed, 
                dy : Math.sin(game.ball.dir) * game.ball.speed,    
                outOfBounds : false,
            }
         );
    },
    update() {
        this.x += this.dx;
        this.y += this.dy;
        if (this.x <= 0) { 
            this.x = -this.x; // the ball will have moved away from the wall the same distance it has moved into the wall
            // MUST MOVE AWAY +
            this.dx = Math.abs(this.dx); // Set the direction as always positive. If you do -this.dx it may stuff up when you have
                                         // the ball and paddle interacting at the same time
        } else if (this.x > canvas.width - this.w) {                               
            this.x = (canvas.width - this.w) - (this.x - (canvas.width - this.w));
            // MUST MOVE AWAY -
            this.dx = -Math.abs(this.dx);
        }
        if (this.y < 0) {
            this.y = -this.y;
            this.dy = Math.abs(this.dy);
        }
        if (this.y > canvas.height) { // ball out of bounds
            this.outOfBounds = true;
        }
        
    },
    checkbrickTopBot(brick){
        if (this.dy < 0) { // moving up
            this.y = (brick.y + brick.h) + ((brick.y + brick.h) - this.y);
            this.dy = -this.dy;
            return true;
        }
        // must be moving down
        this.y = brick.y - ((this.y + this.h) - brick.y) - this.h;
        this.dy = -this.dy;
        return true;

    },
    checkbrickLeftRight(brick){
        if (this.dx < 0) { // moving up
            this.x= (brick.x + brick.w) + ((brick.x + brick.w) - this.x);
            this.dx = -this.dx;
            return true;
        }
        // must be moving down
        this.x = brick.x - ((this.x + this.w) - brick.x) - this.w;
        this.dx = -this.dx;
        return true;
    },
    
    testBrick (brick) { // returns true if brick hit
        
        if (doRectsOverlap(this, brick)) {
            // use ball direction to check which side of brick to test
            // First normalize the speed vector
            var speed = Math.sqrt(this.dx * this.dx + this.dy * this.dy);
            var nx = this.dx / speed;
            var ny = this.dy / speed;
            var xDist,yDist;
            // now find the distance to the brick edges in terms of normalised spped
            if (this.dx < 0) {
                xDist = ((this.x - this.dx) - (brick.x + brick.w)) / nx;
            } else {
                xDist = ((this.x - this.dx + this.w) - brick.x) / nx;
            }
            if (this.dy < 0) {
                yDist = ((this.y - this.dy) - (brick.y + brick.h)) / ny;
            } else {
                yDist = ((this.y - this.dy + this.h) - brick.y) / ny;
                
            }
            // the edge that is hit first is the smallest dist
            if (xDist <= yDist) { // x edge first to hit
                return this.checkbrickLeftRight(brick);
            }
            return this.checkbrickTopBot(brick);
        }
        return false;
    },
    
};


/*===========================================================================================
  Paddle related code 
  ============================================================================================*/
const paddle = {
    ...rectangle,  // assign common
    ...game.paddle.init, // assign startup
    x: (canvas.width / 2 - game.paddle.init.w / 2) | 0,  // | 0 floors the result (same as Math.floor)
    y: canvas.height - game.paddle.fromBottom,		
    hitBall : true,
    leftEdge : 0,
    rightedge : canvas.width -  game.paddle.init.w,
    reset() {
        this.x =  (canvas.width / 2 - game.paddle.init.w / 2) | 0;  // | 0 floors the result (same as Math.floor)
        this.y = canvas.height - game.paddle.fromBottom;
    },
    update() {  
        if (game.autoPlay) {
            keys.ArrowRight = false;
            keys.ArrowLeft = false;
            if (Math.random() < game.auto.follow) {
                var x = ball.x + ((Math.random()- 0.5) * this.w * 2);
                if (ball.x > this.x + this.w * (3/4)) { keys.ArrowRight = true }
                if (ball.x < this.x + this.w * (1/4)) { keys.ArrowLeft = true }
            }
        }
        if (keys.ArrowRight) { this.x += game.paddle.speed }
        if (keys.ArrowLeft) { this.x -= game.paddle.speed }
        this.x = this.x < this.leftEdge ? this.leftEdge : this.x;
        this.x = this.x >= this.rightEdge ? this.rightEdge : this.x;
    },
    testBall(ball) {
        if (doRectsOverlap(this, ball)) {
            // we now know that the ball must be hitting the paddle.
            var cx = (ball.x + ball.w / 2); // get ball x center
            // check for side hits. If so ball goes away from edge
            if (cx < this.x) { // hits paddle left side
                ball.y = this.y - ((ball.y + ball.h) - this.y) - ball.h;
                ball.x = this.x - ball.w;
                ball.dy = -Math.abs(ball.dy); // Must be negative
                ball.dx = -Math.abs(ball.dx); // Must be negative
                this.hitBall = true;
                
            
            } else if( cx > this.x + this.w) { // hits paddle right side
                ball.y = this.y - ((ball.y + ball.h) - this.y) - ball.h;
                ball.x = this.x + this.w;
                ball.dy = -Math.abs(ball.dy); // Must be negative
                ball.dx = Math.abs(ball.dx); // Must be positive
                this.hitBall = true;
            
            } else { // must be hits bat top or have run over the ball too lat
                if (ball.y > this.y + this.h / 2) { // if ball is more than halfway through the bat too lat to rescue so let it continue on
                    return;
                }
                ball.y = this.y - ((ball.y + ball.h) - this.y) - ball.h;
                ball.dy = -Math.abs(ball.dy); // Must be negative
                this.hitBall = true;
            }
        }
            
    },
}
<!-- the id is used to reference the element canvas -->
<canvas id="canvas" width="540" height="400"></canvas>

\$\endgroup\$
1
  • \$\begingroup\$ Thanks! This review is really in depth and helpful. A few things that you mentioned like forgetting to add semicolons were just stupid mistakes on my part and things I usually do right (maybe I just didn't read through my code as in depth as I should.) Anyway thanks for the advice, I will definitely take everything you said into account. \$\endgroup\$ Commented May 15, 2018 at 3:08

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.