Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I've written this bit of javascript to just learn basics of drawing on/interacting with the HTML5 canvas element...

Just want to make sure I'm doing things "correctly" in case I have any glaring code gaffs or there's a more efficient way.

ball.js

var ball = (function(){
    var ball;

    var mouseMoveEvent;
    var prevMouseMoveEvent;

    // set default options
    var default_options = {
        context : "",   // required
        radius : 20,
        color : "#F33", 
        startX: window.innerWidth / 2,
        startY: window.innerHeight / 2
    };

    return {
        draw: function()
        {
            // prep canvas
            ball.o.context.canvas.width  = window.innerWidth;
            ball.o.context.canvas.height = window.innerHeight;

            //capture current position values
            var cur_x = ball.posX;
            var cur_y = ball.posY;

            //capture current context dimensions
            var ctx_width  = ball.o.context.canvas.width;
            var ctx_height = ball.o.context.canvas.height;


            if (ball.isGrabbed)
            {
                //-------------- track ball with mouse when grabbed
                mouseOffsetX = mouseMoveEvent.x - prevMouseMoveEvent.x;
                mouseOffsetY = mouseMoveEvent.y - prevMouseMoveEvent.y;

                ball.posX += mouseOffsetX;
                ball.posY += mouseOffsetY;

                // save previous mouse move state
                prevMouseMoveEvent = mouseMoveEvent;
            }
            else
            {
                //-------------- bounding
                var x_reach = Math.abs(ball.iterX) + ball.o.radius;
                var y_reach = Math.abs(ball.iterY) + ball.o.radius;

                if ((cur_x + x_reach) > ctx_width || (cur_x - x_reach) < 0 )
                    ball.iterX = -(.70 * ball.iterX);

                if ((cur_y + y_reach) > ctx_height || (cur_y - y_reach) < 0 )
                    ball.iterY = -(.70 * ball.iterY);

                ball.iterX *= .999;
                ball.iterY *= .999;
                ball.posX += ball.iterX;
                ball.posY += ball.iterY;
            }

            //-------------- protect browser borders
            // North
            if (ball.posY - ball.o.radius < 0)
                ball.posY = ball.o.radius;
            // South
            else if (ball.posY + ball.o.radius > ctx_height)
                ball.posY = ctx_height - ball.o.radius;
            // East
            else if (ball.posX + ball.o.radius > ctx_width)
                ball.posX = ctx_width - ball.o.radius;
            // West
            else if (ball.posX - ball.o.radius < 0)
                ball.posX = ball.o.radius;

            //-------------- draw
            ball.o.context.beginPath();
            ball.o.context.fillStyle = ball.o.color;
            ball.o.context.arc(ball.posX,ball.posY, ball.o.radius,0,Math.PI*2,true);
            ball.o.context.closePath();
            ball.o.context.fill();
        },
        mouseDown: function(e)
        {
            // grab ball
            if (ball.o.context.isPointInPath(e.x, e.y))
            {
                prevMouseMoveEvent = e;
                ball.isGrabbed = true;
            }
        },
        mouseUp: function(e)
        {
            // release
            if (ball.isGrabbed)
            {
                // set iter speed based on mouse speed on release
                ball.iterX = mouseMoveEvent.x - prevMouseMoveEvent.x;
                ball.iterY = mouseMoveEvent.y - prevMouseMoveEvent.y;

                ball.isGrabbed = false;                
            }          
        },
        mouseMove: function(e)
        {
            if (ball.o.context.isPointInPath(e.x, e.y))
            {
                document.body.style.cursor = "move";
            }
            else
            {
                document.body.style.cursor = "default";
            }
            mouseMoveEvent = e;
        },
        init: function(options) 
        {
            ball = this;

            //load up defaults
            ball.o = default_options;

            // merge in user options that exist
            for (var attr in options) { ball.o[attr] = options[attr]; };

            // set starting values
            ball.posX = ball.o.startX;
            ball.posY = ball.o.startY;
            ball.iterX = 0;
            ball.iterY = 0;
            ball.isGrabbed = false;

            // attach events
            window.onmousedown = ball.mouseDown;
            window.onmouseup = ball.mouseUp;
            window.onmousemove = ball.mouseMove;

            // start
            setInterval(ball.draw, 1);
        },
   };

})();

index.html

<html>
<head>
    <title>Bouncing Ball - jondavidjohn</title>
    <style type="text/css">
        body {
            margin:0px; 
        }

        #area {
            width:100%;
            height:100%;
            margin:0px;
            background:#FF7
        }
    </style>
    <script type="text/javascript" src="ball.js"></script>
    <script type="text/javascript">

        function start()
        {
            canvas = document.getElementById("area");

            // initiate ball
            ball.init({
                context : canvas.getContext('2d'),
                color   : "#F33",
                radius  : 30,
            });
        }

    </script>
</head>
<body onLoad="start();">
    <canvas id="area"></canvas>
</body>
</html>
share|improve this question
add comment (requires an account with 50 reputation)

1 Answer

up vote 1 down vote accepted

This looks pretty good. In your start function, the canvas variable should be declared with a var; implicit globals are one of the Worst Parts.

function start()
{
    var canvas = document.getElementById("area");

    // ...
}

Second, the var statement can declare more than one variable at a time, so lines like

var cur_x = ball.posX;
var cur_y = ball.posY;

should be

var cur_x = ball.posX, cur_y = ball.posY;

Third, this line

for (var attr in options) { ball.o[attr] = options[attr]; };
  1. has an extra semicolon at the end and more importantly
  2. doesn't do a hasOwnProperty check.

The vast majority of the time, when you are iterating over object properties, you should check to make sure the properties actually exist on the object instead of the prototype chain. More more information read this article by Douglas Crockford. So it should look like:

for (var attr in options) {
    if (options.hasOwnProperty(attr))
        ball.o[attr] = options[attr];
}
share|improve this answer
add comment (requires an account with 50 reputation)

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.