HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Splitting canvas app into separate functions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
appintosplittingseparatefunctionscanvas

Problem

This is all in one methods but I would like to see it in individual separate methods. The code works fine but all of the code is in the canvasApp() methods and I would like to see it all in individual methods so that I could call them as below.

Question: How to make all of the code into separate function?

canvasApp();
gameloop();


Code:

function canvasApp() {

  if (!canvasSupport()) {
             return;
        }

    function  drawScreen () {
        context.fillStyle = '#EEEEEE';
        context.fillRect(0, 0, theCanvas.width, theCanvas.height);
        //Box
        context.strokeStyle = '#000000'; 
        context.strokeRect(1,  1, theCanvas.width-2, theCanvas.height-2);
        // Create ball
        y += speed;
        context.fillStyle = "#000000";
        context.beginPath();
        context.arc(x,y,15,0,Math.PI*2,true);
        context.closePath();
        context.fill();
    }

    theCanvas = document.getElementById('canvasOne');
    context = theCanvas.getContext('2d');

    var speed = 5;
    var y = 10;
    var x = 250;

    function gameLoop() {
        window.setTimeout(gameLoop, 20);
        drawScreen();   
    }

    gameLoop();
}

Solution

From a Code Review perspective, what you are asking for, is worse.

Currently, you only need to call canvasApp() and that function does all the work. Inside you have drawScreen which is periodically called, starting in gameLoop.

Still,

  • I would put all the var statements on top for readability



  • I would put var in front of context and theCanvas (!)



  • I would assign your colors ('#EEEEEE') to meaningfully named variables



  • I would assign Math.PI2 to a var doublePi = Math.PI2 for speed



  • I would consider dropping the curly braces from the guard clause



  • //Box is not really a meaningful naming, //border and //background would make more sense`



That would give this:

function canvasApp() {

    if (!canvasSupport())
      return;

    var canvas = document.getElementById('canvasOne'),
        context = canvas.getContext('2d'), 
        borderColor = '#EEEEEE',
        backgroundColor = '#000000',
        ballColor = '#000000',
        speed = 5,
        y = 10,
        x = 250,
        doublePi = Math.PI*2,
        ballSize = 15,
        refreshInterval = 20; //In milliseconds

    function  drawScreen () {
        //Background + Border
        context.fillStyle = borderColor;
        context.fillRect(0, 0, canvas.width, canvas.height);
        //Background
        context.strokeStyle = backgroundColor; 
        context.strokeRect(1,  1, canvas.width-2, canvas.height-2);
        // Create ball
        y += speed;
        context.fillStyle = ballColor;
        context.beginPath();
        context.arc(x,y,ballSize,0,doublePi,true);
        context.closePath();
        context.fill();
    }

    function gameLoop() {
        window.setTimeout(gameLoop, refreshInterval);
        drawScreen();   
    }

    gameLoop();
}


I am still not too excited by redrawing every 20 milliseconds 2 large boxes, but I will leave the research on further optimization to you.

Update: after playing the code and reading it more closely, the code only redraws a square once, and then the border, so that should be fine.

A working jsbin version is here: the animation is fast enough, though disappointing as there is no bouncing..
http://jsbin.com/lidil/2

Code Snippets

function canvasApp() {

    if (!canvasSupport())
      return;

    var canvas = document.getElementById('canvasOne'),
        context = canvas.getContext('2d'), 
        borderColor = '#EEEEEE',
        backgroundColor = '#000000',
        ballColor = '#000000',
        speed = 5,
        y = 10,
        x = 250,
        doublePi = Math.PI*2,
        ballSize = 15,
        refreshInterval = 20; //In milliseconds

    function  drawScreen () {
        //Background + Border
        context.fillStyle = borderColor;
        context.fillRect(0, 0, canvas.width, canvas.height);
        //Background
        context.strokeStyle = backgroundColor; 
        context.strokeRect(1,  1, canvas.width-2, canvas.height-2);
        // Create ball
        y += speed;
        context.fillStyle = ballColor;
        context.beginPath();
        context.arc(x,y,ballSize,0,doublePi,true);
        context.closePath();
        context.fill();
    }

    function gameLoop() {
        window.setTimeout(gameLoop, refreshInterval);
        drawScreen();   
    }

    gameLoop();
}

Context

StackExchange Code Review Q#45790, answer score: 2

Revisions (0)

No revisions yet.