Ggthjy

I created a simple Tic-Tac-Toe. I'm looking for any critiques from a code standpoint: general structure / style / efficiency.

Unfortunately I couldn't get a live demo working because of the dependency on the NodeJS library prompt.

/*  A simple Tic-Tac-Toe game implemented in Javascript V8 3.14.5.9



Players 'X' and 'O' take turn inputing their position on the command line using numbers 1-9



1 | 2 | 3

---------

4 | 5 | 6

---------

7 | 8 | 9



*/





// prompt for nodejs is required: https://github.com/flatiron/prompt

// npm install prompt

var prompt = require('prompt');



var board = {

    1: ' ',

    2: ' ',

    3: ' ',

    4: ' ',

    5: ' ',

    6: ' ',

    7: ' ',

    8: ' ',

    9: ' '

};



function markBoard(position, mark) {

    board[position] = mark.toUpperCase();

}



function printBoard() {

    console.log('n' +

        ' ' + board[1] + ' | ' + board[2] + ' | ' + board[3] + 'n' +

        ' ---------n' +

        ' ' + board[4] + ' | ' + board[5] + ' | ' + board[6] + 'n' +

        ' ---------n' +

        ' ' + board[7] + ' | ' + board[8] + ' | ' + board[9] + 'n');



}



function isInt(value) {

    var x;

    if (isNaN(value)) {

        return false;

    }

    x = parseFloat(value);

    return (x | 0) === x;

}



function validateMove(position) {

    if (isInt(position) === true && board[position] === ' ') {

        return true;

    }

    return false;

}



// Everyone possible combination of three in a row

var winCombinations = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7],

                       [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]];



// Determins if the passed in player has three in a row

function checkWin(player) {

    for (var i = 0; i < winCombinations.length; i++) {

        var markCount = 0;

        for (var j = 0; j < winCombinations[i].length; j++) {

            if (board[winCombinations[i][j]] === player) {

                markCount++;

            }

            if (markCount === 3) {

                return true;

            }

        }

    }

    return false;

}



function playTurn(player) {



    console.log('Your turn player: ' + player);

    prompt.start();

    prompt.get(['position'], function (err, result) {



        if (validateMove(result.position) === true) {

            markBoard(result.position, player);

            printBoard();

            if (checkWin(player) === true) {

                console.log('Winner Winner!');

                return;

            }

            if (player === 'X') {

                playTurn('O');

            } else {

                playTurn('X');

            }

        } else {

            console.log('incorrect input please try again...');

            playTurn(player);

        }

    });

}



console.log('Game started: n' +

    ' 1 | 2 | 3 n' +

    ' --------- n' +

    ' 4 | 5 | 6 n' +

    ' --------- n' +

    ' 7 | 8 | 9 n');





playTurn('X');

This code was very well organized and was extremely easy to follow along.

Looks to me that board is just a 1-based array.

It would be easier (and it makes more sense) to use an array, instead of an object.

But don't change how you are asking the user for input. You want to translate the user input one number back, so if the user enters "1" for box number 1, just subtract 1 from the integer version of the input and then pass it to markBoard.

if (validateMove(result.position) === true) {

    markBoard(result.position - 1, player);

    printBoard();

In your markBoard function, there is no point in calling .toUpperCase as the player variable passed in from playTurn is already a capital letter.

You are making validateMove too complicated. Just do:

return isInt(position) && board[position] === ' ';

If the expression evaluates to true, it will return true. If it evaluates to false, it will return false.

Note: I omitted the === true because JavaScript automatically checks for something to equal true.

It doesn't look like you have anything to check if the user enters an invalid number.

I would add something to the end of validateMove to check to make sure that the option is in bounds.

Here is what I came up with:

return isInt(position) && board[position] === ' ' && (position >= 0 && position <= 8);

It would be >= 1 && <= 9 if you continued to use the object instead of the array for storing squares of the board

This might help your efficiency:

Try moving this conditional from checkWin

if (markCount === 3) {

    return true;

}

Outside of the inner for loop so you aren't doing a conditional check with each iteration.

In checkWin, you do a lot of looping. In each loop, you are destroying and re-making variables as they exit and leave the scope (the loop).

This is very optimal. To help this, move the declarations of markCount, i, and j to the very top of the function (before the loops). You can set them to 0 when they are needed.

Here is what I am saying:

function checkWin(player) {

    var i, j, markCount

    for (i = 0; i < winCombinations.length; i++) {

        markCount = 0;

        for (j = 0; j < winCombinations[i].length; j++) {

            if (board[winCombinations[i][j]] === player) {

                markCount++;

            }

            if (markCount === 3) {

                return true;

            }

        }

    }

    return false;

}

Notice how I removed the vars?

I'm a little confused by what your isInt function is trying to accomplish.

To make it simpler, just run isNaN on the position. It will return NaN (which acts like false in a conditional) if the user entered a string, rather than a number.

This is great. Your validation can be handled for you by prompt, though, e.g.

  const positionQuery = `${player}'s turn. choose a position 1-9`

  const position = {

    properties: {

      [positionQuery]: {

        pattern: /^[1-9]{1}$/,

        message: 'Choose a position 1-9',

        required: true

      }

    }

  }

  prompt.start()

  prompt.get([positionQuery], (err, result) => {

   ...

  }

🚗🏎🚚🚨🚨🚔🚍🚒🚑🚜🚜🚜🚜🏍🚒🚒🚒🚒🚒🚒🚒🚒🚒🚒🏍🏍