10
\$\begingroup\$

I just wrote a Tetris game and I'd like to ask experienced people for a code review, especially in terms of code efficiency and bad coding habits, and if the code is easy to reason about.

Here is the Github repo, and here you can play the game.

(function (undefined) {
    'use strict';

    const board = document.querySelector('canvas#board'),
        boardWidth = 200,
        boardHeight = 440;

    board.width = boardWidth;
    board.height = boardHeight;

    const context = board.getContext('2d'),
        brickSize = 20;

    let frameCounter = 0,
        refreshLag = 20,
        boardColor = 'rgb(69,90,100)',
        game = new Game();

    /**
     * Main loop
     */
    function animate() {
        setTimeout(function () {
            requestAnimationFrame(animate);
            game.continue();
            ++frameCounter;
        }, refreshLag);
    }

    animate();

    /**
     * An enum-like object to identify possible actions
     */
    const userActions = Object.freeze({
        ROTATE: 'rotate',
        MOVE_LEFT: 'moveLeft',
        MOVE_RIGHT: 'moveRight',
        MOVE_DOWN: 'moveDown'
    });

    /**
     * Main game logic
     * @returns {Game}
     * @constructor
     */
    function Game() {
        const self = this;

        this.staticBricks = [];
        this.activeShape = new Shape();
        this._playerScore = 0;

        Object.defineProperty(this, 'playerScore', {
            get: function() {
                return self._playerScore;
            },

            set: function(newScore) {
                self._playerScore = newScore;

                [39, 29, 9, 4, 0].some(function (threshold, index) {
                    if (newScore >= threshold) {
                        self.difficulty = 5 - index;

                        return true;
                    }
                });
            }
        });

        this.difficulty = 1;
        this.inputDisabled = false;

        this.checkFilledRegions = function () {
            let rows = [],
                bricks,
                bricksChecked = 0;

            for (
                let i = boardHeight - brickSize;
                bricksChecked !== this.staticBricks.length;
                i -= brickSize
            ) {
                bricks = this.staticBricks.filter(function (brick) {
                    return brick.y === i;
                });

                rows.push({
                    bricks: bricks,
                    isFull: bricks.length === boardWidth / brickSize
                });

                bricksChecked += bricks.length;
            }

            let newBricks = [],
                rowsSkipped = 0;

            for (let i = 0; i < rows.length; ++i) {
                if (rows[i].isFull) {
                    rows[i].bricks = [];
                    ++rowsSkipped;
                    this.playerScore += rowsSkipped;
                } else {
                    rows[i].bricks.forEach(function (brick) {
                        brick.y += rowsSkipped * brickSize;
                    });
                }

                newBricks = newBricks.concat(rows[i].bricks);
            }

            this.staticBricks = newBricks;
        };

        this.drawScore = function () {
            context.fillStyle = 'rgb(255,255,255)';
            context.font="12px Courier";
            context.fillText('Score: ' + this.playerScore, 0, 10);
        };

        this.boardIsFull = function () {
            return this.staticBricks.some(function (brick) {
                return brick.y < brickSize * 2;
            });
        };

        this.gravityIsActive = function () {
            let gameSpeeds = [null, 15, 12, 10, 8, 4];

            return frameCounter % gameSpeeds[this.difficulty] === 0;
        };

        this.drawBackground = function () {
            context.fillStyle = boardColor;
            context.fillRect(0, 0, boardWidth, boardHeight);
        };

        this.continue = function () {
            this.drawBackground();

            if (this.activeShape.isFrozen) {
                for (let i = 0; i < 4; ++i) {
                    this.staticBricks.push(this.activeShape.bricks.pop());
                }

                if (this.boardIsFull()) {
                    this.staticBricks = [];
                    this.playerScore = 0;
                }

                this.checkFilledRegions();
                this.activeShape = new Shape();
            } else {
                if (this.gravityIsActive()) {
                    this.applyAction(userActions.MOVE_DOWN);
                }

                this.activeShape.draw();
            }

            this.drawStaticBricks();
            this.drawScore();
        };

        this.checkCollisions = function (callback) {
            const self = this,
                collisions = Object.seal({
                    left: false,
                    right: false,
                    bottom: false
                });

            function checkAgainst(obstacle, direction) {
                return function (brick) {
                    if (obstacle === 'board') {
                        switch (direction) {
                            case 'bottom':
                                return brick.y === boardHeight - brickSize;
                            case 'left':
                                return brick.x === 0;
                            case 'right':
                                return brick.x === boardWidth - brickSize;
                        }
                    } else {
                        let collision = false;

                        let callback = function (staticBrick) {
                            switch (direction) {
                                case 'bottom': {
                                    collision = collision ||
                                        brick.y === staticBrick.y - brickSize &&
                                        brick.x === staticBrick.x;
                                    break;
                                }

                                case 'left': {
                                    collision = collision ||
                                        brick.y === staticBrick.y &&
                                        brick.x - brickSize === staticBrick.x;
                                    break;
                                }

                                case 'right': {
                                    collision = collision ||
                                        brick.y === staticBrick.y &&
                                        brick.x + brickSize === staticBrick.x;
                                    break;
                                }
                            }
                        };

                        self.staticBricks.forEach(callback);

                        return collision;
                    }
                };
            }

            this.activeShape.bricks.forEach(function (brick) {
                ['bottom', 'left', 'right'].forEach(function (side) {
                    if (
                        checkAgainst('board', side)(brick) ||
                        checkAgainst('static', side)(brick)
                    ) {
                        collisions[side] = true;
                    }
                });
            });

            callback(collisions);
        };

        this.drawStaticBricks = function () {
            this.staticBricks.forEach(function (staticBrick) {
                staticBrick.draw();
            });
        };

        this.applyAction = function (action) {
            self.checkCollisions(function (collisions) {
                self.activeShape.isFrozen = collisions.bottom;

                switch (true) {
                    case action === userActions.MOVE_RIGHT && collisions.right:
                    case action === userActions.MOVE_LEFT && collisions.left:
                    case action === userActions.MOVE_DOWN && collisions.bottom:
                    case action === userActions.ROTATE && cantBeRotated():
                        break;
                    default:
                        self.activeShape.applyMovement(action);

                        break;
                }

                function cantBeRotated() {
                    const temp = new Shape();

                    temp.orientaion = self.activeShape.orientaion;
                    temp.type = self.activeShape.type;

                    for (let i = 0; i < 4; ++i) {
                        Object.assign(
                            temp.bricks[i],
                            self.activeShape.bricks[i]
                        );
                    }

                    temp.applyMovement(userActions.ROTATE);

                    for (let i = 0; i < 4; ++i) {
                        for (let j = 0; j < self.staticBricks.length; ++j) {
                            if (
                                temp.bricks[i].x === self.staticBricks[j].x &&
                                temp.bricks[i].y === self.staticBricks[j].y
                            ) {
                                return true;
                            } else if (
                                temp.bricks[i].x >= boardWidth ||
                                temp.bricks[i].x <= 0 ||
                                temp.bricks[i].y >= boardHeight
                            ) {
                                return true;
                            }
                        }
                    }

                    return false;
                }
            });
        };

        this.enableInput = function () {
            self.inputDisabled = false;
        };

        this.processAction = function (event) {
            const actions = Object.freeze({
                'ArrowLeft': userActions.MOVE_LEFT,
                'ArrowRight': userActions.MOVE_RIGHT,
                'ArrowUp': userActions.ROTATE,
                // todo: implement 'ArrowDown'
            });

            if (!self.inputDisabled) {
                self.applyAction(actions[event.key]);
                self.inputDisabled = true;
                self.checkCollisions(function (collisions) {
                    self.activeShape.isFrozen = collisions.bottom;
                });
            }
        };

        window.addEventListener('keydown', this.processAction);
        window.addEventListener('keyup', this.enableInput);

        return this;
    }

    /**
     * Tetramino data
     * @returns {Shape}
     * @constructor
     */
    function Shape() {
        this.data = {
            types: [
                {
                    name: 'I',
                    matrix: [
                        [0, -1], [0, 1], [0, 2]
                    ]
                },
                {
                    name: 'O',
                    matrix: [
                        [0, 1], [1, 0], [1, 1]
                    ]
                },
                {
                    name: 'Z',
                    matrix: [
                        [0, -1], [-1, 0], [1, -1]
                    ]
                },
                {
                    name: 'S',
                    matrix: [
                        [-1, -1], [0, -1], [1, 0]
                    ]
                },
                {
                    name: 'T',
                    matrix: [
                        [1, 0], [-1, 0], [1, 1]
                    ]
                },
                {
                    name: 'J',
                    matrix: [
                        [1, 0], [-1, 0], [-1, 1]
                    ]
                },
                {
                    name: 'L',
                    matrix: [
                        [1, 0], [-1, 0], [-1, -1]
                    ]
                }
            ],
            orientations: [
                {
                    angle: 0,
                    matrix: [ [1, 0], [0, 1] ]
                }, {
                    angle: 90,
                    matrix: [ [0, -1], [1, 0] ]
                }, {
                    angle: 180,
                    matrix: [ [-1, 0], [0, -1] ]
                }, {
                    angle: 270,
                    matrix: [ [0, 1], [-1, 0] ]
                }
            ],
            colors: [
                {
                    name: 'orange',
                    rgb: 'rgb(239,108,0)'
                },{
                    name: 'red',
                    rgb: 'rgb(211,47,47)'
                }, {
                    name: 'green',
                    rgb: 'rgb(76,175,80)'
                }, {
                    name: 'blue',
                    rgb: 'rgb(33,150,243)'
                }, {
                    name: 'yellow',
                    rgb: 'rgb(255,235,59)'
                }, {
                    name: 'cyan',
                    rgb: 'rgb(0,188,212)'
                }, {
                    name: 'pink',
                    rgb: 'rgb(233,30,99)'
                }, {
                    name: 'white',
                    rgb: 'rgb(224,224,224)'
                }
            ]
        };

        this.startX = boardWidth / 2;
        this.startY = brickSize;
        this.isFrozen = false;
        this.color = randInt(this.data.colors.length);
        this.type = randInt(this.data.types.length);
        this.orientaion = randInt(this.data.orientations.length);
        this.bricks = [];

        this.draw = function () {
            for (let i = 0; i < 4; ++i) {
                this.bricks[i].draw();
            }
        };

        this.applyMovement = function (direction) {
            switch (direction) {
                case userActions.ROTATE:
                    if (this.data.types[this.type].name !== 'O') {
                        if (this.orientaion === 3) {
                            this.orientaion = 0;
                        } else {
                            ++this.orientaion;
                        }

                        this.applyOrientation();
                    }

                    break;
                case userActions.MOVE_DOWN:
                    this.bricks.forEach(function (brick) {
                        brick.y += brickSize;
                    });
                    break;

                case userActions.MOVE_RIGHT:
                case userActions.MOVE_LEFT:
                    for (let i = 0; i < 4; ++i) {
                        if (direction === userActions.MOVE_LEFT) {
                            this.bricks[i].x -= brickSize;
                        } else {
                            this.bricks[i].x += brickSize;
                        }
                    }

                    break;

                default:
                    break;
            }

            return this;
        };

        this.applyOrientation = function () {
            const
                type = this.data.types[this.type].matrix,
                orientation = this.data.orientations[this.orientaion].matrix;

            let oriented = [];

            // Dot product of the data matrix and the orientation matrix
            for (let i = 0; i < 3; ++i) {
                oriented[i] = [];
                for (let j = 0; j < 2; ++j) {
                    oriented[i][j] = 0;
                    for (let k = 0; k < 2; ++k) {
                        oriented[i][j] += type[i][k] * orientation[k][j];
                    }
                }
            }

            const center = this.bricks[0];

            for (let i = 0; i < 3; ++i) {
                this.bricks[i + 1].x = center.x + oriented[i][0] * brickSize;
                this.bricks[i + 1].y = center.y + oriented[i][1] * brickSize;
            }

            return this;
        };

        for (let i = 0; i < 4; ++i) {
            this.bricks.push(new Brick(
                this.startX,
                this.startY,
                this.data.colors[this.color].rgb
            ));
        }

        this.applyOrientation();

        return this;
    }

    /**
     * Tetramino building block
     * @param {Number} x coordinate
     * @param {Number} y coordinate
     * @param {String} rgb color string
     * @returns {Brick}
     * @constructor
     */
    function Brick(x, y, rgb) {
        this.x = x;
        this.y = y;
        this.rgb = rgb;
        this.draw = function() {
            context.fillStyle = this.rgb;
            context.fillRect(
                this.x,
                this.y,
                brickSize - 1,
                brickSize - 1
            );
        };

        return this;
    }

    /**
     * Random integer generator
     * @returns {Number}
     */
    function randInt(max, min) {
        if (min === undefined) {
            min = 0;
        } else {
            min = Math.ceil(min);
        }

        --max;
        max = Math.floor(max);

        return Math.floor(Math.random() * (max - min + 1)) + min;
    }
})();
\$\endgroup\$
  • \$\begingroup\$ Add keys to support mobile \$\endgroup\$ – webdeb Jan 12 '18 at 5:51
  • \$\begingroup\$ @webdeb already working on it :) \$\endgroup\$ – Semyon Fomin Jan 12 '18 at 10:52
  • \$\begingroup\$ I'm not a Tetris expert, but I feel like the pieces rotate in the wrong direction. \$\endgroup\$ – Kruga Jan 16 '18 at 12:47
  • \$\begingroup\$ @Kruga no, actually this is the way original Tetris worked \$\endgroup\$ – Semyon Fomin Jan 17 '18 at 2:10
2
\$\begingroup\$

Keyboard

Games are a unique coding environment and unlike the majority of web apps, games are not event driven code but rather rely on a main loop to control flow. (Though technically the main loop and IO are event driven they are minor and can be abstractly ignored)

It is a common mistake in web based games to use IO event to drive game action, such as keyboard and mouse/touch. These events occur randomly and out of sync with the main loop that is tied to the display refresh rate. The mouse being particularly nasty as mouse events can fire up to 1000 time a second, many times faster than the refresh rate.

The best way to handle IO in web game is to use the IO event to simply maintain the input device's state. Then in the main loop you poll the current state and take action as required.

Example keyboard handler

An example set up for your game.

// one instance of the keyboard object implemented via a singleton
const keyboard = (() => {
    const keys = {    // a list of only the keys you are interested in
                      // This object holds the current state of the key
                      // True and the key is down false and key is up
        ArrowUp : false,
        ArrowDown : false,
        ArrowLeft : false,
        ArrowRight : false,
        Escape : false,
        anyKey : false,  // This is set to true when any key is pressed
                         // The keyboard object does not flip it back
                         // to false, you must do that after responding to
                         // an anykey state 
    };
    const callbacks = {};  // Sometimes you still want event driven
                           // code this holds a keyNamed callback that is called on 
                           // key down

    // the key event listener is as simple as possible.
    // It takes a key event checks if we are monitoring that key
    // If so sets that key's state and prevents default action
    function keyEvents(e){
        const isDown = e.type === "keydown";
        if(keys[e.code] !== undefined){
            keys[e.code] = isDown;
            e.preventDefault();
        }
        keys.anyKey = isDown;

        // If a the event is a down event check if a callback has been
        // set for that key or any key.
        // If the callback returns true that means it no longer 
        // needs the callback and the callback is removed.
        // Note this configuration only allows for ONE callback
        // per key.
        if(isDown){
            if (callbacks.anyKey) {
                if(callbacks.anyKey(e) === true){
                   callbacks.anyKey = undefined;
                }                   
            }
            if (callbacks[e.code]) { 
                if(callbacks[e.code](e) === true){
                   callbacks[e.code] = undefined;
                }                   
            }
        }
    }
    // The interface to the keyboard object (unfortunately the word interface 
    // is a JavaScript reserved word so I use API) 
    // This interface provide access to the key states
    // Functions to start and stop the keyboard and
    // A function to add callback to any named key,
    const API = {
        keys,
        start(){
            addEventListener("keyup", keyEvents);
            addEventListener("keydown", keyEvents);
            return keys;
        },
        stop(){
            removeEventListener("keyup", keyEvents);
            removeEventListener("keydown", keyEvents);
        },
        addCallback (keyName ,callback) { callbacks[keyName] = callback },
    }
    return API;
})();      

In your game you can create a key mapper object so that the keys can be mapped to actions named actions

const keyMap = {
    moveLeft : "ArrowLeft",
    moveRight: "ArrowRight",
    rotate   : "ArrowUp",
    moveDown : "ArrowDown",
}

There are two ways to handle input:

  • A single click like response. Rotate and left right movement
  • Continued action while key is down. Eg the down action

Click responses

In your game you use the click like response as follows:

 // inside the main loop and in the code related to the action

 if(keyboard.keys[keyMap.moveLeft]){ // is the key down?
     // ..code to instigate the action
     keyboard.keys[keyMap.moveLeft] = false; // clear the key so no further
                                             // action is taken
 }
 // I don't use else if. If there are conflicting inputs  let them battle
 // it out
 if(keyboard.keys[keyMap.moveRight]){
     // ..code to instigate the action
     keyboard.keys[keyMap.moveRight] = false;
 }
 if(keyboard.keys[keyMap.rotate]){
     // ..code to instigate the action
     keyboard.keys[keyMap.rotate] = false;
 }

Hold responses

For action that is continued while the key is down you simply poll the key state and take action while it's down:

 if(keyboard.keys[keyMap.moveDown]){
      // increase down speed
      // no need to clear the key state.
  }

Anykey

I have found it very useful to have an anykey flag that is set true for any key down. To use it you must clear it after use. A common use I have for it is the pause unlock. Where any key will unpause the game state.

  // pause cycle handled in main loop
  if(!game.paused){
      if(keyboard.keys[keyMap.pause]){
          keyboard.keys[keyMap.pause] = false; // clear key
          keyboard.keys.anyKey = false; // IMPORTANT clear anykey
          game.paused = true;
      }
  } else {
      if(keyboard.keys.anyKey){
          // clear both pause and ankey
          keyboard.keys[keyMap.pause] = false; // clear key pause key in case
                                               // user uses it to unpause
          keyboard.keys.anyKey = false; // clear anykey
          game.pause = false;

          // If an action key is pressed that action will still be performed
          // as the key's state is unchanged.
          
      }
   }

Key callbacks

I still find that events can be usefully. I specifically use them in menus and game state changes, or if I need to direct action outside the games scope.

   // A debug helper reload page on escape
   keyboard.addCallback("Escape",reloadPage);

   // Or maybe handling some text input
   keyboard.addCallback("anyKey",(event) => {
       if(event.key !== "Enter"){
           // add key to string
       }else{  
           return true; // removes the anykey envent.
       }
   });


Note this is just a simple example, for more sophisticated key input you can use a similar type object to map more complex actions. Eg double tap as a named key keys.ArrowLeft and keys.ArrowLeftDouble mutually exclusive flags.


Summary

The same applies to mouse and touch input. Many times there is a cross over between input devices. Using state object for the IO allows you to easily manage action across IO types. For example you could add a touch listener to the keyboard object that flags the key states depending on the location of the touch. The same with the mouse.

If you used a seeded random you could also then record the whole game by just recording key stated. Saving an entire game with only a few 100 key stocks and times.

\$\endgroup\$
  • \$\begingroup\$ I have removed the first part of the answer that addressed the OP's update to your first answer. Questions aren't supposed to be updated as such after receiving answers. \$\endgroup\$ – Jamal May 8 '18 at 2:33
3
\$\begingroup\$

Decoupled presentation

The function animate is synced to the display, but you are not rendering any content from that function. You are rendering from the timeout callback that is not synced to the display rate.

requestAnimationFrame is used to ensure that any visual content that is rendered by the callback function is held in back-buffers until the next vertical refresh.

For all other functions content that is rendered is immediately presented to the display upon exit. This will create artifacts like shearing (when content is presented mid frame) and flickering (if competing renders are creating content).

Your animation function with comments

// First the comment says main loop yet you call the function animate, is it not better 
// to call the function mainLoop??
/**
 * Main loop
 */
function animate() {
    setTimeout(function () {  
        // This decouples the game from the frame rate.
        // Content rendered in this function is not part of the animation frame and
        // upon exit the new content is presented to the display
        // rather than during the next vertical refresh.
        // You have effectively negated requestAnimationFrame() benefits
        requestAnimationFrame(animate);  
        game.continue();  
        ++frameCounter;
        // function exits and canvas is immediately presented to the display
    }, refreshLag);

    // This function exist from the requested animation frame. 
    // The browser thinks you are done rendering content. As you have
    // not changed any content nothing is done
}

animate();

When you use requestAnimationFrame it should call the function that does the rendering or there is no point in using it.

Quick fix

This is a quick fix and will likely be adequate for your rendering load and the 20ms will skip every other frame.

function mainLoop(time) { // callback is passed microsecond time in milliseconds.
    setTimeout(function () {  
        requestAnimationFrame(mainLoop);  
    }, refreshLag);
    // render content inside callback
    game.continue();  
    frameCounter ++;
}
requestAnimationFrame(mainLoop);  // start via request

Frame rate control

Use this if you have heavy and varying load rendering, i.e. game runs fine at 60fps but when some intermittent FX (That your game absolutely needs!!!) is rendered it drops to 30fps. You can use this to provide a consistent rate equal to the max load (95% of people cant tell 30fps from 60fps to save their lives)

The implmentation is a bit tricky as requestAnimationFrame callback in not actually strongly synced to the display, only back buffer presentation is synced to display vertical refresh (time when the display hardware is not drawing pixels on the screen)

Thus you need to use a little slack in the timing. To get a good picture of what is happening use devTools performance to see when frames get rendered (Note that devTools adds a little overhead so is not perfect picture of timing)

const frameSlack = 16; // There is a little play in the call time.
                       // requestAnimationFrame is not called in sync
                       // with the display, only content presentation 
                       // is synced to display. The function may be called 
                       // at any time during the next frame but likely within
                       // < 2ms of the previous vsync. 
                       // You may have to move this value if you have
                       // other content rendering outside the main loop

                       // If you notice extra frames reduce this value
                       // If you notice dropped frames increase this value
                       // a good value will depend on the rendering load


const frameRate = 30; // only 60, 30,20,15,10...
var lastTime;
function mainLoop(time) { 
    if(frameRate < 60){
        if(time - lastTime < 1000 / frameRate -  frameSlack ){
            requestAnimationFrame(mainLoop); 
            return; // no content rendered and thus no overhead from presentation
        }
        lastTime = time;
    }
    // render content inside callback
    game.continue();  
    frameCounter ++;
    requestAnimationFrame(mainLoop);
}
requestAnimationFrame(mainLoop);  // start via request

Render at 60fps

Or just render at full rate and control speed in the game rather than via the frame rate.

function mainLoop(time) { 
    game.continue();  
    frameCounter ++;
    requestAnimationFrame(mainLoop);
}
requestAnimationFrame(mainLoop);  // start via request

Summing up.

I was going to review more but that is enough from me. I will just say that over all you have done a good job. A few style issue, need to improve object encapsulation, missing some ES6 features, and

// todo: implement 'ArrowDown'

Can't play Tetris without it. :)

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review! ArrowDown is now implemented and some FX too :) What do you think about saving references to this? It sure looks wrong, but bind looks barely better and in case of that playerScore getter/setter thing it's unusable, or am I wrong? \$\endgroup\$ – Semyon Fomin Jan 12 '18 at 12:35

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.