On Sat, Jun 11, 2011 at 8:01 PM, Nick Morgan <[email protected]> wrote:
> Hi all
>
> I've written a basic Snake game in JavaScript. It's available on jsFiddle
> here:
> http://jsfiddle.net/skilldrick/bg7UF/ (it's over 300 lines so thought
> a bit long for copy-pasting)
> (Also on Github if anyone's interested: https://github.com/skilldrick/jsSnake)
>
> I'm going to be using this for a blog tutorial on canvas, so I want
> the code to be as clear and 'correct' as possible.
>
> I'd really appreciate any feedback/constructive criticism you guys have.
>
> Cheers!
> --
> Nick Morgan
> http://skilldrick.co.uk
> @skilldrick
>
You can safely ignore comments like "merge var declarations", this is
completely dependent upon your preference of style and none of them is
right or wrong.
First, using array for coordinates has no performance benefit over
objects, and results in much less readable code.
| position[0] & position[1]
| position.x & position.y
This latter is more convenient, and allows more abstraction. You can
abstract coordinates as a datatype with methods that operate on them:
| function Point(x,y) {
| this.x = x;
| this.y = y;
| }
|
| Point.prototype.equals = function(point) {
| return this.x === point.x && this.y === point.y;
| };
| Point.protoype.insideBounds = function(min, max) {
| return this.x <= max.x && this.x >= min.x && this.y <= max.y
&& this.y >= min.y;
| };
And use it as:
| var min = new Point(1, 1);
| var max = new Point(JS_SNAKE.widthInBlocks - 1,
JS_SNAKE.heightInBlocks - 1);
|
| if ( ! head.insideBounds(min, max) ) {
| wallCollision = true;
| }
This is a use-case where a prototype object comes in handy, because
you can connect the method `equalCoordinates` with the data
`cooridnate`, and still have only one instance of the method in
memory.
Switch can be elegantly (and efficently) replaced in bindEvents:
| var arrowKeys = { 37: "left", 38: "up", 39: "right", 40: "down" };
|
| function bindEvents() {
| $('body').keydown(function (event) {
| var direction = arrowKeys[event.which];
| if (direction) {
| snake.setDirection(direction);
| event.preventDefault();
| }
| else if (32 == event.which):
| restart();
| }
| });
| // ...
| }
The same could be done to offsets in `advance(apple)` but is not that crucial:
| var offsets = {
| left : new Point(-1, 0), up : new Point( 0, -1),
| right: new Point( 1, 0), down: new Point( 0, 1)
| };
| var offset = offsets[direction];
| if (offset)
| nextPosition.add(offset);
| else
| throw('Invalid direction');
There may be other areas to investigate, but the code overall (aside
from the uneeded use of jQuery, which makes a simple game like this
resource heavy) seems to be well written to me.
- Balázs
--
To view archived discussions from the original JSMentors Mailman list:
http://www.mail-archive.com/[email protected]/
To search via a non-Google archive, visit here:
http://www.mail-archive.com/[email protected]/
To unsubscribe from this group, send email to
[email protected]
