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.

You can see a working example here. Be sure to click on the canvas or the key inputs will not be detected!

Are there any ways I could improve upon this? Anything that should have been done differently? Any best practices I have not implemented?

$(document).ready(function() {

    window.addEventListener('keydown', keyPress, true); // event listener for keyboard presses
    function keyPress(evt) {
        if(engine.activeBlock) { // block is actively falling
            switch(evt.keyCode) {
                case 37: // left arrow
                    engine.activeBlock.keyLeft();
                break;
                case 39: // right arrow
                    engine.activeBlock.keyRight();
                break;
                case 40: // down arrow
                    engine.activeBlock.keyDown();
                break;
            }
        }
    }

    // block object
    function block() {
        this.xStart = function() { // random x coordinate of starting position of block
            var x = Math.floor(Math.random() * (engine.cellsX - 1 + 1)) + 1; // random number between 1 and cellsX
            return (x - 1) * engine.cellSize;
        };
        this.x = this.xStart(); // x coordinate
        this.y = 0 - (engine.cellSize * 1.5); // y coordinate
        this.velocity = 1; // velocity of the block
        this.generateColor = function() { // generates a random color
            var colors = ['blue', 'red', 'green', 'black', 'orange']; // colors array
            return colors[Math.floor(Math.random() * colors.length)];
        };
        this.color = this.generateColor(); // color of the block
        this.keyLeft = function() { // key left
            if(!engine.collisionLeft()) { // check collisions left side
                this.x -= engine.cellSize;
            }
        };
        this.keyRight = function() { // key right
            if(!engine.collisionRight()) { // check collisions right side
                this.x += engine.cellSize;
            }
        };
        this.keyDown = function() { // key down
            if(this.y > 0) { // block is on screen
                this.velocity += 1; // increase block velocity
            }
        };
        this.update = function() { // updates the blocks position
            this.y += this.velocity; // increase velocity
            engine.collisionBelow(); // check collisions below
            ctx.fillStyle = this.color;
            ctx.fillRect(this.x, this.y, engine.cellSize, engine.cellSize);
        };
    }

    // engine object
    function engine() {
        this.cellSize = 100; // size of each cell
        this.cellsX = canvas.width / this.cellSize; // number of cells along the x axis
        this.cellsY = canvas.height / this.cellSize; // number of cells along the y axis
        this.totalCells = this.cellsX * this.cellsY; // total cells
        this.buildGrid = function() { // builds grid
            var arr = new Array(this.cellsX);
            for(var i = 0; i < arr.length; ++i) {
                arr[i] = new Array(this.cellsY);
            }
            return arr;
        };
        this.grid = this.buildGrid(); // holds grid
        this.activeBlock = false; // where a falling block is placed
        this.getGridCoords = function(i) { // x coordinate of where a block should be placed in grid
            return Math.floor(i / this.cellSize);
        };
        this.drawGrid = function() { // outputs the blocks stored in grid
            for(var i = 0; i < this.grid.length; ++i) {
                for(var i2 = 0; i2 < this.grid[i].length; ++i2) {
                    if(this.grid[i][i2]) { // grid cell is occupied
                        ctx.fillStyle = this.grid[i][i2].color;
                        ctx.fillRect(this.grid[i][i2].x, this.grid[i][i2].y, this.cellSize, this.cellSize);
                    }
                }
            }
        };
        this.collisionBelow = function() { // check collisions below
            if(this.activeBlock.y > (canvas.height - this.cellSize)) {
                this.insertBlock(); // insert block into grid
            } else if(this.grid[this.getGridCoords(this.activeBlock.x)][this.getGridCoords(this.activeBlock.y) + 1]) {
                this.insertBlock(); // insert block into grid
            }
        };
        this.collisionLeft = function() { // check collisions left
            if(this.activeBlock.x <= 0) {
                return true;
            } else if(this.grid[this.getGridCoords(this.activeBlock.x) - 1][this.getGridCoords(this.activeBlock.y)]) {
                return true;
            } else if(this.grid[this.getGridCoords(this.activeBlock.x) - 1][this.getGridCoords(this.activeBlock.y) - 1]) {
                return true;
            } else if(this.grid[this.getGridCoords(this.activeBlock.x) - 1][this.getGridCoords(this.activeBlock.y) + 1]) {
                return true;
            }
        };
        this.collisionRight = function() { // check collisions right
            if(this.activeBlock.x >= (canvas.width - this.cellSize)) {
                return true;
            } else if(this.grid[this.getGridCoords(this.activeBlock.x) + 1][this.getGridCoords(this.activeBlock.y)]) {
                return true;
            } else if(this.grid[this.getGridCoords(this.activeBlock.x) + 1][this.getGridCoords(this.activeBlock.y) - 1]) {
                return true;
            } else if(this.grid[this.getGridCoords(this.activeBlock.x) + 1][this.getGridCoords(this.activeBlock.y) + 1]) {
                return true;
            }
        };
        this.insertBlock = function() { // insert block into grid
            this.activeBlock.x = this.cellSize * Math.floor(this.activeBlock.x / this.cellSize); // floor x coordinate to cellSize multiple
            this.activeBlock.y = this.cellSize * Math.floor(this.activeBlock.y / this.cellSize); // floor y coordinate to cellSize multiple
            this.grid[this.getGridCoords(this.activeBlock.x)][this.getGridCoords(this.activeBlock.y)] = this.activeBlock;
            this.activeBlock = new block();
        };
    }

    var canvas = $('canvas')[0]; // holds the canvas
    var ctx = canvas.getContext('2d'); // holds the canvas context
    var engine = new engine(); // initiate the engine
    engine.activeBlock = new block(); // add block

    function loop() { // game loop
        ctx.fillStyle = '#eee';
        ctx.fillRect(0, 0, canvas.width, canvas.height);
        engine.activeBlock.update(); // update active block
        engine.drawGrid(); // draw the grid of blocks
    }

    // run loop
    setInterval(loop, 20);
});
share|improve this question
add comment

1 Answer 1

up vote 2 down vote accepted

Haven't looked at the code in detail, but here are some thoughts on the overall structure:

Right now, there's some mixing of concerns and tight coupling going on. The engine calls update on the active block, but the block then calls back to the engine to check for collisions. In other words, the two are tightly coupled; they're dependent on each other, and its unclear where certain responsibilities lie.

The block is a pretty simple object, which can be reduced to simply being an x, y position, and a velocity. And it should probably stop there. The engine would be responsible for moving the block (the block object can do the calculations, though), checking collisions, accepting keyboard input, etc.. I.e. keep the block "dumb"; it doesn't need to know its context.

I'd also suggest making a similarly "dumb" grid object that the engine object can interrogate and instruct. I.e. the grid doesn't move blocks around, it simply keeps track of them for the engine. The grid could check for collisions, but it'd be up to the engine to ask.

Where exactly to draw the boundaries between the objects and their responsibilities is up to you (there are many ways to break it down), but the idea is to keep things decoupled when possible. I.e. blocks don't rely on a grid or an engine being there; the grid may or may not care about blocks being blocks, just that there's something in a given cell, etc.. Perhaps there are more things, you can extract and/or abstract and encapsulate in objects or constructors, while the engine sits in the middle acting as controller.

On a purely syntactical level, I'd move the methods of into the prototypes for the objects. That is, from this

function block() {
  // instance variables ...
  this.update = function () { ... }
}

to this

function Block() {  // CamelCase since it's a constructor
  // instance variables ...
}

Block.prototype.update = function () {
  ...
};

This will make it easier to do prototypal inheritance, as the methods will actually be prototypal.

share|improve this answer
    
Thank you this has helped! This is my first ever game so I just tried to throw something together. I will try and improve on it. –  ShaShads Feb 25 '13 at 9:55
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.