16
\$\begingroup\$

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.

Pimgd
22.5k5 gold badges68 silver badges144 bronze badges
asked Jun 26, 2014 at 13:34
\$\endgroup\$
7
  • 3
    \$\begingroup\$ I don't get a game over if the snake hits itself; only if it hits the walls. Makes it a little too easy :) \$\endgroup\$ Commented Jun 26, 2014 at 14:41
  • \$\begingroup\$ You should line up the "apple" and the snake on the same line properly so that you can actually see if you're going to eat it or not. \$\endgroup\$ Commented Mar 10, 2015 at 18:10
  • \$\begingroup\$ @Chrillewoodz I don't get it, can you explain more?? \$\endgroup\$ Commented Mar 11, 2015 at 9:45
  • 1
    \$\begingroup\$ This is the apple: - , and when the snake comes for it they line up like this: -_ , instead of: -- . Get it? \$\endgroup\$ Commented Mar 11, 2015 at 13:41
  • 1
    \$\begingroup\$ The demo link is now broken. \$\endgroup\$ Commented Jun 19, 2018 at 1:29

4 Answers 4

9
\$\begingroup\$

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.

answered Jun 26, 2014 at 17:22
\$\endgroup\$
13
\$\begingroup\$

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...

answered Jun 26, 2014 at 14:55
\$\endgroup\$
1
  • 3
    \$\begingroup\$ "Cached" in "is_catched" should probably be "caught" as well. \$\endgroup\$ Commented Jun 27, 2014 at 6:58
4
\$\begingroup\$

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.

answered Jun 26, 2014 at 18:35
\$\endgroup\$
2
\$\begingroup\$

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";

answered Jun 26, 2014 at 17:03
\$\endgroup\$
1
  • \$\begingroup\$ The use strict suggestion needs clarification; "starting with" is vague to my pedantic ear. "Strict" has scope so it really matters where you put it. \$\endgroup\$ Commented Nov 9, 2018 at 17:08

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.