Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

After posting my first question and my first attempt which you can see here,

My first attempt at canvas spatial grid collision? Please be honest!

I have re-written it to try and implement what the person gave as an answer. I didn't do the prototype method as it was overkill for this and besides I am saving that for my next project!

Here is the re-written code:

$(document).ready(function() {

var canvas = $('canvas')[0]; // canvas object
var ctx = canvas.getContext('2d'); // canvas context
var engine = new engine(new grid(new block().getSize())); // run the game

window.addEventListener('keydown', keyPressDown, true); // event listener for keydown
function keyPressDown(evt) {
    if(engine.activeBlock) { // block is falling
        switch(evt.keyCode) {
            case 37: // left arrow
                engine.pressLeft();
            break;
            case 39: // right arrow
                engine.pressRight();
            break;
            case 40: // down arrow
                engine.activeBlock.pressDown();
            break;
        }
    }
}

window.addEventListener('keyup', keyPressUp, true); // event listener for keyup
function keyPressUp(evt) {
    if(engine.activeBlock) { // block is falling
        switch(evt.keyCode) {
            case 40:
                engine.activeBlock.releaseDown();
            break;
        }
    }
}

// block object
function block() {
    this.x = 500; // x coord
    this.y = 0; // y coord
    this.size = 50; // size of block squared
    this.getSize = function() { // returns size of the block
        return this.size;
    };
    this.baseVelocity = 1; // base speed
    this.velocity = this.baseVelocity; // current speed
    this.genColor = function() { // random block color
        var colors = ['yellow', 'red', 'blue', 'green', 'orange', 'fuchsia'];
        return colors[Math.floor(Math.random() * colors.length)];
    };
    this.color = this.genColor(); // blocks color
    this.update = function() { // update blocks position
        this.y += this.velocity;
    };
    this.pressDown = function() { // key press down
        this.velocity += 1;
    };
    this.releaseDown = function() { // key release down
        this.velocity = this.baseVelocity;
    };
    this.moveLeft = function() {
        this.x -= this.size;
    };
    this.moveRight = function() {
        this.x += this.size;
    };
}
// grid object
function grid(blockSize) {
    this.cellSize = blockSize; // cell size of grid
    this.cellsX = canvas.width / this.cellSize; // cells along x axis
    this.cellsY = canvas.height / this.cellSize; // cells along y axis
    this.buildGrid = function() { // returns built grid
        var arr = new Array(this.cellsX);
        for(var i = 0; i < arr.length; ++i) {
            arr[i] = new Array(this.cellsY);
        }
        return arr;
    };
    this.arr = this.buildGrid(); // holds grid
    this.getCoords = function(i) { // get relevant grid coords
        return Math.floor(i / this.cellSize);
    };
    this.checkDown = function(block) { // check grid for collisions down
        if(this.getCoords(block.y) == (this.cellsY - 1)) { // block has hit bottom of canvas
            return true;
        } else if(this.arr[this.getCoords(block.x)][this.getCoords(block.y) + 1]) { // block has landed on top of another block
            return true;
        }
    };
    this.checkLeft = function(block) { // check grid for collisions left
        if(this.getCoords(block.x) <= 0) { // block has hit left edge
            return true;
        } else if(this.arr[this.getCoords(block.x) - 1][this.getCoords(block.y)]) { // block is on left
            return true;
        } else if(this.arr[this.getCoords(block.x) - 1][this.getCoords(block.y) - 1]) { // block is on left
            return true;
        } else if(this.arr[this.getCoords(block.x) - 1][this.getCoords(block.y) + 1]) { // block is on left
            return true;
        }
        return false;
    };
    this.checkRight = function(block) { // check grid for collisions right
        if(this.getCoords(block.x) >= (this.cellsX - 1)) {
            return true;
        } else if(this.arr[this.getCoords(block.x) + 1][this.getCoords(block.y)]) {
            return true;
        } else if(this.arr[this.getCoords(block.x) + 1][this.getCoords(block.y) - 1]) {
            return true;
        } else if(this.arr[this.getCoords(block.x) + 1][this.getCoords(block.y) + 1]) {
            return true;
        }
        return false;
    };
    this.storeBlock = function(block) { // store block in grid
        block.x = this.cellSize * Math.floor(block.x / this.cellSize); // floor x coord to nearest cellSize multiple
        block.y = this.cellSize * Math.floor(block.y / this.cellSize); // floor y coord to nearest cellSize multiple
        this.arr[this.getCoords(block.x)][this.getCoords(block.y)] = block;
    };
}
// engine object
function engine(grid) {
    this.framerate = 20; // framerate
    this.activeBlock = new block(); // active block
    this.nextBlock = new block(); // next block
    this.drawBlock = function(block) { // draw block onto canvas
        ctx.fillStyle = block.color;
        ctx.fillRect(block.x, block.y, block.size, block.size);
        ctx.fillStyle = 'rgba(0, 0, 0, 0.2)';
        ctx.fillRect(block.x, block.y, block.size, block.size);
        ctx.fillStyle = block.color;
        ctx.fillRect(block.x + (block.size / 10), block.y + (block.size / 10), block.size - ((block.size / 10) * 2), block.size - ((block.size / 10) * 2));
    };
    this.drawGrid = function() { // draw existing blocks stored in grid
        for(var i = 0; i < grid.arr.length; ++i) {
            for(var i2 = 0; i2 < grid.arr[i].length; ++i2) {
                if(grid.arr[i][i2]) {
                    this.drawBlock(grid.arr[i][i2]);
                }
            }
        }
    };
    this.pressLeft = function() { // press left
        if(!grid.checkLeft(this.activeBlock)) { // check grid collision
            this.activeBlock.moveLeft(); // move left
        }
    };
    this.pressRight = function() { // press right
        if(!grid.checkRight(this.activeBlock)) { // check grid collision
            this.activeBlock.moveRight(); // move right
        }
    };
    this.resetCanvas = function() { // reset canvas background
        ctx.fillStyle = '#eee';
        ctx.fillRect(0, 0, canvas.width, canvas.height);
    };
    this.loop = function() { // the loop
        this.resetCanvas();
        this.drawGrid();
        this.drawBlock(this.activeBlock);
        this.activeBlock.update();
        if(grid.checkDown(this.activeBlock)) { // check grid collision down
            grid.storeBlock(this.activeBlock); // store block in grid
            this.activeBlock = this.nextBlock;
            this.nextBlock = new block();
        }
    };
    this.run = function() { // set interval
        setInterval(this.loop.bind(this), this.framerate); // bind the loop
    };
    this.run(); // run on instance
}
});

Are there any more improvements that I could change to this? Any best practices that I haven't done?

You can view what the code does here: http://jsfiddle.net/ShaShads/LM3PN/embedded/result/

Thanks

share|improve this question
add comment

1 Answer

up vote 3 down vote accepted

Just as a general point to consider, you're making everything public. Consider your Block

function block() {
    this.x = 500; // x coord
    this.y = 0; // y coord
    this.size = 50; // size of block squared
    this.getSize = function() { // returns size of the block
        return this.size;
    };
}

Do you want someone to be able to change the size of your block? It doesn't look like you do.

var b = new block();
b.size = -5 // I'm pretty sure you don't want this to be possible.

Consider only making things public if you want other parts of your script to be able to change it.

function Block() {
    this.x = 500;
    this.y = 0; 
    var size = 50; 
    this.getSize = function() { 
        return size;
    };
}

var b = new Block();
b.size // doesn't exist -> people have to use getSize function.

Thinking about it fully, you'd probably want your x and y coordinates to be private too - it should only be possible to update it via your update function. If you're interested, check out this page: http://javascript.crockford.com/private.html

share|improve this answer
    
Thanks my first game I coded like this was in java for android and was pretty basic, guess I didn't think about the differences coding in javascript which is available client side. –  ShaShads Mar 5 '13 at 20:37
    
@ShaShads it's a pretty good effort :). Another small point would be your keyPressUp function, there's no point in using a switch statement if you only care about one possible value. Also, you need to check evt.which as well as evt.keyCode as some browsers use one and not the other. –  RobH Mar 5 '13 at 20:42
    
Thanks for the info. I didn't even notice that about the keyPressUp function, I can guarantee that I copy/pasted it in a hurry to see if it worked from the keyDown functions!! Thanks buddy you have been a great help :) –  ShaShads Mar 5 '13 at 20:45
    
@ShaShads looks like I was getting confused with jQuery about the event.which: stackoverflow.com/a/4471691/1402923. No problem, have fun! –  RobH Mar 5 '13 at 20:51
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.