Is this acceptable javascript?

[hustlerinc]

Hi, I'm new to javascript and game developement in general. I would like to know if this code is ok, also where can I improve?

 

It's a simple pong game, not 100% playable because of difficulty to score and a few bugs. However the intention was to learn not create a game for the app store. So if someone would like to dissect my code and give me some critique on it I would appreciate it.

var canvas = document.getElementById('canvas');var c = canvas.getContext('2d');function Game() {var self = this;	this.Run = function() {		if (canvas != null) {			self.gameLoop = setInterval(self.Loop, 20); // do not use setInterval! do RAF instead		}	}		this.Update = function() {		bot.Move();		ball.Move();		ball.Bounce();		self.Score();	}		this.Draw = function() {		c.clearRect(0, 0, canvas.width, canvas.height);		player.Render();		bot.Render();		ball.Render();				score.Render();	}			this.Loop = function() {		self.Update();		self.Draw();	}		this.Paddle = function(X, color) {		this.width = 10;		this.height = 80;		this.X = X;		this.Y = (canvas.height / 2) - (this.height / 2);		this.speed = 4;		this.color = color;				this.score = 0;	}		this.Paddle.prototype.Render = function() {		c.fillStyle = this.color;		c.fillRect(this.X, this.Y, this.width, this.height);	}		var player = new self.Paddle(10, '#0000ff');	var bot = new self.Paddle((canvas.width - 10) - 10, '#ff0000');		bot.Move = function() {		if (ball.Y <= this.Y + 32) {			bot.Y -= bot.speed;		} else if (ball.Y >= (this.Y + this.height) - 32) {			bot.Y += bot.speed;		}	}			this.Ball = function() {		this.color = '#000000';		this.radius = 5;		this.X = canvas.width / 2;		this.Y = canvas.height / 2;		this.speed = 5;		this.angle = 0;		this.maxAngle = 7;	}		this.Ball.prototype.Render = function() {		c.fillStyle = this.color		c.beginPath();		c.arc(this.X, this.Y, this.radius, 0, Math.PI*2, true)		c.closePath();		c.fill();	}		this.Ball.prototype.Move = function() {		this.X += this.speed;		this.Y += this.angle;	}		this.Ball.prototype.Bounce = function() {		if ((this.X - this.radius) <= (player.X + player.width) && this.Y >= player.Y && this.Y <= (player.Y + player.height)) {			this.speed = -this.speed;			this.hit = this.Y - player.Y;						if (this.hit <= (player.height / 2) - 1) {				hitPercent = ((player.height / 2) - this.hit) / (player.height / 2);				this.angle = -(this.maxAngle * hitPercent);			} else if (this.hit >= (player.height / 2) + 1) {				hitPercent = ((player.height / 2) - this.hit) / (player.height / 2);				this.angle = -(this.maxAngle * hitPercent);			}		} else if ((this.X + this.radius) >= (bot.X) && this.Y >= bot.Y && this.Y <= (bot.Y + bot.height)) {			this.speed = -this.speed;			this.hit = this.Y - bot.Y;						if (this.hit <= (bot.height / 2) - 1) {				hitPercent = ((bot.height / 2) - this.hit) / (bot.height / 2);				this.angle = -(this.maxAngle * hitPercent);			} else if (this.hit >= (bot.height / 2) + 1) {				hitPercent = ((bot.height / 2) - this.hit) / (bot.height / 2);				this.angle = -(this.maxAngle * hitPercent);			}		}				if ((this.Y - this.radius) <= 0) {			this.angle = -this.angle;		} else if ((this.Y + this.radius) >= canvas.height) {			this.angle = -this.angle;		}	}		var ball = new self.Ball();		this.Score = function() {				if (ball.X <= 0) {			bot.score += 1;			self.Reset();		} else if (ball.X >= 480) {			player.score += 1;			self.Reset();			ball.speed = -ball.speed;		}	}		this.Score.prototype.Render = function() {		this.scoreText = player.score + ' - ' + bot.score;		this.scoreTextX = (canvas.width / 2) - (c.measureText(this.scoreText).width / 2);		c.fillStyle = '#000000';		c.font = 'bold 20px Arial';		c.fillText(this.scoreText, this.scoreTextX, 20);	}		var score = new self.Score();		this.Reset = function() {		player.Y = (canvas.height / 2) - (player.height / 2);		bot.Y = (canvas.height / 2) - (bot.height / 2);		ball = new self.Ball();	}		document.addEventListener('keydown', function(event) {		if(event.keyCode == 38) {			player.Y -= player.speed;		}		if(event.keyCode == 40) {			player.Y += player.speed;		}	});}

 

[dreta]

Your code structure is kind of a mess (in the same sense that interleaving public and private field definitions in C++ is a mess), especially the Score function and the way you use it, but since you're looking for strictly JavaScript feedback, and since structuring code is a skill you learn with time, i won't comment much on that.

 

The glaring issue is this piece of code:

    document.addEventListener('keydown', function(event) {        if(event.keyCode == 38) {            player.Y -= player.speed;        }        if(event.keyCode == 40) {            player.Y += player.speed;        }    });

You have no control over when the player's position is being modified or the actual speed of the player, since it depends on how fast the player can press a key. At the same time you don't allow the player to just hold down the key to keep moving. The proper way of handling input would be something like this:

    document.addEventListener('keydown', function(event) {        if(event.keyCode == 38) {            moveUp = true;        }        if(event.keyCode == 40) {           moveDown = true;        }    });    document.addEventListener('keyup', function(event) {        if(event.keyCode == 38) {            moveUp = false;        }        if(event.keyCode == 40) {           moveDown = false;        }    });//In the update routine...if(moveUp){    player.Y -= player.speed;}if(moveDown){    player.Y += player.speed;}

[mwatt]

Three JavaScript things I will comment on:

 

About the intermixing of variables and function definitions within your Game constructor function (one of the things that Dreta commented upon):  In most languages, declaring variables at or near the point of use makes good sense, and that appears to be what you are doing here.  JavaScript is different than most languages however, in that it has no block scope, but does enforce function scope.  The effect of this is that the JavaScript interpreter will act as if all of your variables have been declared at the top of each function - a phenomenon known as "hoisting".  For this reason, JavaScript elders such as Douglas Crockford recommend declaring variables at the top of your functions, therefore aligning your visible code with what will actually happen.  There are examples available as to what "gotchas" can occur if you don't understand this hoisting behavior, but I am not going to go into them here - search for JavaScript hoisting on the web.

 

About the uppercase first letter of some of your functions: The typical rule of thumb is to start functions with a lowercase character (and camel case after that) unless the function will be used as a constructor (i.e. something that you will call with new to create a new object instance).  It looks like you are not consistently following this rule of thumb above, unless I misread the code.

 

Regarding your use of "self" as an alias for "this".  This is a common trick to work around situations in which the "this" pointer has been reassigned by outside forces such as event handlers and setTimeout calls.  I am not sure I understand why it is needed in your code here, unless you are protecting against that.

 

Regarding the way the game works, aside from the JavaScript:  I don't feel qualified to comment on that yet, I too am still a beginner in that arena.

[alex_h]

 

Regarding your use of "self" as an alias for "this".....  I am not sure I understand why it is needed in your code here, unless you are protecting against that.

 

Risking getting this completely wrong here but my understanding is that within any methods of the Game object above, the keyword 'this' will point to the scope of the method itself rather than the scope of the instance of Game. So for example inside the 'Run' method the OP has to use 'self' to reference the scope of Game, if they used 'this' instead it would refer to the scope of the function Run rather than the function Game.

 

Interestingly I've recently had to start working with Cocos2D HTML5 for my day job which uses the JohnResig psuedo class syntax whereby Objects are defined more like Object literals, and in that scenario the scope seems to work differently, so you don't need to keep a reference to self/this. Example:

var Test = Blah.extend({myMethod:function(){    //inside here, keyword 'this' still refers to the scope of Test, not the scope of myMethod}});

[Bodman]

From a functioning point, code checks out.

 

But with regards to organization. I think it could be improved.

 

I have re-factored the code (non tested).  

 

The biggest thing was separating the different objects. Make sure your code reads like a book.

One other thing is the bot object, I created a object prototype that extends the paddle object, making it easier to add to.

var canvas = document.getElementById('canvas');var c = canvas.getContext('2d');//Notice I do not use self as reference,//Singleton type structurevar Game = {    run:function(){        this.ball = new Ball();        this.player = new Player(10, '#0000ff');        this.bot = new Bot((canvas.width - 10) - 10, '#ff0000');        this.score = new Score(this);        if(canvas != null){           this.gameLoop = setInterval(this.proxy(this.loop,this),20);        }        this.initKeyEvents();    },    update:function(){        this.bot.move(this.ball, this.bot);        this.ball.move();        this.ball.bounce(this.player, this.bot);        this.score.update();    },    draw:function(){        c.clearRect(0, 0, canvas.width, canvas.height);        this.player.render();        this.bot.render();        this.ball.render();                this.score.render();    },    loop:function(){        this.update();        this.draw();    },    reset:function(){        this.player.y = (canvas.height / 2) - (player.height / 2);        this.bot.y = (canvas.height / 2) - (bot.height / 2);        this.ball = new Ball();    },    //This is a helper, allows you to run callbacks in the correct scope    proxy:function(method, scope) {        return function() {            if (FreeRider.developerMode === false) {                try {                    return method.apply(scope, arguments);                } catch(e) { }            } else {                return method.apply(scope, arguments);            }        }    },    initKeyEvents:function(){        document.addEventListener('keydown', function(event) {            var player = this.player;            if(event.keyCode == 38) {                player.y -= player.speed;            }            if(event.keyCode == 40) {                player.y += player.speed;            }        });    }}//Paddle Objectfunction Paddle(x,color){  this.init(x, color);}Paddle.prototype = {          init:function(){        this.width = 10;        this.height = 80;        this.x = x;        this.y = (canvas.height / 2) - (this.height / 2);        this.speed = 4;        this.color = color;        this.score = 0;    },        render:function() {        c.fillStyle = this.color;        c.fillRect(this.x, this.y, this.width, this.height);    }}//Ball Objectfunction Ball(){    this.init();}Ball.prototype = {    init:function(){        this.color = '#000000';        this.radius = 5;        this.X = canvas.width / 2;        this.Y = canvas.height / 2;        this.speed = 5;        this.angle = 0;        this.maxAngle = 7;    },           render:function() {        c.fillStyle = this.color        c.beginPath();        c.arc(this.x, this.y, this.radius, 0, Math.PI*2, true)        c.closePath();        c.fill();    },        move:function(){        this.x += this.speed;        this.y += this.angle;    },    bounce:function(player, bot){                if((this.x - this.radius) <= (player.x + player.width)          && this.y >= player.y          && this.y <= (player.y + player.height) ){            this.speed = -this.speed;            this.hit = this.y - player.y;            if (this.hit <= (player.height / 2) - 1) {                hitPercent = ((player.height / 2) - this.hit) / (player.height / 2);                this.angle = -(this.maxAngle * hitPercent);            } else if (this.hit >= (player.height / 2) + 1) {                hitPercent = ((player.height / 2) - this.hit) / (player.height / 2);                this.angle = -(this.maxAngle * hitPercent);            }        }         else if ((this.x + this.radius) >= (bot.x) && this.y >= bot.y && this.y <= (bot.y + bot.height)) {            this.speed = -this.speed;            this.hit = this.y - bot.y;                        if (this.hit <= (bot.height / 2) - 1) {                hitPercent = ((bot.height / 2) - this.hit) / (bot.height / 2);                this.angle = -(this.maxAngle * hitPercent);            } else if (this.hit >= (bot.height / 2) + 1) {                hitPercent = ((bot.height / 2) - this.hit) / (bot.height / 2);                this.angle = -(this.maxAngle * hitPercent);            }        }                if ((this.y - this.radius) <= 0) {            this.angle = -this.angle;        } else if ((this.y + this.radius) >= canvas.height) {            this.angle = -this.angle;        }    }    move:function() {        this.X += this.speed;        this.Y += this.angle;    }    }//Bot Object (Extend Paddle Object)function Bot(x, color){    this.init(x, color);}Bot.prototype = new Paddle();Bot.prototype._super = Bot.prototype.init;Bot.prototype.init = function(x,color){    this._super(x,color);}Bot.prototype.move = function(ball, bot){    if (ball.y <= this.y + 32) {        bot.y -= bot.speed;    } else if (ball.y >= (this.y + this.height) - 32) {        bot.y += bot.speed;    }}//Score Objectfunction Score(gameInstance) {    this.game = gameInstance;}   Score.prototype = {    update:function(){        var gameInstance = this.game;        var ball = gameInstance.ball;        var bot = gameInstance.bot;        var player = gameInstance.player;        if (ball.x <= 0) {            bot.score += 1;            gameInstance.reset();        } else if (ball.x >= 480) {            player.score += 1;            gameInstance.Reset();            ball.speed = - ball.speed;        }    },    render:function(){        var gameInstance = this.game;        var player = gameInstance.player;        var bot = gameInstance.bot;        var scoreText = player.score + ' - ' + bot.score;        var scoreTextX = (canvas.width / 2) - (c.measureText(scoreText).width / 2);        c.fillStyle = '#000000';        c.font = 'bold 20px Arial';        c.fillText(scoreText, scoreTextX, 20);    }}     

[Vidsneezes]

 

From a functioning point, code checks out.

 

But with regards to organization. I think it could be improved.

 

I have re-factored the code (non tested).  

 

The biggest thing was separating the different objects. Make sure your code reads like a book.

One other thing is the bot object, I created a object prototype that extends the paddle object, making it easier to add to.

var canvas = document.getElementById('canvas');var c = canvas.getContext('2d');//Notice I do not use self as reference,//Singleton type structurevar Game = {    run:function(){        this.ball = new Ball();        this.player = new Player(10, '#0000ff');        this.bot = new Bot((canvas.width - 10) - 10, '#ff0000');        this.score = new Score(this);        if(canvas != null){           this.gameLoop = setInterval(this.proxy(this.loop,this),20);        }        this.initKeyEvents();    },    update:function(){        this.bot.move(this.ball, this.bot);        this.ball.move();        this.ball.bounce(this.player, this.bot);        this.score.update();    },    draw:function(){        c.clearRect(0, 0, canvas.width, canvas.height);        this.player.render();        this.bot.render();        this.ball.render();                this.score.render();    },    loop:function(){        this.update();        this.draw();    },    reset:function(){        this.player.y = (canvas.height / 2) - (player.height / 2);        this.bot.y = (canvas.height / 2) - (bot.height / 2);        this.ball = new Ball();    },    //This is a helper, allows you to run callbacks in the correct scope    proxy:function(method, scope) {        return function() {            if (FreeRider.developerMode === false) {                try {                    return method.apply(scope, arguments);                } catch(e) { }            } else {                return method.apply(scope, arguments);            }        }    },    initKeyEvents:function(){        document.addEventListener('keydown', function(event) {            if(event.keyCode == 38) {                this.player.y -= player.speed;            }            if(event.keyCode == 40) {                player.y += player.speed;            }        });    }}//Paddle Objectfunction Paddle(x,color){  this.init(x, color);}Paddle.prototype = {          init:function(){        this.width = 10;        this.height = 80;        this.x = x;        this.y = (canvas.height / 2) - (this.height / 2);        this.speed = 4;        this.color = color;        this.score = 0;    },        render:function() {        c.fillStyle = this.color;        c.fillRect(this.x, this.y, this.width, this.height);    }}//Ball Objectfunction Ball(){    this.init();}Ball.prototype = {    init:function(){        this.color = '#000000';        this.radius = 5;        this.X = canvas.width / 2;        this.Y = canvas.height / 2;        this.speed = 5;        this.angle = 0;        this.maxAngle = 7;    },           render:function() {        c.fillStyle = this.color        c.beginPath();        c.arc(this.x, this.y, this.radius, 0, Math.PI*2, true)        c.closePath();        c.fill();    },        move:function(){        this.x += this.speed;        this.y += this.angle;    },    bounce:function(player, bot){                if((this.x - this.radius) <= (player.x + player.width)          && this.y >= player.y          && this.y <= (player.y + player.height) ){            this.speed = -this.speed;            this.hit = this.y - player.y;            if (this.hit <= (player.height / 2) - 1) {                hitPercent = ((player.height / 2) - this.hit) / (player.height / 2);                this.angle = -(this.maxAngle * hitPercent);            } else if (this.hit >= (player.height / 2) + 1) {                hitPercent = ((player.height / 2) - this.hit) / (player.height / 2);                this.angle = -(this.maxAngle * hitPercent);            }        }         else if ((this.x + this.radius) >= (bot.x) && this.y >= bot.y && this.y <= (bot.y + bot.height)) {            this.speed = -this.speed;            this.hit = this.y - bot.y;                        if (this.hit <= (bot.height / 2) - 1) {                hitPercent = ((bot.height / 2) - this.hit) / (bot.height / 2);                this.angle = -(this.maxAngle * hitPercent);            } else if (this.hit >= (bot.height / 2) + 1) {                hitPercent = ((bot.height / 2) - this.hit) / (bot.height / 2);                this.angle = -(this.maxAngle * hitPercent);            }        }                if ((this.y - this.radius) <= 0) {            this.angle = -this.angle;        } else if ((this.y + this.radius) >= canvas.height) {            this.angle = -this.angle;        }    }    move:function() {        this.X += this.speed;        this.Y += this.angle;    }    }//Bot Object (Extend Paddle Object)function Bot(x, color){    this.init(x, color);}Bot.prototype = new Paddle();Bot.prototype._super = Bot.prototype.init;Bot.prototype.init = function(x,color){    this._super(x,color);}Bot.prototype.move = function(ball, bot){    if (ball.y <= this.y + 32) {        bot.y -= bot.speed;    } else if (ball.y >= (this.y + this.height) - 32) {        bot.y += bot.speed;    }}//Score Objectfunction Score(gameInstance) {    this.game = gameInstance;}   Score.prototype = {    update:function(){        var gameInstance = this.game;        var ball = gameInstance.ball;        var bot = gameInstance.bot;        var player = gameInstance.player;        if (ball.x <= 0) {            bot.score += 1;            gameInstance.reset();        } else if (ball.x >= 480) {            player.score += 1;            gameInstance.Reset();            ball.speed = - ball.speed;        }    },    render:function(){        var gameInstance = this.game;        var player = gameInstance.player;        var bot = gameInstance.bot;        var scoreText = player.score + ' - ' + bot.score;        var scoreTextX = (canvas.width / 2) - (c.measureText(scoreText).width / 2);        c.fillStyle = '#000000';        c.font = 'bold 20px Arial';        c.fillText(scoreText, scoreTextX, 20);    }}     

 

That is some fine looking code.  I agree with this.

[1-800-STAR-WARS]

 

Risking getting this completely wrong here but my understanding is that within any methods of the Game object above, the keyword 'this' will point to the scope of the method itself rather than the scope of the instance of Game. So for example inside the 'Run' method the OP has to use 'self' to reference the scope of Game, if they used 'this' instead it would refer to the scope of the function Run rather than the function Game.

 

Interestingly I've recently had to start working with Cocos2D HTML5 for my day job which uses the JohnResig psuedo class syntax whereby Objects are defined more like Object literals, and in that scenario the scope seems to work differently, so you don't need to keep a reference to self/this. Example:

var Test = Blah.extend({myMethod:function(){    //inside here, keyword 'this' still refers to the scope of Test, not the scope of myMethod}});

 

That's what Function.prototype.bind() is for! There's even a handy polyfill on the MDN page.

[alex_h]

Sure, I'm aware of bind, call and apply as means of controlling the scope in which a function executes, I just didn't happen to mention them in my reply.

[mwatt]

Also, depending upon target (mobile is the issue mostly), Function.prototype.bind may not be supported.  For example, iPad 1 and I think the some or all of the iPad 2 versions do not support bind.  For sure you can employ a shim, but I don't know how effective bind polyfills are.  I started to read about limitations on one implementation and decided it was too much trouble to try and understand.  Personally, I decided to just make sure I can really use bind or not use it at all.  I'm open to re-thinking this though if someone slaps me around about it.

[alex_h]

I think the some or all of the iPad 2 versions do not support bind.  

 

It would be a question of what iOS version the hardware is running, and hence which version of Safari is being used. For what it's worth I've being using a bind polyfill I copied from Pixi.js and have never come across any problems running my games on iOS5 on an iPad1 or any other hardware.

[mwatt]

>> It would be a question of what iOS version the hardware is running, and hence which version of Safari is being used.

Indeed.  Kind of  hard to find the information on the web.  I did nail it down to knowing iPad 3 is ok and probably the first cut of iPad 2 is not - that was as far as I got.

 

>> For what it's worth I've being using a bind polyfill I copied from Pixi.js and have never come across any problems running my games on iOS5 on an iPad1 or any other hardware.

This is good information, thank you very much.