Skip to main content
Code Review

Return to Answer

Bounty Awarded with 100 reputation awarded by Caimen
added 193 characters in body
Source Link
Peter Olson
Peter Olson
  • 791
  • 4
  • 15

I have put my review of the code on JsFiddleJsFiddle.

console.log("Game starting...");

var ship = {
    name: "Enterprise",
    x: 125,
    y: 100120,
    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 < -20laser.height) {
            lasers.splice(i, 1);
            continue;
        }
        cxt.moveTo(lsr.x, lsr.y);
        cxt.lineTo(lsr.x, lsr.y - laser.height);
        cxt.stroke();
        lsr.y -= 6;laser.moveInterval;
    }
}

function drawShip() {
    if (ship.left) {
        ship.x -= 5;ship.moveInterval;
    }
    if (ship.right) {
        ship.x += 5;ship.moveInterval;
    }
    if (ship.up) {
        ship.y -= 5;ship.moveInterval;
    }
    if (ship.down) {
        ship.y += 5;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, 30map.drawInterval)

I have put my review of the code on JsFiddle.

console.log("Game starting...");

var ship = {
    name: "Enterprise",
    x: 125,
    y: 100,
    width: 50,
    height: 40,
    left: false,
    right: false,
    up: false,
    down: false,
    fire: false,
    firerate: 5,
    cfirerate: 0,
    color: "#000000"
},
    map = {
        width: 300,
        height: 300,
        color: "#808080"
    },
    laser = {
        height: 20,
        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 < -20) {
            lasers.splice(i, 1);
            continue;
        }
        cxt.moveTo(lsr.x, lsr.y);
        cxt.lineTo(lsr.x, lsr.y - laser.height);
        cxt.stroke();
        lsr.y -= 6;
    }
}

function drawShip() {
    if (ship.left) {
        ship.x -= 5;
    }
    if (ship.right) {
        ship.x += 5;
    }
    if (ship.up) {
        ship.y -= 5;
    }
    if (ship.down) {
        ship.y += 5;
    }
    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, 30)

I have put my review of the code on JsFiddle.

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)
Source Link
Peter Olson
Peter Olson
  • 791
  • 4
  • 15

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: 100,
    width: 50,
    height: 40,
    left: false,
    right: false,
    up: false,
    down: false,
    fire: false,
    firerate: 5,
    cfirerate: 0,
    color: "#000000"
},
    map = {
        width: 300,
        height: 300,
        color: "#808080"
    },
    laser = {
        height: 20,
        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 < -20) {
            lasers.splice(i, 1);
            continue;
        }
        cxt.moveTo(lsr.x, lsr.y);
        cxt.lineTo(lsr.x, lsr.y - laser.height);
        cxt.stroke();
        lsr.y -= 6;
    }
}

function drawShip() {
    if (ship.left) {
        ship.x -= 5;
    }
    if (ship.right) {
        ship.x += 5;
    }
    if (ship.up) {
        ship.y -= 5;
    }
    if (ship.down) {
        ship.y += 5;
    }
    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, 30)

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