5
\$\begingroup\$

I have recently finished my first iteration of a template method implementation for a JavaScript canvas animation of the snake game. However, I would love for the community to critique this code and inform me where I am going wrong?

In my opinion, a bad smell for me is the number of times this is used throughout. Furthermore, the number of properties in the SnakeAnimator implementation (Note: These properties are passed upon extension).

const VectorFactory = require("./model/vector");
const _ = require("lodash");
const Animator = require('./animators/animator');

let SnakeAnimator = {
    vectors: [],
    canvas: null,
    xAdd: null,
    canvas_context: null,
    canvas_height: 0,
    canvas_width: 0,

    initialise: function (options) {
        this.canvas_context = this.canvas.getContext("2d");
        this.canvas_width = this.canvas.width = window.innerWidth;
        this.canvas_height = this.canvas.height = window.innerHeight;
        this.canvas_context.strokeStyle = options.colour;
    },

    draw: function () {
        this.canvas_context.beginPath();

        for( let i = 0; i < this.vectors.length; i++) {
            this.canvas_context.moveTo(this.vectors[i][0].x, this.vectors[i][0].y);
            this.canvas_context.lineTo(this.vectors[i][1].x, this.vectors[i][1].y);
        }

        this.canvas_context.stroke();
        this.canvas_context.closePath();
    },

    update: function() {
        for (let i = 0; i < this.vectors.length; i++) {
            this.vectors[i][0].x = this.vectors[i][0].x + this.xAdd;
            this.vectors[i][1].x = this.vectors[i][1].x + this.xAdd;
        }
    },

    clear: function() {
        this.canvas_context.clearRect(0, 0, this.canvas_width, this.canvas_height);
    },

    addSnake: function() {
        this.vectors.push([VectorFactory(0, this.canvas_height / 2), VectorFactory(this.xAdd, this.canvas_height / 2)]);
    }
};
_.extend(SnakeAnimator, Animator);

module.exports = SnakeAnimator;

The template method implementation is as follows:

let Animator = {
    animate: function() {
        this.update();
        this.clear();
        this.draw();
    }
};

module.exports = Animator;

I would greatly appreciate any feedback on how I can improve this / my overall knowledge of JavaScript applications should be structured as I have struggled with this for a while.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Good question, I hope you get some great answers! \$\endgroup\$ – Phrancis Feb 4 '16 at 0:26
1
\$\begingroup\$
const VectorFactory = require("./model/vector");
const _ = require("lodash");
const Animator = require('./animators/animator');

Consider using ES6 and imports. That way, your code will be a bit more future-proof. The caveat is that you need to have a build system and a transpiler. Since you have require, I suppose you do have a build system in place. You just need the transpiler.

Now I suppose you use the code this way:

const SnakeAnimator = require('./animators/snake-animator');

const snakeAnimatorInstance = _.extend(SnakeAnimator);

snakeAnimatorInstance.canvas = referenceToCanvas

snakeAnimatorInstance.initialise();

gameLoop(snakeAnimatorInstance.animate);

Looks ok, but it would be nice to automate much of the setup using a factory function that accepts params, and pops it in to a composed object.

const SnakeAnimator = require('./animators/snake-animator');

snakeAnimatorInstance = SnakeAnimator.createInstance({
  canvas: referenceToCanvas,
  // other options for initialization
});

gameLoop(snakeAnimatorInstance.animate);

Now for the properties

vectors: [],
canvas: null,
xAdd: null,
canvas_context: null,
canvas_height: 0,
canvas_width: 0,

Looks like the only public property is canvas. All the others appear to be only used by internal functions like initialize. It would be nice to indicate that they shouldn't be modified directly. By convention, a _ is prefixed to the name.

Also, JS follows the camelCase convention in naming (notice innerWidth and innerHeight). Suggesting you follow for consistency.

\$\endgroup\$
  • \$\begingroup\$ Many thanks for the feedback, all the points are valid and I'll implement them. I'll keep them in mind for the future :) \$\endgroup\$ – Barry McAuley Feb 4 '16 at 15:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.