Simple JavaScript canvas game

Question

Here is a link to the code on JSFiddle.

This is my first attempt at playing with canvas. Before I move on doing anything else, it would be nice to have insight from somebody who knows canvas and JavaScript better than me.

Things I am looking for:

• Ways to optimize animation

• Ways to optimize the lazer drawing (I know I need to clear the lazers from the array every once in awhile when they are no longer within the drawing area, just haven't gotten around to it yet.)

• Ways to optimize the code in general and have good code re-use.

HTML:

<canvas id="world" style="height: 300px; width: 300px;" /> 

JavaScript:

console.log("Game starting..."); var ship = new Object(); ship.name = "Enterprise"; ship.x = 0; ship.y = 0; ship.width = 50; ship.left = false; ship.right = false; ship.up = false; ship.down = false; ship.fire = false; ship.firerate = 5; ship.cfirerate = 0; var lazers = new Array(); var world = document.getElementById('world'); var cxt = world.getContext("2d"); $(document).bind('keydown', function(e) { if(e.keyCode==37){ ship.left = true; } if(e.keyCode==38){ ship.up = true; } if(e.keyCode==39){ ship.right = true; } if(e.keyCode==40){ ship.down = true; } if(e.keyCode==90){ //Z console.log("pew pew"); ship.fire = true; } }); $(document).bind('keyup', function(e) { if(e.keyCode==37){ ship.left = false; } if(e.keyCode==38){ ship.up = false; } if(e.keyCode==39){ ship.right = false; } if(e.keyCode==40){ ship.down = false; } if(e.keyCode==90){ //Z ship.fire = false; } }); function createLazer(type) { if (type == 1) {//LEFT LAZER cxt.beginPath(); cxt.moveTo(125+ship.x,140+ship.y); cxt.lineTo(125+ship.x,130+ship.y); var l = new Object(); l.type = type; l.x = ship.x; l.y = ship.y; return l; } else if (type == 2) {//RIGHT LAZER cxt.beginPath(); cxt.moveTo(125+ship.x+ship.width,140+ship.y); cxt.lineTo(125+ship.x+ship.width,130+ship.y); var l = new Object(); l.type = type; l.x = ship.x; l.y = ship.y; return l; } } function drawWorld() { cxt.fillStyle="#808080"; cxt.fillRect(0,0,300,300); } function drawLazers() { for (x = 0; x < lazers.length; x++) { cxt.beginPath(); cxt.strokeStyle="#FF0000"; if (lazers[x].type == 1) { cxt.moveTo(125+lazers[x].x,140+lazers[x].y); cxt.lineTo(125+lazers[x].x,120+lazers[x].y); } else if (lazers[x].type == 2) { cxt.moveTo(125+lazers[x].x+ship.width,140+lazers[x].y); cxt.lineTo(125+lazers[x].x+ship.width,120+lazers[x].y); } cxt.stroke(); lazers[x].y = lazers[x].y - 6; //console.log("drawing lazer" + lazers[x].x + lazers[x].y); } } function drawShip() { if (ship.left) { ship.x = ship.x -5; } if (ship.right) { ship.x = ship.x +5; } if (ship.up) { ship.y = ship.y -5; } if (ship.down) { ship.y = ship.y +5; } if (ship.fire) { if (ship.cfirerate == 0) { lazers.push(createLazer(1)); lazers.push(createLazer(2)); ship.cfirerate = ship.firerate; } } if (ship.cfirerate != 0) { ship.cfirerate = ship.cfirerate - 1; } cxt.beginPath(); cxt.strokeStyle="#000000"; cxt.moveTo(125+ship.x,140+ship.y); cxt.lineTo(150+ship.x,120+ship.y); cxt.lineTo(175+ship.x,140+ship.y); cxt.stroke(); } function clear() { cxt.clearRect(0, 0, 300, 300); } function gameLoop() { drawWorld(); drawShip(); drawLazers(); } setInterval(function() { clear(); gameLoop(); }, 30); 

Answer 1

Cool program!

I have put my review of the code on JsFiddle.

A basic synopsis of what I thought to improve:

• Everything constant about the map, ship, lasers, and keycodes is all in one place to improve scalability.

• I used object literals and array literals instead of new Object() and new Array() because using them is shorter and and makes things easier to manipulate.

• The keydown and keyup event handlers were refactored to eliminate duplicate code.

• The createLaser and drawLasers methods were refactored. I removed some drawing code from createLaser because it didn't seem to do anything, and I removed calculations in drawLasers that were redundant with createLaser.

• I added code in drawLasers to remove lasers from the array that are no longer on the map. I also removed or rearranged drawing code that didn't do anything or was being called too many times. I removed the clear() function because it didn't seem to do anything.

• I changed statements of form x = x + y, x = x - y and x = x + 1 to x += y, x -= y, and x++, respectively.

• I changed one instance of the form array.push(x); array.push(y); to array.push(x,y);

• I renamed lazer to laser because I kept typing laser and it caused bugs that here hard to find. You can rename it back, if you are accustomed to typing lazer.

Here is a copy of the revised code:

console.log("Game starting..."); var ship = { name: "Enterprise", x: 125, y: 120, width: 50, height: 40, left: false, right: false, up: false, down: false, fire: false, firerate: 5, cfirerate: 0, moveInterval: 5, color: "#000000" }, map = { width: 300, height: 300, color: "#808080", drawInterval: 30 }, laser = { height: 20, moveInterval: 6, color: "#FF0000" }, lasers = [], keys = { left: 37, up: 38, right: 39, down: 40, fire: 90 //Z }, getKey = function(key) { for (var i in keys) { if (keys.hasOwnProperty(i)) { if (keys[i] === key) { return i }; } } }, eventValues = { keyup: false, keydown: true }, types = { right: 1, left: 2 }; var world = document.getElementById('world'); var cxt = world.getContext("2d"); $(document).bind('keydown keyup', function(e) { var key = getKey(e.keyCode); ship[key] = eventValues[e.type]; }); function createLaser(type) { var x = ship.x; if (type === types.right) { x += ship.width; } var y = laser.height + ship.y; return { type: type, x: x, y: y, } } function drawWorld() { cxt.fillStyle = map.color; cxt.fillRect(0, 0, map.width, map.height); } function drawLasers() { cxt.beginPath(); cxt.strokeStyle = laser.color; for (var i = 0; i < lasers.length; i++) { var lsr = lasers[i]; if (lsr.y < -laser.height) { lasers.splice(i, 1); continue; } cxt.moveTo(lsr.x, lsr.y); cxt.lineTo(lsr.x, lsr.y - laser.height); cxt.stroke(); lsr.y -= laser.moveInterval; } } function drawShip() { if (ship.left) { ship.x -= ship.moveInterval; } if (ship.right) { ship.x += ship.moveInterval; } if (ship.up) { ship.y -= ship.moveInterval; } if (ship.down) { ship.y += ship.moveInterval; } if (ship.fire) { if (ship.cfirerate === 0) { lasers.push(createLaser(types.left), createLaser(types.right)); ship.cfirerate = ship.firerate; } } if (ship.cfirerate !== 0) { ship.cfirerate--; } cxt.beginPath(); cxt.strokeStyle = ship.color; cxt.moveTo(ship.x, ship.y + (ship.height / 2)); cxt.lineTo(ship.x + (ship.width / 2), ship.y); cxt.lineTo(ship.x + ship.width, ship.y + (ship.height / 2)); cxt.stroke(); } function gameLoop() { drawWorld(); drawShip(); drawLasers(); } setInterval(gameLoop, map.drawInterval) 

If you see anything that you think is weird, or have a question about what I did, just ask me about it.

Answer 2

It's more personal preference than any hard requirement, but I always prefer object literal syntax to individual value assignments. That would turn your initial declarations into this:

var ship = { name: "Enterprise", x: 0, y: 0. width: 50, left: false, right: false, up: false, down: false, fire: false, firerate: 5, cfirerate: 0 }, lazers = []; 

Additionally, multiple variables can be declared with a single var statement if separated by commas, also as demonstrated.

You may also benefit from adding a boundary that prevents the Ship from moving off of the World. As it stands now you can lose it.