Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I created a classic snake game in the canvas element. I have not considered best practices when doing this, I just wanted to finish it first. Now it's time to improve the coding practice. You can help me out by mentioning bad practices, giving code improvements and suggesting anything else.

<!DOCTYPE HTML>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<title>Feed the Snake v 1.1 beta</title>
<style>
body
{
    background:#000;
    color:#FFF;
}
canvas
{
    background:#FFF;
}
#controls
{
    position:absolute;
    top:0;
    right:0;
    margin:10px;
}
</style>
<script type="text/javascript">
var snake = window.snake || {};
function launchFullscreen(element) {
  if(element.requestFullscreen) {
    element.requestFullscreen();
  } else if(element.mozRequestFullScreen) {
    element.mozRequestFullScreen();
  } else if(element.webkitRequestFullscreen) {
    element.webkitRequestFullscreen();
  } else if(element.msRequestFullscreen) {
    element.msRequestFullscreen();
  }
}
window.onload = function(){
    document.addEventListener("fullscreenchange", function(){snake.game.adjust();});
    document.addEventListener("webkitfullscreenchange", function(){snake.game.adjust();});
    document.addEventListener("mozfullscreenchange", function(){snake.game.adjust();});
    document.addEventListener("MSFullscreenChange", function(){snake.game.adjust();});

    snake.game = (function()
    {
        var canvas = document.getElementById('canvas');
        var ctx = canvas.getContext('2d');
        var status=false;
        var score = 0;
        var old_direction = 'right';
        var direction = 'right';
        var block = 10;
        var score = 0;
        var refresh_rate = 250;
        var pos = [[5,1],[4,1],[3,1],[2,1],[1,1]];
        var scoreboard = document.getElementById('scoreboard');
        var control = document.getElementById('controls');
        var keys = {
            37 : 'left',
            38 : 'up',
            39 : 'right',
            40 : 'down'
            };
        function adjust()
        {
            if (document.fullscreenElement || document.webkitFullscreenElement || document.mozFullScreenElement || document.msFullscreenElement )
            {
                canvas.width=window.innerWidth;
                canvas.height=window.innerHeight;
                control.style.display='none';
            }
            else
            {
                canvas.width=850;
                canvas.height=600;
                control.style.display='inline';
            }
        }
        var food = [Math.round(Math.random(4)*(canvas.width - 10)), Math.round(Math.random(4)*(canvas.height - 10)),];
        function todraw()
        {
            for(var i = 0; i < pos.length; i++)
            {
                draw(pos[i]);
            }
        }
        function giveLife()
        {
            var nextPosition = pos[0].slice();
            switch(old_direction)
            {
                case 'right':
                    nextPosition[0] += 1;
                    break;
                case 'left':
                    nextPosition[0] -= 1;
                    break;
                case 'up':
                    nextPosition[1] -= 1;
                    break;
                case 'down':
                    nextPosition[1] += 1;
                    break;    
            }
            pos.unshift(nextPosition);
            pos.pop();
        }
        function grow()
        {
            var nextPosition = pos[0].slice();
            switch(old_direction)
            {
                case 'right':
                    nextPosition[0] += 1;
                    break;
                case 'left':
                    nextPosition[0] -= 1;
                    break;
                case 'up':
                    nextPosition[1] -= 1;
                    break;
                case 'down':
                    nextPosition[1] += 1;
                    break;    
            }
            pos.unshift(nextPosition);
        }
        function loop()
        {
            ctx.clearRect(0,0,canvas.width,canvas.height);
            todraw();
            giveLife();
            feed();
            if(is_catched(pos[0][0]*block,pos[0][1]*block,block,block,food[0],food[1],10,10))
            {
                score += 10;
                createfood();
                scoreboard.innerHTML = score;
                grow();
                if(refresh_rate > 100)
                {
                    refresh_rate -=5;
                }
            }
            snake.game.status = setTimeout(function() { loop(); },refresh_rate);
        }
        window.onkeydown = function(event){
             direction = keys[event.keyCode];
                if(direction)
                {
                    setWay(direction);
                    event.preventDefault();
                }
            };
        function setWay(direction)
        {
            switch(direction)
            {
                case 'left':
                    if(old_direction!='right')
                    {
                        old_direction = direction;
                    }
                    break;
                case 'right':
                    if(old_direction!='left')
                    {
                        old_direction = direction;
                    }
                    break;
                case 'up':
                    if(old_direction!='down')
                    {
                        old_direction = direction;
                    }
                    break;
                case 'down':
                    if(old_direction!='up')
                    {
                        old_direction = direction;
                    }
                    break;
            }

        }
        function feed()
        {
            ctx.beginPath();
            ctx.fillStyle = "#ff0000";
            ctx.fillRect(food[0],food[1],10,10);
            ctx.fill();
            ctx.closePath();
        }
        function createfood()
        {
            food = [Math.round(Math.random(4)*850), Math.round(Math.random(4)*600)];
        }
        function is_catched(ax,ay,awidth,aheight,bx,by,bwidth,bheight) {
            return !(
            ((ay + aheight) < (by)) ||
            (ay > (by + bheight)) ||
            ((ax + awidth) < bx) ||
            (ax > (bx + bwidth))
            );
        }
        function draw(pos)
        {
            var x = pos[0] * block;
            var y = pos[1] * block;
            if(x >= canvas.width || x <= 0 || y >= canvas.height || y<= 0)
            {
                    document.getElementById('pause').disabled='true';
                    snake.game.status=false;
                    ctx.clearRect(0,0,canvas.width,canvas.height);
                    ctx.font='40px san-serif';
                    ctx.fillText('Game Over',300,250);
                    ctx.font = '20px san-serif';
                    ctx.fillStyle='#000000';
                    ctx.fillText('To Play again Refresh the page or click the Restarts button',200,300);
                    throw ('Game Over');
            }
            else
            {
                ctx.beginPath();
                ctx.fillStyle='#000000';
                ctx.fillRect(x,y,block,block);
                ctx.closePath();
            }
        }
        function pause(elem)
        {
            if(snake.game.status)
            {
                clearTimeout(snake.game.status);
                snake.game.status=false;
                elem.value='Play'
            }
            else
            {
                loop();
                elem.value='Pause';
            }
        }
        function begin()
        {
            loop();
        }
        function restart()
        {
            location.reload();
        }
        function start()
        {
            ctx.fillStyle='#000000';
            ctx.fillRect(0,0,canvas.width,canvas.height);
            ctx.fillStyle='#ffffff';
            ctx.font='40px helvatica';
            ctx.fillText('Vignesh',370,140);
            ctx.font='20px san-serif';
            ctx.fillText('presents',395,190);
            ctx.font='italic 60px san-serif';
            ctx.fillText('Feed The Snake',240,280);
            var img = new Image();
            img.onload = function()
            {
                ctx.drawImage(img,300,300,200,200);
                ctx.fillRect(410,330,10,10);
            }
            img.src ='snake.png';
        }
        function fullscreen()
        {
            launchFullscreen(canvas);
        }
        return {
            pause: pause,
            restart : restart,
            start : start,
            begin: begin,
            fullscreen : fullscreen,
            adjust : adjust,
        };
    })();
    snake.game.start();
}
</script>
</head>
<body>
<canvas width="850" height="600" id="canvas" style="border:1px solid #333;" onclick="snake.game.begin();">
</canvas>
<div id="controls" style="float:right; text-align:center;">
    <input type="button" id="pause" value="Play" onClick="snake.game.pause(this);" accesskey="p">
    <input type="button" id="restart" value="Restart" onClick="snake.game.restart();">
    <br/><br/>
    <input type="button" id="fullscreen" value="Play Fullscreen" onClick="snake.game.fullscreen();">
    <br/><br/>
    <div style="font-size:24px;">
    Score : 
    <span id="scoreboard">0</span>
    </div>
</div>
</body>
</html>

You can see a live version of the game here.

share|improve this question
3  
I don't get a game over if the snake hits itself; only if it hits the walls. Makes it a little too easy :) –  Flambino Jun 26 at 14:41
    
@Flambino oh thanks, I forgot that. –  Vignesh Jun 27 at 4:06

4 Answers 4

up vote 6 down vote accepted

From a once over:

Good

  • I like how you use an IIFE
  • I really like how you use direction = keys[event.keyCode];

Not so good

  • You are not consistently applying the 2nd good technique, for example this:

    function setWay(direction)
    {
        switch(direction)
        {
            case 'left':
                if(old_direction!='right')
                {
                    old_direction = direction;
                }
                break;
            case 'right':
                if(old_direction!='left')
                {
                    old_direction = direction;
                }
                break;
            case 'up':
                if(old_direction!='down')
                {
                    old_direction = direction;
                }
                break;
            case 'down':
                if(old_direction!='up')
                {
                    old_direction = direction;
                }
                break;
        }
    
    }
    

    could simply have been

    function setWay(direction)
    {
      var oppositeDirection = {
        left : 'right',
        right: 'left',
        up: 'down',
        down:'up'
      }
    
      if( direction != oppositeDirection[old_direction] ){
        old_direction = direction;
      }
    }
    

    I will leave deep thoughts to on whether

    • You want to specify that 'left' is the opposite of 'right', since you already specified 'right' is the opposite of 'left'
    • Whether you would want to merge oppositeDirection and keys
  • You copy pasted some code in giveLife and grow that could also benefit from the above approach. I would have written this:

        switch(old_direction)
        {
            case 'right':
                nextPosition[0] += 1;
                break;
            case 'left':
                nextPosition[0] -= 1;
                break;
            case 'up':
                nextPosition[1] -= 1;
                break;
            case 'down':
                nextPosition[1] += 1;
                break;    
        }
    

    as

    //2 properly named array indexes for x and y
    var X = 0; 
    var Y = 1;
    //vectors for each direction
    var vectors = {
      right : { x : 1 , y : 0 },
      left  : { x : -1 , y : 0 },
      up    : { x : 0 , y : -1 },
      down  : { x : 0 , y : 1 }
    }
    
    function updatePosition( direction ){
    
      var vector = vectors( direction );
      if( vector ){
        nextPosition[X] += vector.x;
        nextPosition[Y] += vector.y;
      }
      else{
        throw "Invalid direction: " + direction
      }
    }
    

    The advantages here are:

    • If you wanted to play snake with 8 directions, you could
    • No silent failure if an invalid direction is passed along
  • The following code gives me the creeps:

    function launchFullscreen(element) {
      if(element.requestFullscreen) {
        element.requestFullscreen();
      } else if(element.mozRequestFullScreen) {
        element.mozRequestFullScreen();
      } else if(element.webkitRequestFullscreen) {
        element.webkitRequestFullscreen();
      } else if(element.msRequestFullscreen) {
        element.msRequestFullscreen();
      }
    }
    

    have you considered using something like

    function launchFullscreen(e) {
      var request = e.requestFullscreen || 
                    e.mozRequestFullScreen || 
                    e.webkitRequestFullscreen || 
                    e.msRequestFullscreen;
      request();
    }
    
  • This is also is not a pretty sight:

    document.addEventListener("fullscreenchange", function(){snake.game.adjust();});
    document.addEventListener("webkitfullscreenchange", function(){snake.game.adjust();});
    document.addEventListener("mozfullscreenchange", function(){snake.game.adjust();});
    document.addEventListener("MSFullscreenChange", function(){snake.game.adjust();});
    

    That should at least be

    document.addEventListener("fullscreenchange",       snake.game.adjust );
    document.addEventListener("webkitfullscreenchange", snake.game.adjust );
    document.addEventListener("mozfullscreenchange",    snake.game.adjust );
    document.addEventListener("MSFullscreenChange",     snake.game.adjust );
    

    and really there has to be a better way than to subscribe to every browser event ;) I am assuming you are not simply providing snake.game.adjust because it is not yet initialized at that point. I would rather solve that problem then creating functions to deal with that problem.

share|improve this answer

Some thoughts on your general code style (some points might depend on personal preference):

  1. I suggest splitting HTML/CSS/JS into different files
  2. Your use of indention and whitespaces is inconsistent

    function launchFullscreen(element) {
      if(element.requestFullscreen) {
        element.requestFullscreen();
    

    has an indention of two spaces

        snake.game = (function()
        {
            var canvas = document.getElementById('canvas');
    

    has an indention of four spaces

            if(x >= canvas.width || x <= 0 || y >= canvas.height || y<= 0)
            {
                    document.getElementById('pause').disabled='true';
                    snake.game.status=false;
    

    has an indention of eight spaces

    var status=false; // no spaces before/after '='
    var block = 10; // space before/after '='
    
  3. You have two times:

    var score = 0;
    
  4. Method names are inconsistent is_catched, setWay, todraw.

  5. Consider writing constants in uppercase to differentiate them from variables you intend to modify: BLOCK instead of block or in this case something like BLOCK_SIZE is more appriopriate

  6. You declare and assign your food variable the first time somewhere between all the functions, although you have a creatfood method

  7. There are several magic numbers, that you could/should turn into variables

  8. Some parameter names are rather cryptic: is_catched(ax,ay,awidth,aheight,bx,by,bwidth,bheight)

  9. You could use objects instead of arrays for some variables. E.g.: food is an array with 2 elements (probably x/y pos). You could turn this into an object { x: XXX, y: XXX }. That might improve readability in some places.

  10. Currently your update logic seems to be mixed with your draw logic. It is probably better (and easier to maintain), if you seperate these. Also, you check for game over inside your draw call...

share|improve this answer
2  
"Cached" in "is_catched" should probably be "caught" as well. –  Stephan Jun 27 at 6:58

I would suggest drawing the snake as a single polyline instead of as a bunch of blocks, so that you only call stroke once instead of calling fillRect as many times as blocks has the snake.

In general, it would be more efficient to stroke one complex shape instead of a bunch of simple ones. You can see that in this test.

Another option that I would consider is to clearRect only the last block of the snake and then draw (fillRect) only the new one, so that you don't have to redraw all the scene, only the parts that have changed. Another test here.

You will have to test both options and see which is better for you, but I would go for the second one.

I would also consider using requestAnimationFrame insted of setTimeout. From the MDN doc:

The window.requestAnimationFrame() method tells the browser that you wish to perform an animation and requests that the browser call a specified function to update an animation before the next repaint. The method takes as an argument a callback to be invoked before the repaint.

Also check this post about that. It explains the basics and also how to set a custom frame rate, so that you can still use refresh_rate in your code.

There are two options:

Wrapping the requestAnimationFrame in a setTimeout:

function draw() {
    setTimeout(function() {
        requestAnimationFrame(draw);
        // Drawing code goes here
    }, customTime);
}

Or using deltas:

var time;
function draw() {
    requestAnimationFrame(draw);
    var now = new Date().getTime(),
        dt = now - (time || now);

    time = now;

    // Drawing code goes here... for example updating an 'x' position:
    this.x += 10 * dt; // Increase 'x' by 10 units per millisecond
}

In your case I think it would be better the first option because the snake can only be drawn in certain blocks, so it doesn't make much sense to me to use deltas here.

share|improve this answer

To @Sykin's answer, I would add:

Bug

If you press down and then left faster than the snake takes to slither "one step", the snake turns around and slithers over itself.

Indentation, code style, and code consistency

I find that JSLint really helps me get a consistent code style.

Strict mode

I suggest you use strict mode in your JS, by starting with a line "use strict";

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.