8
\$\begingroup\$

This is my very first program in JavaScript. I am trying to teach myself from JavaScript: The Definitive Guide, 6th Edition.

The code is also available on my bitbucket.

I decided to remake the classical game Xonix as an exercise. I took some concepts from this video.

I would like to ask you for advice in:

  1. Style: I really have no idea how should good JavaScript look like. Please, could you point out the most serious flaws in my code?

    My mentality is blocked in the C++ frame of reference and I do not truly get the entire prototype based inheritance. My code looks somewhat sick to me. There are too many uses of this, but I do not know how to cure this. Also I miss the separation of code to headers and sources and find it hard to organize my code.

  2. Performance: The game seems to run good and fluently on Chrome, but on Firefox 35.0.1 for Ubuntu it

    • runs 2 times slower
    • feels jittery
    • caps the CPU at 100%
    • sometimes causes "too much recursion error"

I tried to profile the code on Firefox and 99.8% of time is spent on rendering. Also, I believe that the source of too much recursion is the Game.floodFill function. Is it possible, that a simple floodFill would cause this problem? How can I speed up the rendering?

I tried to refactor the code to be as small and as readable as possible, but I think I am just making it worse at this point. I do not expect you to read it all, but I would be very grateful if you pointed out the most serious flaws.

index.html:

<!DOCTYPE html>
<html>
<head lang="en">
    <meta charset="UTF-8">
    <title></title>
</head>
<body>
<canvas id="canvas" width=640 height=480></canvas>
<script src="main.js"></script>
</body>
</html>

main.js

;(function()
{
    var scale = 10;
    var nMonsters = 3;
    var KEYS = {LEFT : 37, UP : 38, RIGHT : 39, DOWN : 40};

    var randomInt = function(up, down)
    {
        return Math.round(down + Math.random() * (up - down));
    };
    var flip = function()
    {
        return Math.random() < 0.5;
    };

    this.running = true;
    var Game = function (canvasId)
    {
        var canvas = document.getElementById(canvasId);
        this.c = canvas.getContext("2d");
        canvas.width  = this.width * scale;
        canvas.height = this.height * scale;
        this.player = new Player(this.width / 2, 0, this);
        this.keyBoard = new KeyboardController;

        this.field = new Array(this.width);
        for (var i = 0; i < this.width; ++i)
        {
            this.field[i] = new Array(this.height);
        }

        this.FIELD = {EMPTY : 0, FULL : 1, INCONSTRUCTION : 2, RIGHT_EVALUATED : 3, LEFT_EVALUATED : 4};
        this.COLORS = ["black", "green", "yellow", "magenta", "blue"];

        for (var i = 0; i < this.field.length; ++i)
        {
            for (var j = 0; j < this.field[i].length; ++j)
            {
                if (i < 3 || j < 3 || i >= this.width - 3 || j >= this.height - 3)
                {
                    this.field[i][j] = this.FIELD.FULL;
                }
                else
                {
                    this.field[i][j] = this.FIELD.EMPTY;
                }
            }
        }

        this.monsters = [];
        for (var i = 0; i < nMonsters; ++i)
        {
            var min = 10;
            this.monsters.push(new Monster(randomInt(min, this.width  - min),
                                           randomInt(min, this.height - min),
                                           flip() ? -0.5 : 0.5,
                                           flip() ? -0.5 : 0.5,
                                           this));
        }

        var self = this;
        var tick = function()
        {
            if (!this.running)
            {
                return;
            }
            self.update();
            self.render();
            requestAnimationFrame(tick);
        };
        tick();
    };

    Game.prototype =
    {
        height: 64,
        width: 100,

        update: function ()
        {
            switch (this.keyBoard.lastKey)
            {
                case  KEYS.DOWN:
                    if (this.keyBoard.keysPressed[KEYS.DOWN]  || this.player.running)
                    {
                        this.player.moveDown();
                    }
                    break;
                case  KEYS.RIGHT:
                    if (this.keyBoard.keysPressed[KEYS.RIGHT] || this.player.running)
                    {
                        this.player.moveRight();
                    }
                    break;
                case  KEYS.UP:
                    if (this.keyBoard.keysPressed[KEYS.UP]    || this.player.running)
                    {
                        this.player.moveUp();
                    }
                    break;
                case  KEYS.LEFT:
                    if (this.keyBoard.keysPressed[KEYS.LEFT]  || this.player.running)
                    {
                        this.player.moveLeft();
                    }
                    break;
            }

            var i = Math.round(this.player.x);
            var j = Math.round(this.player.y);
            if (this.field[i][j] === this.FIELD.INCONSTRUCTION || this.hitByMonsters())
            {
                this.lose();
            }

            if (this.field[i][j] === this.FIELD.EMPTY)
            {
                this.player.running = true;
                switch (this.keyBoard.lastKey)
                {
                    case KEYS.DOWN:
                        this.player.leftPoints.push({x: i + 1, y: j});
                        this.player.rightPoints.push({x: i - 1, y: j});
                        break;
                    case KEYS.UP:
                        this.player.leftPoints.push({x: i - 1, y: j});
                        this.player.rightPoints.push({x: i + 1, y: j});
                        break;
                    case KEYS.RIGHT:
                        this.player.leftPoints.push({x: i, y: j - 1});
                        this.player.rightPoints.push({x: i, y: j + 1});
                        break;
                    case KEYS.LEFT:
                        this.player.leftPoints.push({x: i, y: j + 1});
                        this.player.rightPoints.push({x: i, y: j - 1});
                        break;
                }

                this.field[i][j] = this.FIELD.INCONSTRUCTION;
            }
            if (this.field[i][j] === this.FIELD.FULL)
            {
                if (this.player.running)
                {
                    this.computeFill();
                    this.player.leftPoints.length = 0;
                    this.player.rightPoints.length = 0;
                }
                this.player.running = false;
            }

            for (var i = 0; i < nMonsters; ++i)
            {
                this.monsters[i].move();
            }
        },

        render: function ()
        {
            for (var i = 0; i < this.field.length; ++i)
            {
                for (var j = 0; j < this.field[i].length; ++j)
                {
                    this.fillField(i, j, this.COLORS[this.field[i][j]]);
                }
            }
            this.fillField(this.player.x, this.player.y, this.player.color);
            for (var i = 0; i < this.monsters.length; ++i)
            {
                this.fillField(this.monsters[i].x, this.monsters[i].y, this.monsters[i].color);
            }
        },

        fillField: function (x, y, color)
        {
            this.c.fillStyle = color;
            this.c.fillRect(x * scale, y * scale, scale, scale);
        },

        replaceField: function (before, after)
        {
            for (var i = 0; i < this.field.length; ++i)
            {
                for (var j = 0; j < this.field[i].length; ++j)
                {
                    if (this.field[i][j] === before)
                    {
                        this.field[i][j] = after;
                    }
                }
            }
        },

        floodFill : function(x, y, startingField, targetField)
        {
            var filled = 0;
            if (this.field[x][y] !== startingField)
            {
                return filled;
            }
            this.field[x][y] = targetField;
            ++filled;
            filled += this.floodFill(x+1, y  , startingField, targetField);
            filled += this.floodFill(x  , y+1, startingField, targetField);
            filled += this.floodFill(x-1, y  , startingField, targetField);
            filled += this.floodFill(x  , y-1, startingField, targetField);
            return filled;
        },

        hitByMonsters : function()
        {
            return !this.noMonstersIn(this.FIELD.INCONSTRUCTION);
        },

        noMonstersIn : function(fieldType)
        {
            for (var i = 0; i < this.monsters.length; ++i)
            {
                if (this.field[this.monsters[i].xPos]
                              [this.monsters[i].yPos] === fieldType)
                {
                    return false;
                }
            }
            return true;
        },

        lose : function ()
        {
            self.running = false;
        },

        computeFill : function()
        {
            this.replaceField(this.FIELD.INCONSTRUCTION, this.FIELD.FULL);

            var leftStartingPoint = null;
            for (var i = 0; i < this.player.leftPoints.length; ++i)
            {
                if (this.field[this.player.leftPoints[i].x][this.player.leftPoints[i].y] === this.FIELD.EMPTY)
                {
                    leftStartingPoint = this.player.leftPoints[i];
                    break;
                }
            }
            var leftSize = leftStartingPoint ? this.floodFill(leftStartingPoint.x,
                                                              leftStartingPoint.y,
                                                              this.FIELD.EMPTY,
                                                              this.FIELD.LEFT_EVALUATED) : 0;

            var rightStartingPoint = null;
            for (var i = 0; i < this.player.rightPoints.length; ++i)
            {
                if (this.field[this.player.rightPoints[i].x][this.player.rightPoints[i].y] === this.FIELD.EMPTY)
                {
                    rightStartingPoint = this.player.rightPoints[i];
                    break;
                }
            }
            var rightSize = rightStartingPoint ? this.floodFill(rightStartingPoint.x,
                                                                rightStartingPoint.y,
                                                                this.FIELD.EMPTY,
                                                                this.FIELD.RIGHT_EVALUATED) : 0;

            rightSize = this.noMonstersIn(this.FIELD.RIGHT_EVALUATED) ? rightSize : Infinity;
            leftSize  = this.noMonstersIn(this.FIELD.LEFT_EVALUATED)  ? leftSize  : Infinity;

            if (rightSize === Infinity && leftSize === Infinity)
            {
                this.replaceField(this.FIELD.RIGHT_EVALUATED, this.FIELD.EMPTY);
                this.replaceField(this.FIELD.LEFT_EVALUATED,  this.FIELD.EMPTY);
            }
            else if (rightSize > leftSize)
            {
                this.replaceField(this.FIELD.LEFT_EVALUATED,  this.FIELD.FULL);
                this.replaceField(this.FIELD.RIGHT_EVALUATED, this.FIELD.EMPTY);
            }
            else
            {
                this.replaceField(this.FIELD.LEFT_EVALUATED,  this.FIELD.EMPTY);
                this.replaceField(this.FIELD.RIGHT_EVALUATED, this.FIELD.FULL);
            }
        }
    };


    var Player = function (x, y, game)
    {
        this.maxX = game.width  - 1;
        this.maxY = game.height - 1;

        this.x = x;
        this.y = y;
        this.running = false;
        this.color = "red";
        this.leftPoints  = [];
        this.rightPoints = [];
    };

    Player.prototype =
    {
        moveLeft : function()
        {
            if (this.xPos > 0)
            {
                --this.x;
            }
        },
        moveRight : function()
        {
            if (this.xPos < this.maxX)
            {
                ++this.x;
            }
        },
        moveUp : function()
        {
            if (this.yPos > 0)
            {
                --this.y;
            }
        },
        moveDown : function()
        {
            if (this.yPos < this.maxY)
            {
                ++this.y;
            }
        },

        get xPos () {return Math.round(this.x);},
        get yPos () {return Math.round(this.y);}
    }

    var Monster = function (x, y, dx, dy, game)
    {
        this.game = game;
        this.x = x;
        this.y = y;
        this.dx = dx;
        this.dy = dy;
        this.color = "white";
    };

    Monster.prototype =
    {
        move : function()
        {
            var thisX = Math.round(this.x);
            var thisY = Math.round(this.y);
            var nextX = Math.round(this.x + this.dx);
            var nextY = Math.round(this.y + this.dy);

            if (this.game.field[thisX][nextY] === this.game.FIELD.FULL)
            {
                this.dy = -this.dy;
            }
            if (this.game.field[nextX][thisY] === this.game.FIELD.FULL)
            {
                this.dx = -this.dx;
            }
            this.x += this.dx;
            this.y += this.dy;
        },

        get xPos () {return Math.round(this.x);},
        get yPos () {return Math.round(this.y);}
    };

    var KeyboardController = function()
    {
        this.keysPressed = {};
        this.lastKey = null;
        var self = this;
        window.onkeydown = function(e)
        {
            self.keysPressed[e.keyCode] = true;
            self.lastKey = e.keyCode;
        };
        window.onkeyup = function(e)
        {
            self.keysPressed[e.keyCode] = false;
        };
    };


    window.onload = function()
    {
        var game = new Game("canvas");
    };
})()
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Stylewise, your code looks fantastic to me! By the way, would also suggest you read JavaScript: The Good Parts by Douglas Crockford as it explains prototype inheritance as well as the pitfalls to watch for when coding js. \$\endgroup\$
    – jsanc623
    Commented Feb 10, 2015 at 16:02
  • \$\begingroup\$ I would most definitely remove the recursion if you're having performance problems. The rest of your code seems to be pretty efficient. If that doesn't help look into Math.Random and Math.Round alternatives? I know there's a bitshift alternative to rounding. \$\endgroup\$
    – Shelby115
    Commented Feb 20, 2015 at 4:09
  • \$\begingroup\$ @Shelby115 You certainly seem to have a lot of tips for performance! Why don't you post an answer and provide further explanation? I'm sure the OP would appreciate it. \$\endgroup\$
    – SirPython
    Commented Feb 20, 2015 at 15:51
  • \$\begingroup\$ @SirPython I don't feel what I've said is answer worthy. My Math alternatives are merely speculation and the recursion comment is just that, a comment reassuring his already existing suspicion. \$\endgroup\$
    – Shelby115
    Commented Feb 20, 2015 at 17:37
  • \$\begingroup\$ @Shelby115 But then how am I and others supposed to give you reputation for it? Also, in my answer, I never said anything about performance; your answer would give just what the OP needs. \$\endgroup\$
    – SirPython
    Commented Feb 20, 2015 at 17:51

1 Answer 1

5
\$\begingroup\$

Game class

First off, why is it a class? You are only using it once. It makes much more sense to just save the class's fields as normal variables.

However, if you want to keep the object oriented flow, you can make this an object, and access it's fields as like: Game.field

So instead of this:

var Game = function() {
    ...
}

You can have:

var Game = {
    ...
}

But, having it be a class provides benefits, such as being able to have a field/method be either public or private. It's up to you which one you go with.

Second of all, the Game class does a lot of work in it's constructor. A constructor is meant to declare and define different fields and methods; your Game constructor does not do this. To aid that, it would be better to put all that set up into a function that initializes everything, and starts the game.

To add to that, everything is very spread out. In your class constructor, you should have all of your fields at the top, and all the methods at the bottom. You seem to be throwing things all over the place.

That being said, you class constructor would now look like this:

var Game = function (canvasId)
{
    var canvas = document.getElementById(canvasId);
    this.c = canvas.getContext("2d");
    canvas.width  = this.width * scale;
    canvas.height = this.height * scale;
    this.player = new Player(this.width / 2, 0, this);
    this.keyBoard = new KeyboardController;
    this.field = new Array(this.width);
    this.FIELD = {EMPTY : 0, FULL : 1, INCONSTRUCTION : 2, RIGHT_EVALUATED : 3, LEFT_EVALUATED : 4};
    this.COLORS = ["black", "green", "yellow", "magenta", "blue"];
    this.monsters = [];
    var self = this;

    var tick = function()
    {
        if (!this.running)
        {
            return;
        }
        self.update();
        self.render();
        requestAnimationFrame(tick);
    };

    this.init = function() {
        for (var i = 0; i < this.width; ++i)
        {
            this.field[i] = new Array(this.height);
        }
        for (var i = 0; i < this.field.length; ++i)
        {
            for (var j = 0; j < this.field[i].length; ++j)
            {
                if (i < 3 || j < 3 || i >= this.width - 3 || j >= this.height - 3)
                {
                    this.field[i][j] = this.FIELD.FULL;
                }
                else
                {
                    this.field[i][j] = this.FIELD.EMPTY;
                }
            }
        }


        for (var i = 0; i < nMonsters; ++i)
        {
            var min = 10;
            this.monsters.push(new Monster(randomInt(min, this.width  - min),
                                           randomInt(min, this.height - min),
                                           flip() ? -0.5 : 0.5,
                                           flip() ? -0.5 : 0.5,
                                           this));
        }
        tick();
    }
};

I also made a small extra change; I moved the tick() function call so now, when the game is inited, the game will start after that extra set-up.

KeyboardController class

This class doesn't seem to serve it's full purpose. All it does it set up two event listeners that store some information from the event.

The main problem is that the Game class is doing this class's job. As in, the Game class is doing a lot of checking of the key presses.

To me, that seems like the KeyboardController's job.

Getting and setting

From my knowledge of Java, it is common practice to have getters and setters, rather than leaving the fields open for anything to touch and change.

You do do this in some places, but not in others.

Although, do not create too many getters and setters. For example, if you have a two dimensional array, you don't want to have a getter and setter for each individual array member.

Disclaimer: I'm not sure if the same practice applies to JavaScript.

Indentation

Judging by your post, you come from a C++ background. Also, I can tell by your indentation.

In JavaScript, it is common to put the { on the same line as the signature, if/else statement, etc.

Comments

You don't have any, and that can become a problem, especially with games where you do a bunch of crazy arithmetic on different numbers, coordinates, and other doo-hickeys.

You shouldn't over comment, but at least explain lines that aren't blatantly obvious.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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