On 12 June 2011 17:12, Balázs Galambosi <[email protected]> wrote:
> 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.
Yes, as I said before, I don't follow JsLint religiously :)
> 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.
Yes, I like that approach - a refactor may be in order! I was striving
for the simplest code possible, but this would definitely clean it up.
>
> 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();
> | }
> | });
> | // ...
> | }
Yes, I had that same thought yesterday while driving, definitely cleaner.
>
> 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');
>
That's definitely cleaner in some ways but adds more complexity, so I
think I'll leave it.
>
> 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)
As I said before, I'm aiming this towards beginner JavaScripters, so I
made a conscious choice to use jQuery, despite the downsides.
> seems to be well written to me.
>
Thanks!
--
Nick Morgan
http://skilldrick.co.uk
@skilldrick
--
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]
