3
\$\begingroup\$

I wrote a basic tic toe game. See, https://jsfiddle.net/shai1436/Lgy1u84s/4/

I am not satisfied with the way I have designed classes and how I have implemented the undo feature. Please suggest feedback.

    //player0 is O and player1 is X
    let board;
    class Game {
        constructor() {
            this.player = 0;
            this.setTurnText(this.player + 1);
            this.setResultText();
        }

        togglePlayer() {
            if (this.player === 1)
                this.player = 0;
            else
                this.player = 1;
            this.setTurnText(this.player + 1);
        }

        setTurnText(player) {
            const ele = document.getElementById('turn-text');
            ele.innerText = 'Player ' + player + ' turn';
        }

        setResultText() {
            const ele = document.getElementById('result-text');
            ele.innerText = ' ';
        }

        declareWinner(player) {
            const ele = document.getElementById('result-text');
            ele.innerText = 'Player ' + player + ' won';
            console.log("player " + player + " won ");
        }

        declareDraw() {
            const ele = document.getElementById('result-text');
            ele.innerText = ' Draw ';
        }
    }

    class Board {
        constructor() {
            this.gameBoard = new Array(new Array(3), new Array(3), new Array(3));
            this.gameStatus = null; // 0: player0 wins, 1: player1 wins, 2: draw, null: undecided
            this.cellsFilled = 0;
            this.findGameStatus = this.findGameStatus.bind(this);
            this.game = new Game();
            this.boardCanvas = new BoardCanvas('canvas');
            this.gameHistory = new Array();
        }

        updateBoard(indices) {
            if (!this.canDraw(indices))
                return;
            this.gameBoard[indices.x][indices.y] = this.game.player;
            this.gameHistory.push(indices);
            this.cellsFilled++;
            this.updateBoardCanvas();
            this.findGameStatus(indices);
            if (this.gameStatus === 0 || this.gameStatus === 1)
                this.game.declareWinner(this.gameStatus + 1);
            else if (this.gameStatus === 2)
                this.game.declareDraw();
            this.game.togglePlayer();
        }

        updateBoardCanvas() {
            this.boardCanvas.drawBoard(this.gameBoard);
        }

        undo() {
            const indices = this.gameHistory.pop();
            this.gameBoard[indices.x][indices.y] = undefined;
            this.updateBoardCanvas();
            this.game.togglePlayer();
            this.cellsFilled--;
        }

        canDraw(indices) {
            const iscellEmpty = this.gameBoard[indices.x][indices.y] === undefined;
            const isGameInProgress = this.gameStatus === null;
            return iscellEmpty && isGameInProgress;
        }

        findGameStatus(indices) {
            if (this._checkRow(indices) ||
                this._checkColumn(indices) ||
                this._checkDiagonal() ||
                this._checkReverseDiagonal()) {
                this.gameStatus = this.game.player;
            }
            else if (this.cellsFilled === 9) {
                this.gameStatus = 2;
            }
        }

        _checkRow(indices) {
            const row = indices.x;
            for (let i = 0; i < 3; i++) {
                if (this.gameBoard[row][i] !== this.game.player)
                    return false;
            }
            return true;
        }

        _checkColumn(indices) {
            const col = indices.y;
            for (let i = 0; i < 3; i++) {
                if (this.gameBoard[i][col] !== this.game.player)
                    return false;
            }
            return true;
        }

        _checkDiagonal() {
            for (let i = 0; i < 3; i++) {
                if (this.gameBoard[i][i] !== this.game.player)
                    return false;
            }
            return true;
        }

        _checkReverseDiagonal() {
            for (let i = 0; i < 3; i++) {
                if (this.gameBoard[i][2 - i] !== this.game.player)
                    return false;
            }
            return true;
        }
    }

    class BoardCanvas {
        constructor(id) {
            this.canvas = document.getElementById(id);
            this.ctx = this.canvas.getContext('2d');
            this.drawBoard();
            this.addClickListener();
        }

        mapIndicesToCanvasCells(x, y) {
            var bbox = this.canvas.getBoundingClientRect();
            const loc = {
                x: x - bbox.left * (canvas.width / bbox.width),
                y: y - bbox.top * (canvas.height / bbox.height)
            };
            loc.x = Math.floor(loc.x / 100) * 100;
            loc.y = Math.floor(loc.y / 100) * 100;
            return loc;
        }

        drawCross(y, x) {
            this.ctx.save();
            this.ctx.translate(x, y);
            this.ctx.beginPath();
            this.ctx.moveTo(20, 20);
            this.ctx.lineTo(80, 80);
            this.ctx.moveTo(80, 20);
            this.ctx.lineTo(20, 80);
            this.ctx.stroke();
            this.ctx.restore();
        }

        drawCircle(y, x) {
            this.ctx.save();
            this.ctx.translate(x, y);
            this.ctx.beginPath();
            this.ctx.arc(50, 50, 30, 0, Math.PI * 2, true);
            this.ctx.stroke();
            this.ctx.restore();
        }

        drawBoard(board) {
            this.clearBoard();
            for (let i = 0; i < 3; i++) {
                for (let j = 0; j < 3; j++) {
                    this.ctx.strokeRect(100 * i, 100 * j, 100, 100);
                    if (board && board[i][j] === 0)
                        this.drawCircle(100 * i, 100 * j);
                    else if (board && board[i][j] === 1)
                        this.drawCross(100 * i, 100 * j);
                }
            }
        }

        addClickListener() {
            this.canvas.onclick = (e) => {
                const loc = this.mapIndicesToCanvasCells(e.clientX, e.clientY);
                const indices = {};
                let temp = loc.x;
                indices.x = Math.floor(loc.y / 100);
                indices.y = Math.floor(temp / 100);
                board.updateBoard(indices);
            }
        }

        clearBoard() {
            this.ctx.clearRect(0, 0, this.canvas.width, this.canvas.height);
        }
    }

    const init = () => {
        board = new Board();
    }

    init();

    const undo = () => {
        board.undo();
    }

    window.init = init;
    window.undo = undo;

\$\endgroup\$
1
  • \$\begingroup\$ Great answer from @radarbob, just wanted to give an example on usage of Array methods: for (let i = 0; i < 3; i++) { if (this.gameBoard[row][i] !== this.game.player) return false; } return true Can be rewritten like: return this.gameBoard[row].every(foo => foo === this.game.player) \$\endgroup\$
    – Mohrn
    Aug 5, 2019 at 16:37

1 Answer 1

4
\$\begingroup\$

Player is confusing

Only 1 player is defined. This spawns the need for confusing code that looks like this.player is 0 then 1 then 2 then 3 and so on. And player-value incrementing is spread over many methods which my spidey sense says "uh-oh, player disconnects ahead!".

changePlayer() { this.player = this.player === 0 ? 1 : 0; }
currentPlayuer() { return this.player; }

Personally, when I write a second method for a given thing I start to consider making a separate class. Classes should be about exposing related functionality. Good classes expose functionality and hide state.


Array Iterator Functions

Read up on Array.map, Array.every, Array.some, et cetera. These will really clean up the array looping.


Class decoupling

Class purpose needs to be more precisely nailed down conceptually. Then the existing coupling will be more evident. Mixing UI functionality into many classes seems universal in my coding experiences. It just too easy to do when updating the screen is a simple one liner.

Game sounds like it should be the overall game manager. It should be coordinating the other objects through their own APIs, but is directly manipulating raw state that should be in other objects such as display.

Board is only the board and should only be aware of its own state - which squares are occupied. But it is also handling display details. gameHistory sounds like high level functionality that belongs in a conceptually higher level class.

BoardCanvas sounds like the place for display functions, but it is not. The DOM and Canvas are conceptually display components for tic-tac-toe and only BoardCanvas should have to use them. BoardCanvas needs an a tic-tac-toe game appropriate API. addClickListener() is a spot-on example of good decoupling.


Board contains a Game or vice versa?

As a general rule have higher level classes contain lower level classes. Board is a low level and thus "stupid" class. Keep it stupid. It should not be coordinating Game - BoardCanvas interaction; which will happen if you invert the containment heirarchy.


undo

 const undo = () => { board.undo(); }

You'll end up naturally writing lots of these "pass through" functions with decoupled classes. This invisible hand of OO, so to speak, will make high level classes read appropriately high level and classes at all levels will be able to "mind their own business".


game flow logic

In the spirit of expressing high level logic, at the highest level, I imagine the game as a loop. Whether this logic is in Game or a new class is a design decision but the overall point is "layers of abstraction" in the application.

// initialze variables, create objects, etc.
var noWinner = true;
...

while (noWinner) {
  ...

  // testing for a winner or tie game should be somewhere in the
  // method we're calling (or method chain it might be calling).
  //  An if-else in this game loop takes away part of the "who won?" logic 
  // from its proper place. 
  noWinner = this.hasWon(currentPlayer());
}

boardCanvas.displayWinner(this.winner);
// I suppose the winner could be "its a tie" 
\$\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.