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.

This is the first time I've written JavaScript in this way (previously all I've done mostly is DOM manipulation and form validation in vanilla and jQuery).

I've begun working on a basic Pong game. It's currently unfinished, but before I continue, I'd like to get some feedback from experienced JS developers:

Full codebase is available here.

The main pong.js file is as so:

var PaddleObj = function() { };

PaddleObj.prototype.init = function(game_object, side) {
    this.game = game_object;
    this.width = 15;
    this.height = 100;

    if (side == 'left')
        this.position = {x:40, y:this.game.height/2};
    else
        this.position = {x:this.game.width-40, y:this.game.height/2};
}

PaddleObj.prototype.render = function(ctx) {
    ctx.fillStyle = "white";
    ctx.fillRect (this.position.x-this.width/2, this.position.y-this.height/2, this.width, this.height);
}

PaddleObj.prototype.setYPosition = function(y_pos) {
    this.position.y = y_pos;
    if ((this.position.y - this.height/2) < 0) 
        this.position.y = this.height/2;
    if ((this.position.y + this.height/2) > this.game.height)
        this.position.y = this.game.height - this.height/2;
}

PaddleObj.prototype.update = function(delta_time) {
    //don't do anything on update by default
}

PaddleObj.prototype.isBallColliding = function(ball) {
    return (ball.position.x < (this.position.x + this.width/2))
        &&  (ball.position.x > (this.position.x - this.width/2))
        &&  (ball.position.y < (this.position.y + this.height/2))
        &&  (ball.position.y > (this.position.y - this.height/2));
}

AIPaddleObj = function() { 
    this.speed = 100;
};

AIPaddleObj.prototype = PaddleObj.prototype;
AIPaddleObj.constructor = AIPaddleObj;

AIPaddleObj.prototype.update = function(delta_time) {
    if (this.game.ball.position.y < this.position.y) {
        this.setYPosition(this.position.y -= this.speed * delta_time);
    }
    if (this.game.ball.position.y > this.position.y) {
        this.setYPosition(this.position.y += this.speed * delta_time);
    }
}




var BallObj = function() { };

BallObj.prototype.init = function(game_object, initial_speed, pos_x, pos_y) {
    this.game = game_object;
    this.position = {x:pos_x, y:pos_y};
    this.vector = {x:initial_speed, y:initial_speed};
}

BallObj.prototype.update = function(frame_time) {
    var previous_position = this.position;

    this.position.x += this.vector.x*frame_time;
    this.position.y += this.vector.y*frame_time;
    if (this.position.x > this.game.width) {
        this.position.x = this.game.width;
        this.vector.x *= -1;
    }
    if (this.position.x < 0) {
        this.position.x = 0;
        this.vector.x *= -1;
    }
    if (this.position.y < 0) {
        this.position.y = 0;
        this.vector.y *= -1;
    }
    if (this.position.y > this.game.height) {
        this.position.y = this.game.height;
        this.vector.y *= -1;
    }

    if (this.game.aiPaddle.isBallColliding(this) || this.game.playerPaddle.isBallColliding(this)) {
        this.position = previous_position;
        this.vector.x *= -1;
    }
}

BallObj.prototype.render = function(ctx) {
    ctx.beginPath();
    ctx.fillStyle="white";
    ctx.arc(this.position.x, this.position.y, 10, 0 , 2 * Math.PI, false);
    ctx.fill();
    //ctx.stroke();
}





var GameObj = function() {
    this.gameCanvas = document.createElement('canvas');
}

GameObj.prototype.setCanvasSize = function(width, height) {
    this.width = width;
    this.height = height;
    this.gameCanvas.width = width;
    this.gameCanvas.height = height;
}

GameObj.prototype.init = function(parent, width, height) {
    this.setCanvasSize(width, height);
    parent.appendChild(this.gameCanvas);
    this.ball = new BallObj();
    this.ball.init(this, 100, width/2, height/2);
    this.playerPaddle = new PaddleObj();
    this.playerPaddle.init(this, 'left');
    this.gameCanvas.onmousemove = function(playerPaddle) {
        return function(e) {
            playerPaddle.setYPosition(e.offsetY || e.layerY);
        }
    }(this.playerPaddle);
    this.aiPaddle = new AIPaddleObj();
    this.aiPaddle.init(this,'right');
}

GameObj.prototype.update = function(delta_time) {
    this.ball.update(delta_time);
    this.aiPaddle.update(delta_time);
}

GameObj.prototype.drawFrame = function() {
    var ctx = this.gameCanvas.getContext("2d");
    ctx.fillStyle="black";
    ctx.fillRect(0,0,this.gameCanvas.width,this.gameCanvas.height);
    this.ball.render(ctx);
    this.playerPaddle.render(ctx);
    this.aiPaddle.render(ctx);
}

GameObj.prototype.run = function(delta_time) {
    this.update(delta_time);
    this.drawFrame();
}




var setup = function() {
    var div = document.getElementById('pongDiv');
    var game = new GameObj();
    game.init(div, 640, 480);
    var previous_time = 0;
    var frameCallback = function(timestamp) {
        if (previous_time == 0) {
            previous_time = timestamp;
        }
        //convert to seconds
        delta_time = (timestamp - previous_time)/1000;
        //fix the delta time to a max of 0.1 seconds, so that even if frame rate is really slow, the jumps don't get too big
        if (delta_time > 0.1)
            delta_time = 0.1;

        previous_time = timestamp;
        game.run(delta_time);
        window.requestAnimationFrame(frameCallback);
    }
    window.requestAnimationFrame(frameCallback);
    /*
    window.onresize = function() {
        game.setCanvasSize(window.innerWidth, window.innerHeight);
    }
    */
}

I would really appreciate any comments on general structure or techniques that I've used here from experienced devs.

share|improve this question
add comment

1 Answer 1

up vote 2 down vote accepted
+50

I don't see anything wrong with your code as it stands but there are at least two things you may need to think about as it develops:

  1. can objects drawn on the canvas interfere with each other, and if so, how can the programme deal with that?

  2. are you likely to want a library of drawing functions, and if so, how will the individual rendering functions access those?

I would deal with these concerns by setting up an object called Canvas or View which handles all of the drawing and can contain generic drawing functions, e.g. circle, to avoid repetition within the methods rendering individual objects.

Then I would actually move the rendering methods to within this object, and have a single method canvas.draw that draws everything you want on the canvas. This is in some ways less elegant, but separates the main game logic from the drawing logic (analogous to separating content from style in web design), and helps to deal with the possibility of objects interacting in the display; for instance, what happens if they overlap? The order or way they are drawn may need to vary depending on the other objects. (Though this is not an issue in your game currently). Canvas can keep a register of which objects need to be drawn and contain consistent logic to draw them in the right way.

The ball and paddle objects don't need to be passed the entire game object; they could be passed a more limited object, say frame, that only contains what they 'need to know' (in this case, the width and height of the playing area).

As GameObj and Canvas are only likely to have a single instance each and their methods unlikely to be rewritten in other parts of the code, I would tend to write each within a single constructor function so that variables can be shared among their methods without having to make everything a public property of the object.

Finally, it would seem natural to merge setup with the code initiating GameObj.

// ...
function Canvas(width, height) {
    var el = document.createElement('canvas'),
        objects = [],
        ctx = el.getContext("2d");
    this.setSize = function(width, height) {
        el.width = width;
        el.height = height;
    };
    this.setSize(width, height);
    this.appendTo = function (div) {
        div.appendChild(el);
    };
    this.register = function (type, object) {
        objects.push({
            type: type,
            object: object
        });
    };
    this.draw = function () {
        for (var i = 0; i < objects.length; i++) {
            render[objects[i].type](objects[i].object);
        }
    };
    var render = {
        ball: function (obj) {
            circle(obj.position.x, obj.position.y, 10, 'white');
        },
        paddle: function (obj) {
            ctx.fillStyle = "white";
            ctx.fillRect(obj.position.x - obj.width / 2, obj.position.y - obj.height / 2, obj.width, obj.height);
        }
    };

    function circle(x, y, radius, colour) {
        ctx.beginPath();
        ctx.fillStyle = colour;
        ctx.arc(x, y, radius, 0, 2 * Math.PI, false);
        ctx.fill();
    }
    // can add extra generic drawing functions here 
}

function GameObj() {
    var canvas,
    frame,
    ball,
    playerPaddle;
    init(document.getElementById('pongDiv'), 640, 480);

    function init(div, width, height) {
        ballObj.init(frame, 100, width / 2, height / 2);
        playerPaddle.init(frame, left);
        canvas = new Canvas(width, height);
        canvas.appendTo(div);
        frame = {
            width: width,
            height: height
        };
        ball = new BallObj();
        playerPaddle = new PlayerPaddle();
        canvas.register({type: 'ball', object: ball});
        canvas.register({type: 'paddle', object: playerPaddle});
        // ...
    }
    var previous_time = 0;
    var frameCallback = function (timestamp) {
        if (previous_time == 0) {
            previous_time = timestamp;
        }
        //convert to seconds
        delta_time = (timestamp - previous_time) / 1000;
        //fix the delta time to a max of 0.1 seconds, so that even if frame rate is really slow, the jumps don't get too big
        if (delta_time > 0.1) delta_time = 0.1;

        previous_time = timestamp;
        update(delta_time);
        canvas.draw();     // draws all of the objects registered earlier
        window.requestAnimationFrame(frameCallback);
    }
    window.requestAnimationFrame(frameCallback);

    //...
}
share|improve this answer
    
This is a really great answer. Thank you so much! –  bencoder Feb 11 '13 at 19:20
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.