up vote 1 down vote favorite

I have been playing with JavaScript for some time now and made a simple canvas animation. I tried to make my code clean but I would like to hear your opinions about it.

GitHub

'use strict';

var Ball = function (options = {}) {
	var self = {};
	
	var x = options.x || 0;
	var y = options.y || 0;
	var width = options.width || 20;
	var height = options.height || 20;
	var color = options.color || '#aaaaaa';
	var verticalSpeed = options.verticalSpeed || 1;
	var horizontalSpeed = options.horizontalSpeed || 1;
	
	self.move = function () {
		x += horizontalSpeed;
		y += verticalSpeed;
	};
	
	self.verticalBounce = function () {
		verticalSpeed = -verticalSpeed;
	};
	
	self.horizontalBounce = function () {
		horizontalSpeed = -horizontalSpeed;
	};
	
	self.getX = function () {
		return x;
	};
	
	self.getY = function () {
		return y;
	};
	
	self.getWidth = function () {
		return width;
	};
	
	self.getHeight = function () {
		return height;
	};
	
	self.getColor = function () {
		return color;
	};
	
	self.getVerticalSpeed = function () {
		return verticalSpeed;
	};
	
	self.getHorizontalSpeed = function () {
		return horizontalSpeed;
	};
		
	return Object.seal(self);
};

var Board = function (options = {}) {
	var self = {};
	
	var objects = [];
	var canvas = options.canvas || document.getElementsByTagName('canvas')[0];
	var ctx = canvas.getContext('2d');
	
	var moveObjects = function () {
		objects.forEach(function (object) {
			object.move();
		});
	};
	
	var detectCollisions = function () {
		
		var wallCollision = function (object) {
			if (object.getX() <= 0 || (object.getX() + object.getWidth()) > canvas.width) object.horizontalBounce();
			if (object.getY() <= 0 || (object.getY() + object.getHeight()) > canvas.height) object.verticalBounce();
		};
		
		var areColliding = function (objectA, objectB) {
			return (
				objectA.getX() < objectB.getX() + objectB.getWidth() &&
				objectA.getX() + objectA.getWidth() > objectB.getX() &&
				objectA.getY() < objectB.getY() + objectB.getHeight() &&
				objectA.getHeight() + objectA.getY() > objectB.getY()
			);
		}
		
		objects.forEach(function (currentObject, index, objects) {
			wallCollision(currentObject);
			for (var i = index + 1; i < objects.length; i++) {
				var nextObject = objects[i];
				if (areColliding(currentObject, nextObject)) {
					if (Math.sign(currentObject.getHorizontalSpeed()) !== Math.sign(nextObject.getHorizontalSpeed())) {
						currentObject.horizontalBounce();
						nextObject.horizontalBounce();
					}
					if (Math.sign(currentObject.getVerticalSpeed()) !== Math.sign(nextObject.getVerticalSpeed())) {
						currentObject.verticalBounce();
						nextObject.verticalBounce();
					}
				}
			}
		});
	};
	
	var draw = function () {
		ctx.clearRect(0, 0, canvas.width, canvas.height);
		objects.forEach(function (object) {
			ctx.fillStyle = object.getColor();
			ctx.fillRect(object.getX(), object.getY(), object.getWidth(), object.getHeight());
		});
	};
	
	self.frame = function () {
		moveObjects();
		draw();
		detectCollisions();
	};
	
	self.addObject = function (object) {
		objects.push(object);
	};
	
	return Object.seal(self);
};

var ball1 = Ball();
var ball2 = Ball({horizontalSpeed: -1, x: 638, y: 123, color: '#ff0000', width: 25, height: 60, verticalSpeed: 2, horizontalSpeed: 2});
var ball3 = Ball({verticalSpeed: -1, x: 235, y: 453, color: '#00ff00', width: 75, height: 50});
var ball4 = Ball({horizontalSpeed: 1, x: 300, y: 300, color: '#0000ff', width: 50, height: 30});
var board = Board();

board.addObject(ball1);
board.addObject(ball2);
board.addObject(ball3);
board.addObject(ball4);

setInterval(board.frame, 1000/60);
body {
	background: #000;
	text-align: center;
}
	
canvas {
	background: #434343;
	border: 5px solid #323232;
}
<!doctype html>
<html>
<head>
	<link rel="stylesheet" href="css/main.css">
</head>
<body>
	<canvas width="800" height="600"></canvas>
</body>
</html>

share|improve this question
up vote 0 down vote

Some observations:

  • Try not to define methods inside the Ball constructor, it uses more memory since a new function is created for each ball instance. Rather create them using ES2016 classes (if available) or defining them on the prototype.

  • Rather than defining getX, getY, etc, methods rather create properties (again using ES2016 syntax if possible or otherwise defineProperty)

  • I would add draw and checkCollision methods to the Ball class and move the code there.

  • I would add a class and objects for the walls. This has the advantage that they can draw themselves in the same way as a Ball (The main program just becomes a loop asking every object to draw itself) and that you can generalize collision detection as just being between two objects without worrying about their types. As you add different objects to your code this keeps things much simpler.

share|improve this answer

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.