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:

Canvas spatial grid collision

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!

$(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.

share|improve this question

1 Answer 1

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

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.