Tic-Tac-Toe vanilla JS Pseudo OOP

Question

I'm working on building a simple game in vanilla JS (tic-tac-toe). Some weeks ago I created a functional MVP and asked some questions about it (Vanilla JS Tic-Tac-Toe). The code worked, but it was a single monolithic mess and the UI was ugly, so I decided to improve it.

So, I refactored the code. I organized it into three files:

1. main.js - contains the state and controls the game flow
2. UIFunctions.js - UI related (painting cells, cleaning the screen, etc)
3. gameCheckerFunctions - process the state and verifies if we have a winner
4. gameStateFunctions - basically saves moves made and change the current player

So, the questions are:

1. How to implement a MVC pattern? Right now, the state and HTML elements are global variables, but I think this could be stored in state and UI classes Also, I'm having troubles with the concept of controller. Is main.js the code that will be the controller in an MVC pattern?

2. There's a better algorithm to check for the winner?

3. Is it a good looking code? How it could be improved? What looks ugly and could be done in a better way? I really would like to develop good practices from the beginning.

4. Some features that I would like to implement are a CPU player, storing the score in localStorage, remember the player name, auth, two-player network gamming, etc. I'm in the correct way of doing things?

Game can be played https://nabla-f.github.io/

The code:

app.js

// IMPORTS;

import checker from './refactored/gameCheckerFunctions.js';
import game from './refactored/gameStateFunctions.js';
import ui from './refactored/UIFunctions.js';

// CONFIG

const P1_CLASS = 'player1'; // Defined in styles.css
const P2_CLASS = 'player2'; // Defined in styles.css

// STATE

let state = {
    activePlayer: P1_CLASS,
    boardArray: [[null, null, null], [null, null, null], [null, null, null]],
    boardDim: 3,
    moves: 0
}

let view = {
    button: null,
    cells: null,
    resultScreen: {
        screen: null,
        text: null,
        img: null
    }
};


// WAIT FOR THE DOM

window.addEventListener('DOMContentLoaded', (event) => {
    console.log('DOM fully loaded and parsed');
    main();
});

// MAIN FUNCTION

function main() {

    // GET ELEMENTS FROM DOM AND ADD EVENT LISTENERS
    view.button = document.querySelector("#resetBtn");
    view.cells = document.querySelectorAll(".cell");
    view.resultScreen.screen = document.querySelector("#done");
    view.resultScreen.text = document.querySelector('.lead');
    view.resultScreen.img = document.getElementById('winnerImg');
    view.button.addEventListener('click', () => {ui.restartGameUI(view.cells, view.resultScreen)});
    view.button.addEventListener('click', () => {game.restartGame(state, P1_CLASS); console.log(state)});
    view.cells.forEach( cell => cell.addEventListener('click', turn) );
    view.cells.forEach( cell => cell.classList.add('player1Turn') );

    // GAME FLOW FUNCTION EXECUTED EACH TIME A PLAYER MAKES A MOVE
    function turn() {
        console.log(`Now playing: ${state.activePlayer}`)

        let cell = this; // currently clicked cell

        if (!cell.hasAttribute('data-disabled')) {
            game.storeMove(cell, state.boardArray, state.activePlayer);
            state.moves += 1

            ui.paintCell(cell, state.activePlayer);
            
            if (checker.checkWinner(state.boardArray, state.boardDim, state.activePlayer)) {
                ui.paintWinner(view.resultScreen, state.activePlayer)
            }
            else if (checker.checkDraw(state.moves)) {
                ui.paintDraw(view.resultScreen);
            }
            else {
                state.activePlayer = game.changeTurn(state.activePlayer, P1_CLASS, P2_CLASS);
                ui.changeTurnHover(view.cells, state.activePlayer, P1_CLASS, P2_CLASS);
            }
        }
    };
};

UIFunctions.js

// FUNCTIONS

export default class UIFunctions {
    static paintCell(cell, playerClass) {
        console.log('cell clicked: ' + cell.id);
        cell.classList.add(playerClass);
        cell.setAttribute('data-disabled', 'true');
    }
    
    static paintWinner(screen, playerClass) {
        screen.screen.classList.add(`${playerClass}winner`);
        screen.img.classList.add(`${playerClass}win-img`);
    }
    
    static paintDraw(screen) {
        screen.screen.classList.add(`drawgame`);
        screen.text.innerText = '...is a draw...';
    }

    static changeTurnHover(cells, activePlayer, p1Class, p2Class) {
        cells.forEach( cell => cell.classList.remove(`${p1Class}Turn`) );
        cells.forEach( cell => cell.classList.remove(`${p2Class}Turn`) );
        cells.forEach( cell => {
            if (!cell.hasAttribute('data-disabled')) {
                cell.classList.add(`${activePlayer}Turn`);
            }
        })
    }

    static restartGameUI(cells, screen) {
        cells.forEach( cell => cell.classList.remove('player1') );
        cells.forEach( cell => cell.classList.remove('player2') );
        cells.forEach( cell => cell.classList.remove('player2Turn') );
        cells.forEach( cell => cell.classList.add('player1Turn') );
        cells.forEach( cell => cell.removeAttribute('data-disabled') );
        screen.screen.classList.remove('player1winner');
        screen.screen.classList.remove('player2winner');
        screen.screen.classList.remove('drawgame');
        screen.img.classList.remove('player1win-img');
        screen.img.classList.remove('player2win-img');
        screen.text.innerText = 'Winner is ';
    }
}

gameCheckerFunctions.js

export default class gameCheckerFunctions {

    static INDEXES = [0, 1, 2]
    static DIAG_INDEXES = [ [0, 1, 2], [2, 1, 0] ];

    static getValueFromArray(array, row, column) {
        return array[row][column]
    }

    static checkRows(boardArray, boardDim, playerClass) {
        for (let i=0; i < boardDim; i++) {
            let movesInLine = [];
            for (let column of this.INDEXES) {
                movesInLine.push(boardArray[i][column]);
            }

            if ( movesInLine.every( cell => cell == playerClass ) ) {return true;}
        }

        return false;
    }


    static checkColumns(boardArray, boardDim, playerClass) {
        for (let i=0; i < boardDim; i++) {
            let movesInLine = [];
            for (let row of this.INDEXES) {
                movesInLine.push(boardArray[row][i]);
            }

            if ( movesInLine.every( cell => cell == playerClass ) ) {return true;}
        }

        return false;
    }


    static checkDiags(boardArray, playerClass) {
        for (let diag of this.DIAG_INDEXES) {
            let movesInLine = [];
            for (let i=0; i <= 2; i++) {
                let move = boardArray[i][diag[i]];
                movesInLine.push(move);
            }

            if ( movesInLine.every( cell => cell == playerClass ) ) {return true;}
        }

        return false;
    }


    static checkWinner(boardData, boardDim, playerClass) {
        if ( this.checkRows(boardData, boardDim, playerClass)
            || this.checkColumns(boardData, boardDim, playerClass)
            || this.checkDiags(boardData, playerClass) ) {
            console.log("This function works, there's a winner!");
            return true
        }
        else {
            console.log('No winner this time');
            return false
        }
    }

    static checkDraw(moves) {
        if (moves >= 9) {
            return true;
        }
        else {
            return false
        }
    }
    
    constructor() {
        throw new Error(" This class can't be instantiated");
    }
}

gameStateFunctions.js

// FUNCTIONS

export default class gameStateFunctions {
    
   static changeTurn(activePlayer, p1Class, p2Class) {
        //turnFlag = !turnFlag;
        const newPlayer = (activePlayer == p1Class) ? p2Class : p1Class
        return newPlayer
    }
    
    
    static storeMove(cell, boardArray, activePlayer) {
        console.log(`current state in storeMove(): `)
        console.log(boardArray);
        let coordinates = cell.dataset.coord.split(':')
        const [x, y] = coordinates;
        if (boardArray[x][y] === null) {
            boardArray[x][y] = activePlayer;
        }
    }
    

    static restartGame(state, startingPlayerClass) {
        state.activePlayer = startingPlayerClass;
        state.moves = 0;
        state.boardArray = [[null, null, null], [null, null, null], [null, null, null]];
        console.clear();
        console.log('GAME RESTARTED');
    }

    
    constructor() {
        throw new Error(" This class can't be instantiated");
    }
}

index.html

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <script type="module" src="app.js"></script>
    <link rel="stylesheet" href="styles/normalize.css">
    <link rel="stylesheet" href="styles/styles.css">
    <title>Tic-Tac-Toe vanilla JS</title>
</head>
<body>
    <div id="board">
        <div class="cells">
            <div class="cell" id="cell-1" data-coord="0:0"></div>
            <div class="cell" id="cell-2" data-coord="0:1"></div>
            <div class="cell" id="cell-3" data-coord="0:2"></div>
            <div class="cell" id="cell-4" data-coord="1:0"></div>
            <div class="cell" id="cell-5" data-coord="1:1"></div>
            <div class="cell" id="cell-6" data-coord="1:2"></div>
            <div class="cell" id="cell-7" data-coord="2:0"></div>
            <div class="cell" id="cell-8" data-coord="2:1"></div>
            <div class="cell" id="cell-9" data-coord="2:2"></div>
        </div>
        <div id="done">
            <div class="lead">
              Winner is
            </div>
            <div id="winnerImg"></div>
            <button id="resetBtn" type="reset">Play Again</button>
        </div>
    </div>
</body>
</html>

Answer 1

Answering Questions

How to implement a MVC pattern? Right now, the state and HTML elements are global variables, but I think this could be stored in state and UI classes Also, I'm having troubles with the concept of controller. Is main.js the code that will be the controller in an MVC pattern?

First, I want to mention that I love to put all of my state into a single global object like that. I find it to be a very organized way to manage state, and helps developers to keep tabs on all of the potential state of the webpage. Some enterprise libraries, like Redux, actually revolve around the principle of storing the webpage's state into a single object like this. So, don't feel like there's anything wrong with doing that. And, if you want to do it with your view data as well, I don't have a problem with that. (This may not be a very OOP way of doing it, but not everything needs to be OOP)

When it comes to designing webpages, what's important is to have a clear separation of UI and business logic. You're already doing this really well (so, kudos to you), but I will give you a couple more tips on how you can further improve this sort of separation further down. Model-view-controller is one way to achieve this sort of separation of UI and business logic, but it isn't the only way, and to be honest I don't find much value in trying to separate out the controller logic from the view logic, the two are already highly coupled, so such separation often does not provide much benefit.

But, if you're trying to go for the model-view-controller pattern, what you'll want to do is move all of the definitions for your event listeners into their own controller file. Currently, you have them all defined within app.js. These event listeners can be pretty lightweight, where most of what they do is simply make calls to the model or view. The exact way in which the responsibilities of model, view, and controller should be separated is disputed, you'll find different people with different opinions, but as long as your event listeners are found in one region of your application, your view logic in another, and your business logic in another, and your business logic never touches view or controller logic, you should be golden.

There's a better algorithm to check for the winner?

There sure is. Try this one out.

function checkWinner(boardData, playerClass) {
    const threeInARows = [
        // horizontal
        [[0, 0], [0, 1], [0, 2]],
        [[1, 0], [1, 1], [1, 2]],
        [[2, 0], [2, 1], [2, 2]],
        // vertical
        [[0, 0], [1, 0], [2, 0]],
        [[0, 1], [1, 1], [2, 1]],
        [[0, 2], [1, 2], [2, 2]],
        // diagonal
        [[0, 0], [1, 1], [2, 2]],
        [[0, 2], [1, 1], [2, 0]]
    ];

    return threeInARows.some(
        threeInARow => threeInARow.every(([x, y]) => boardData[y][x] === playerClass)
    );
}

While yes, it's possible to derive general-purpose algorithms that check for a three-in-a-row row-wise, column-wise, and on each diagonal, it's not really necessary here. We're dealing with a very small tic-tac-toe board, and it's dead simple to just hard-code all of the different possible three-in-a-rows, and because this is a straightforward solution, it's going to be less likely to be buggy, all while being much simpler to read and understand.

Is it a good looking code? How it could be improved? What looks ugly and could be done in a better way? I really would like to develop good practices from the beginning.

See my suggestions that I present later on. Keep in mind that many of my suggestions are ones that tend to be the most noticeable for experienced developers, but also the least important. For example, I'll mention that you're inconsistent with your use of quotes. Will developers notice this? Some will, yes. Does this problem matter? Not much, inconsistency in quotes does not effect how easy it is to read or maintain the code. Other problems matter a little more, but many of them are just minor tips that don't matter much.

Some features that I would like to implement are a CPU player, storing the score in localStorage, remember the player name, auth, two-player network gamming, etc. I'm in the correct way of doing things?

Most of these add-ons should be relatively straightforward. You may have to rework one thing or another to get it done properly, but that's ok for something like this. However, network gaming is a whole different story. Developing a game that can be played over the internet required a very different overall structure to your game.

First, you're going to have to ask yourself if you're ok with users having the ability to cheat on this game. Will the users just be playing with friends, or will they be playing with strangers? How much is at stake if they end up playing someone who's cheating? As this is tic-tac-toe, perhaps you won't care so much about cheating, as the game is already a completely unfair game that puts a strong bias on the first player.

The reason why I ask this is because this will influence how you architect and write the code. For example, say you design the system so that if you want to take your turn, you simply send a message like "{ type: "OCCUPY_SQUARE", x: 0, y: 1 }" to the server, and the server forwards this message to your opponent. If you care about preventing cheating, you'll additionally have to put checks in place to make sure someone doesn't modify their client's code to send three messages like this in a row, thus winning the game before the opponent has even taken a turn. In some cases, these checks will need to be done on the server. If you don't care about cheat protection (which you probably wouldn't for a game like this), then you can make the server into a simple message-forwarding system, and put the bulk of the logic into the client code, under the assumption that users will not tamper with it, and even if they do, it's not a big deal.

Next, you have to architect your program, so that any action that may eventually need to be sent over the wire is implemented as a JSON-serializable message. Here's the basic structure I like to use for this type of program (there's different ways of doing it, this is just what I've done in the past).

First, I'll make my model export two things - the current state, and an executeAction() function. All state modifications must be done by sending an action (which is just a plain object) to executeAction(). executeAction() will then read this object's properties and decide how to mutate the state.

// --- model.js --- //
export const currentState = { ...initial state... }

export function executeAction(action) {
    if (action.type === 'OCCUPY_SQUARE') {
        const { x, y } = action;
        // ...logic related to occupying a square...
        // (Feel free to break out helper functions, so you don't bloat executeAction())
    } else if (action.type === 'RESET') {
        const { someArg } = action;
        // ...logic related to this action...
    } else {
        throw new Error(`Unknown action type ${action.type}`);
    }
}

At a later point, when you're ready to hook this game up to the internet, it becomes easy to JSON-serialize these actions and send them to your opponent. For example, if you click a square, the event handler will create an action like { type: 'OCCUPY_SQUARE', x: 1, y: 2 } then send that off to some send function. This function will pass the action along to your executeAction() function, causing your own state to be correctly modified, and it will send the action over the internet, to your opponent's client, where some code on their machine will receive the action and send it into their executeAction() function, causing them to end up in the exact same state as you are in.

The next key to this puzzle would be to update the UI based on how the current state was updated. A simple way to do this, would be to simply make it so once you're done calling executeAction() with your desired action, you send the exact same action over to a function exported on the ui side, which we'll call executeUiAction(). The UI will take this action, along with the updated state, and figure out what actually needs to be changed in the UI to bring the UI up to date with what the state looks like.

I usually use a slightly more complicated variation of this pattern that's capable of handling some other issues (the details depend on the project I'm working on), but what I presented should get you pretty far, and would probably accomplish everything you need for this tic-tac-toe game. But, feel free to tweak or even change it drastically as you go along, it's just a rough-draft idea of what you can do to handle online play.

Structural changes

const state = {
    ...
    boardDim: 3,
    ...
};

boardDim doesn't belong in state, as it's never going to change, so it's not "state". If wanted, you could calculate the board size from boardArray instead, by just doing boardArray.length. You could even make a helper function to do this. (Also, boardDim is not the correct name for this. Technically, the dimensions of the board is 2, it's 2d. Perhaps "size" would be a better word?)

Regarding separation of UI from business logic, make sure to keep anything related to the view out of the model. Theoretically, you should be able to rip out the UI and trivially replace it with a completely different one (like, a terminal-based UI), and not have to touch a line of your model's code. Not that this ever happens in real codebases, but this is the sort of separation we're striving to achieve. You seem to generally do good at this, but you do break this here and there, for example, you tend to pass around variables that are named playerClass or startingPlayerClass, alluding to CSS classes, but for all the model knows, CSS may not even be a thing in the environment its running in. I would instead do something along these lines in your model:

export PLAYER = {
    player1: 'player1',
    player2: 'player2',
}

This is an "enum" (not really, JavaScript doesn't have enums, but for all intents and purposes we can treat this sort of construct just like an enum). This "enum" will be located inside the model. Your model functions can be defined to expect a value from this enum. Your UI logic can then translate these enum values to CSS class names, or it can even choose to use the enum values as a CSS class name if it so wants.

In a similar vein, storeMove() shouldn't accept an HTML element as a parameter, since it shouldn't even know that it's running in an environment where HTML is available. The parsing of the HTML element's data attribute should be done on the UI logic side.

From what it looks like, your app.js is mostly UI logic, which is just fine. If this is what you're going for, then go ahead and move all business-logic related stuff out of there (I'm mostly referring to the state object), and move it into the modules that are dedicated to housing the business logic. And, again, if you want to split your UI logic in two, where all event listeners live separately from the rest of the view logic, that's fine too.

Misc improvements

Most of your modules are structured like this:

export default class UIFunctions {
    static changeTurn(...) { ... }
    static storeMove(...) { ... }
    static restartGame(...) { ... }
    constructor() {
        throw new Error(" This class can't be instantiated");
    }
}

If you don't intend for the class to ever be constructed, then you're using the wrong tool. A direct alternative would be to just use an object literal, like this:

export default {
    changeTurn(...) { ... },
    storeMove(...) { ... },
    restartGame(...) { ... }
};

But, for these scenarios, an even better solution would be to just export the functions directly from the module, like this:

export function changeTurn(...) { ... }
export function storeMove(...) { ... }
export function restartGame(...) { ... }

Make sure to mark something as private if there's no reason to publicly expose it. For example, in gameCheckerFunctions, you can make functions like checkRows() private by placing a # character in front (see this page for more info). Alternatively, if you decide to follow the above recommendation and change this class into a bunch of exported functions, you can simply choose to not export functions like checkRows() to make them private to the module.

Try to make your program have a single source of truth for all state/data. If a piece of state can be derived from another piece of state, then create a function to do this transformation and use that function, this is usually simpler than trying to keep two separate pieces of state in sync with each other. One example of extra state is the data-disabled attributes you place on the cell elements. The cells are aware of their coordinates, and you store in the state object who's in what cell, so you can already figure out via other means which cells should be disabled, you don't need this additional data-disabled attribute.

Never use ==, always use === instead.

Minor Details

Take out the console.log()s. Generally, console.log() is used for debugging purposes, not for verbose logging of what your program is doing. In fact, flooding the logs with these sorts of logs can make debugging more difficult, because it's hard to see the logs you care about amidst the myriad of other logs being generated. (If you personally find this sort of logging useful, then by all means continue to do so on your personal projects, but I would refrain from doing it in shared codebases).

Instead of having multiline if conditions, I like to break the condition out into a separate variable, for example, instead of this:

if ( this.checkRows(boardData, boardDim, playerClass)
    || this.checkColumns(boardData, boardDim, playerClass)
    || this.checkDiags(boardData, playerClass) ) {
    ...
}

I find it more readable to do this:

const hasWon = (
    this.checkRows(boardData, boardDim, playerClass)
    || this.checkColumns(boardData, boardDim, playerClass)
    || this.checkDiags(boardData, playerClass
);

if (hasWon) {
    ...
}

You've got a couple of functions that are structured like this:

static checkDraw(moves) {
    if (moves >= 9) {
        return true;
    }
    else {
        return false;
    }
}

Remember that operators such as >= return a boolean already, so this whole function can be rewritten as follows:

static checkDraw(moves) {
    return moves >= 9;
}

You have this line of code:

let cell = this; // currently clicked cell

Instead of writing a comment explaining what this variable is, just change the name of the variable to be more self-explanatory.

let currentlyClickedCell = this;

The getValueFromArray() function is dead code.

Stuff that really doesn't matter

You're missing a number of semicolons. And, after function myFnName() {} a semicolon is not needed.

Be consistent with your use of single/double quotes, and with let/const. You seem to use these all randomly.

As an overwhelming common convention among JavaScript developers, class names start with an upper-case letter (instead of gameStateFunctions do GameStateFunctions).

You've probably been told to always use {} after an if. This is good advice, however, it's unnecessary if you choose to place a single statement on the same line as the if.

// Instead of this:
if ( movesInLine.every( cell => cell === playerClass ) ) {return true;}

// Just do this:
if ( movesInLine.every( cell => cell === playerClass ) ) return true;

Update: Answering comments

How would I separate out controller logic?

Here's an extremely simple, concrete example that shows how one might separate view, model, and controller logic from a very simple webpage.

// MODEL //

const model = {
  state: { counter: 0 },
  moveCountBy(delta) {
    model.state.counter += delta;
  },
};

// CONTROLLER //

const controller = {
  onIncrement() {
    model.moveCountBy(1);
    view.updateCounterDisplay();
  },
  onDecrement() {
    model.moveCountBy(-1);
    view.updateCounterDisplay();
  },
};

// VIEW //

let view;

{
  const decrementEl = document.getElementById('decrement');
  const incrementEl = document.getElementById('increment');
  const counterDisplayEl = document.getElementById('counter-display');
  
  decrementEl.addEventListener('click', controller.onDecrement);
  incrementEl.addEventListener('click', controller.onIncrement);
  
  view = {
    updateCounterDisplay() {
      counterDisplayEl.innerText = model.state.counter;
    },
    init() {
      view.updateCounterDisplay();
    }
  };
}

// MAIN //

view.init();

<button id="decrement">-</button>
<span id="counter-display"></span>
<button id="increment">+</button>

Don't take this example as doctrine. It's just a rough-draft idea of how you could make the separation happen. There's different ways to do it, and if you feel like something else would be more organized, then do it.

Answer 2

It's good for what it is, and the end result looks nice.

I want to suggest you look into Typescript for type safety and readability, a framework like React/Vue/Angular/Svelte to avoid having to do DOM manipulation, a state management library like MobX for readability, etc.

However, it seems that your goal is to use plain HTML/CSS/JS.

Some thoughts:

You can use the dataset attribute to access your data-XXX attributes: https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes

Consider using event bubbling to catch events at a higher level. Then you won't need so many event listeners, and you'll be able to replace the DOM more easily.

Consider putting your entire game state into a single object. Then restarting the game would be as simple as state.game = new Game

Finally, consider having one function that generates the view from the game state. Then whenever the game changes, you'd regenerate the view from the model and slam it into the DOM.

You could use something like https://handlebarsjs.com/ to help with the HTML generation.

This would move you a lot closer towards a MVC implementation.