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>1 Answer 1
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 beif (ball.x < 0 || ball.x > 480) {KeyboardEvent.keyCodeis a depreciated property and could be removed at any time. UseKeyboardEvent.keyorKeyboardEvent.codeNever use
setIntervalto time rendering as it is not synced to the display refresh. UserequestAnimationFrameinstead.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 itemlength. To set an array's length use the length propertybricks.length = length;windowis the default object, there is really no reason for you to reference it. You don't use it forwindow.document,window.setInterval, or any of the variables you declare in global scope so why use it forwindow.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,paddleandbricksall contain the functions they need todraw,update,init/resetand 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. Thebricksare anArraywith 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
showingor 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
gamehas 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>-
\$\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\$Asher– Asher2018年05月15日 03:08:03 +00:00Commented May 15, 2018 at 3:08