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.