8
\$\begingroup\$

I am trying to learn how to write good, clean, and efficient JavaScript. I consider myself a novice, at best, in JavaScript. Any comments are appreciated.

In an effort to become a better JavaScript programmer, I am trying to write a simple JavaScript animation using HTML5's new canvas element. Functionally speaking, when the canvas is clicked, a new circle (or Ball) is added to the canvas and begins moving to the lower right corner of the canvas. When the ball hits a wall or collides with another ball, the speed at which the x and y coordinates are moving is inverted.

Is it a good idea to be modifying the this object? For instance does the following pattern make sense?

function Point(I) {
    this.x = I.x;
    this.y = I.y;
    this.z = I.z;
    return this;
}

Is there any reason why I shouldn't be sending a object and pulling variables off of the input object? I like the syntax for creation, but are there any problems with this pattern?

new Point({
    x: 10,
    y: 30
});
new Point({
    x: 50,
    y: 25
    z: 3
});

Full Code

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Ball Collision</title>
<script src="js/modernizr-1.7.js"></script>
<script>
    window.onload = function () {
        //"use strict";

        // Source: http://www.html5rocks.com/tutorials/canvas/notearsgame/
        Number.prototype.clamp = function(min, max) {
            return Math.min(Math.max(this, min), max);
        };


        if (Modernizr.canvas) {
            var canvas = document.createElement('canvas'),
                ctx = canvas.getContext('2d');


            canvas.width = 500;
            canvas.height = 375;
            canvas.style.border = "3px solid red";

            function Point(I) {
                console.log("Point created");
                this.x = I.x;
                this.y = I.y;
                this.z = I.z;
                return this;
            }

            function Ball(I) {
                console.log("Ball created");

                var SPEED_LIMIT = 10;

                this.point = new Point(I.point);
                this.radius = (I.radius > 10 ? I.radius : 10);
                this.color = I.color;
                this.speed = new Point(I.speed);
                this.collision = false;
                this.mass = I.mass || 1;

                this.draw = function() {
                    this.point.x = this.point.x.clamp(0 + this.radius, canvas.width - this.radius);
                    this.point.y = this.point.y.clamp(0 + this.radius, canvas.height - this.radius);
                    ctx.beginPath();
                    ctx.arc(this.point.x, this.point.y, this.radius, 0, Math.PI*2, false);
                    ctx.closePath();
                    ctx.fillStyle = this.color;
                    ctx.fill();
                };

                this.update = function() {
                    this.clear();

                    if ( this.point.x <= this.radius || this.point.x >= (canvas.width - this.radius)) {
                        this.speed.x *= -1;
                    }

                    if ( this.point.y <= this.radius || this.point.y >= (canvas.height - this.radius)) {
                        this.speed.y *= -1;
                    }

                    if ( this.speed.x > SPEED_LIMIT ) {
                        this.speed.x = SPEED_LIMIT;
                    }
                    if ( this.speed.x < SPEED_LIMIT * -1 ) {
                        this.speed.x = SPEED_LIMIT * -1;
                    }
                    if ( this.speed.y > SPEED_LIMIT ) {
                        this.speed.y = SPEED_LIMIT;
                    }
                    if ( this.speed.y < SPEED_LIMIT * -1 ) {
                        this.speed.y = SPEED_LIMIT * -1;
                    }

                    this.point.x += this.speed.x;
                    this.point.y += this.speed.y;
                };

                this.clear = function() {
                    ctx.beginPath();
                    ctx.arc(this.point.x - 1, this.point.y - 1, this.radius + 2, 0, Math.PI*2, false);
                    ctx.closePath();
                    ctx.fillStyle = "#fff";
                    ctx.fill();
                };

                return this;
            }

            var balls = [];

            function handleCollisions() {
                function collide(ball1, ball2) {
                    ball1.speed.x *= -1;
                    ball2.speed.x *= -1;
                    ball1.speed.y *= -1;
                    ball2.speed.y *= -1;
                }


                function checkCollisions() {
                    var i, j, a, b;

                    for ( i = 0; i < balls.length; i++ ) {
                        for ( j = i+1; j < balls.length; j++ ) {
                            a = balls[i];
                            b = balls[j];

                            var dx = Math.abs(a.point.x - b.point.x);
                            var dy = Math.abs(a.point.y - b.point.y);
                            var d = (dx*dx)+(dy*dy);
                            var r = (a.radius + b.radius) * (a.radius + b.radius);

                            if ( d < r && ( !a.collision || !b.collision ) ) {
                                a.collision = true;
                                b.collision = true;
                                collide(a, b);
                            } else if ( d > r ) {
                                a.collision = false;
                                b.collision = false;
                            }                           
                        }
                    }
                }

                checkCollisions();
            }

            function update() {
                balls.forEach(function(ball) {
                    ball.update();
                });

                handleCollisions();
            }

            function draw() {
                balls.forEach(function(ball) {
                    ball.draw();
                });
            }

            var FPS = 30;
            var loop = setInterval( function() {
                update();
                draw();
            }, 1000/FPS);

            canvas.onclick = function(e) {              
                var radiusAndMass = Math.floor(Math.random() * 50) + 1;
                var speed = Math.floor(Math.random() * 10) + 1;

                balls.push(new Ball({
                    point: {
                        //x: Math.floor(Math.random() * canvas.width),
                        //y: Math.floor(Math.random() * canvas.height),
                        x: e.offsetX,
                        y: e.offsetY
                    },
                    color: "#"+((1<<24)*Math.random()|0).toString(16),
                    radius: radiusAndMass,
                    mass: radiusAndMass,
                    speed: {
                        x: speed,
                        y: speed
                    }
                }));
            };


            document.getElementById('canvas-container').appendChild(canvas);
        }
    };
</script>   
</head>
<body>
<div id="wrap">
    <div id="canvas-container">

    </div>
</div>
</body>
</html>
\$\endgroup\$
7
\$\begingroup\$

Is it a good idea to be modifying the this object? For instance does the following pattern make sense?

function Point(I) {
    this.x = I.x;
    this.y = I.y;
    this.z = I.z;
    return this;
}

Yes and no. Modifying this is normal in a constructor function, but what you're doing here is unusual. Something like this would be more common:

function Point(x, y, z) {
    this.x = x;
    this.y = y;
    this.z = z;
}

With this parameter list, it's very clear what we are passing in (3D coordinates); with just I, it's not clear unless we read the code in the constructor. Not a huge deal here, but with Ball, we have no clue what I is supposed to contain unless we read the entire constructor.

There's no reason to return this; you're invoking it with new, and the result of that expression will be the constructed object. The function doesn't need to return anything.

Is there any reason why I shouldn't be sending a object and pulling variables off of the input object? I like the syntax for creation, but are there any problems with this pattern?

There's nothing wrong with it, but since your Point doesn't do anything (i.e. it has no function properties, only x, y and z) you might as well forget about Point and just use object literals if you're passing object literals to the Point constructor anyway. In other words, you're creating an object literal and then copying all of its properties into something that happens to be called a "Point" but aside from that has the exact same capabilities as the object you passed into the constructor. Why do that extra step?


With Ball, you should attach function properties to Ball.prototype rather than assign them inside of the constructor. There is no reason for each Ball object to have its own separate set of function properties, and doing so incurs extra overhead. In other words, with your code someBall.draw != anotherBall.draw, but it would be much more efficient if all balls reference the same draw function property.

\$\endgroup\$
5
\$\begingroup\$

I don't think there are any strong reasons towards any of the approaches (object vs values), but I will list out all the advantages/disadvantages of both ways so that you'll choose what seems better for you:

Passing in an object of the structure you're describing (function Point(I))

[+] creating new instances from older instances or compatible objects (anything with x, y and z) is trivial: new Point(oldPoint) or new Point(thingThatQuaksLikeAPoint)*

[-] creating new instances from just numbers becomes tedious and annoying (and creates garbage (which MAY be an issue in some cases (really bad for number crunching/real time apps on slow hardware/bad JS implementations))). new Point({ x: 1, y: 2, z: 3 }) vs new Point(1, 2, 3) Or, take the following function for instance:

function add(P1, P2) {
    var newX = P1.x + P2.x;
    var newY = P1.y + P2.y;
    var newZ = P1.z + P2.z;

    return new Point({ x: newX, y: newY, z: newZ });

    // why do I have to wrap my numbers it in an object?!? :(
    // new Point(newX, newY, newZ); is so much more to the point!
}

[-] the constructor has access to more than it needs to - it's nice in general to set fences in your app and guarantee that some boundaries are not crossed. If you pass in a full object then by mistake or temptation you might modify the state of the passed object (which leads to very subtle bugs)

Passing in individual components (function Point(x, y, z))

[+] creating new instances from numbers is trivial, sweet short and to the point - it's as easy as new Point(1, 2, 3)

[-] if calling with other Points then it will become tedious to extract x, y and z every time new Point(otherPoint.x, otherPoint.y, otherPoint.z) vs new Point(otherPoint)

So why not support both ways?!

function Point(x, y, z) {
    if (arguments.length === 1) {
        // we got only one argument - it's a Point!
        var that = x; // just aliasing
        this.x = that.x;
        this.y = that.y;
        this.z = that.z;
    } else {
        // let's presume there can be only these 2 cases
        this.x = x;
        this.y = y;
        this.z = z;
    }
}

This might be a bit slower, but it's worth it IMO - it makes creating points a breeze ;)


I do see a couple of bits of code which could be improved in the sample that you pasted. If you're looking to write nice, clean JS then consider looking up how the prototype works. Also learn why requestAnimationFrame is much nicer than setTimeout (which is nicer than setInterval). Also, you may use rgb(1, 2, 3) as a valid color instead of #010203.

* See duck typing

\$\endgroup\$
2
\$\begingroup\$

I think that pattern is fine. It has the advantage that you have "named arguments", which is very readable and can be useful if you call the some function/constructor with different sets of arguments.

The only thing I'd suggest is to use another name for the parameter, because capital letters are generally used for constructor functions:

function Point(p) {
    this.x = p.x;
    this.y = p.y;
    this.z = p.z;
    return this;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.