4
\$\begingroup\$

I have created a simple Tic Tac Toe game using JavaScript and using jQuery. I have used bootstrap for the UI. I am just a beginner, so some feedback on the JavaScript element would be great.

$(document).ready(function(){
  // true = player 1
  // false = player 2
  var pressedIndex;
  var pressedElement;
  var status = $('.centeredText').filter('.status');

  $('.well').on('click',function(){
    pressedElement = $(this);
    pressedIndex = pressedElement.data('index');
    //Check if the game is in progress
    if(isWinner() === false){
      //Check if the move is legal
      if(isMoveValid(pressedIndex)){
        // Add the marker (O/X) to UL and array
        var marker = addMarkerToBoard(pressedIndex);
        pressedElement.find('.contentOfWell').text(marker);
        // Next player turn
        nextPlayerTurn();
        if(turn){
          status.text("Player 1 - X Turn")
        } else {
          status.text("Player 2 - O Turn");
        }
      } else {
        status.text("Invalid Move!");
      }
    } else {
      status.text("Game over");
    }
  });

  });

var turn = true;
var board = [];
var messageToUser;

function nextPlayerTurn(){
  if(turn){
    turn = false;
  } else {
    turn = true;
  }
  return turn;
}

function addMarkerToBoard(index){
  // true - player 1 is a X
  // false - player 2 is a O
  if(turn){
    board[index] = "X";
    return "X";
  } else {
    board[index] = "O";
    return "O";
  }
}

function isWinner(){
  // X | X | X
  // X | X | X
  // x | X | X

  var top = board[0];
  var middle = board[3];
  var bottom = board[6];
  var winner = false;

  //Not good code since at the start the array indexes are all undefined so it will return true.
  // Fixed by not allowing some spaces to be undefined
  //1. Horizontal Rows
  if(board[1] === top && board[2] === top && top != undefined){
    winner = true;
  } else if (board[4] === middle && board[5] === middle && middle != undefined){
    winner = true;
  } else if(board[7] === bottom && board[8] === bottom && bottom != undefined){
    winnder = true;
  //2. Vertical Rows
  } else if(board[3] === top && board[6] === top && top != undefined){
    winner = true;
  } else if(board[4] === board[1] && board[7] === board[1] && board[1] != undefined){
    winner = true;
  } else if(board[5] === board[2] && board[8] === board[2] && board[2]){
    winner = true;
  //3. Across
} else if(board[4] === top && board[8] === top && top != undefined){
    winner = true;
  } else if(board[4] === board[2] && board[6] === board[2] && board[2] != undefined){
    winner = true;
  } else {
    winner = false;
  }
  return winner;
}
function isGameOver(){

}

function isMoveValid(index){
  // So space is not take
  if(board[index] === undefined){
    return true;
  } else {
    return false;
  }
}
.centeredText{
  text-align:center;
  padding:0;
  margin:0;
}
.board{
  padding:1%;
}

.takeFullSize{
  display: block;
  width:100%;
  height:10%;
}
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="utf-8" />
  <meta name="viewport" content="width=device-width,initial-scale=1" />
    <link rel="stylesheet" href="styling.css" />
    <link rel="stylesheet" href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css">
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.12.2/jquery.min.js"></script>
    <script src="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/js/bootstrap.min.js"></script>
    <script src="logic.js"></script>


    <title>Tic tack toe</title>
</head>
<body>
  <div class="container-fluid">
    <h1 class="centeredText">Tick Tack Toe</h1>
    <h3 class="centeredText status">Player 1 Turn</h3>
    <div class="board">
    <div class="row">
      <!-- Start of Row 1 -->
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="0">
            <p class="contentOfWell centeredText">A</p>
          </div>
        </div>
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="1">
            <p class="contentOfWell centeredText">B</p>
          </div>
        </div>
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="2">
            <p class="contentOfWell centeredText">C</p>
          </div>
        </div>
      <!-- End of Row 1 -->

      <!-- Start of Row 2 -->
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="3">
            <p class="contentOfWell centeredText">E</p>
          </div>
        </div>
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="4">
            <p class="contentOfWell centeredText">F</p>
          </div>
        </div>
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="5">
            <p class="contentOfWell centeredText">G</p>
          </div>
        </div>
      <!-- End of Row 2 -->

      <!-- Start of Row 3 -->
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="6">
            <p class="contentOfWell centeredText">H</p>
          </div>
        </div>
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="7">
            <p class="contentOfWell centeredText">I</p>
          </div>
        </div>
        <div class="col-xs-4 col-lg-4">
          <div class="well" data-index="8">
            <p class="contentOfWell centeredText">J</p>
          </div>
        </div>
      <!-- End of Row 3 -->
    </div>
  </div>
  </div>
</body>

Improve this question
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Also, when you copy-paste your code, select it all and press Ctrl+K to indent it. \$\endgroup\$
    – CodesInTheValley
    Jul 16, 2016 at 13:18

1 Answer 1

Reset to default
2
\$\begingroup\$

Display wins, losses, and draws immediately

In your game, when the game comes to a player winning, a player losing, or the players reaching a draw, the game does not notify the players with a "Game Over!" until the board is clicked again.

To fix this, you should check to see if the player's move is a valid move and then place the marker before you do any win checking:

  $('.well').on('click',function(){
    pressedElement = $(this);
    pressedIndex = pressedElement.data('index');
    //Check if the game is in progress
    if(isMoveValid(pressedIndex)){ // <----
        // Add the marker (O/X) to UL and array
        var marker = addMarkerToBoard(pressedIndex);
        pressedElement.find('.contentOfWell').text(marker);
        // Next player turn
        nextPlayerTurn();
        if(isWinner() === false){ // <-----
          //Check if the move is legal

        ...
  });

Global variables

Avoid global variables when you can. board in this case is okay, but in the future, it may be better inside an object (as would all these other functions).

However, turn is really bad. You should just have the nextPlayerTurn function return the value that tells which player's turn it is, rather than setting the global variable. And, on that note, you should specify which player true and false correspond to in some documentation:

/**
 * Returns true for X and false for O.
 */
function nextPlayerTurn(){
  return !turn;
}

I also simplified the conditional for you.

Simplify win checking

I'm not going to go too much into it because I've said it way too many times before, but your checking for the win can be simplified greatly by storing all the possible win combinations in a large array.

So, here's what the structure of your array would look like:

var winCombos = [
    [index, index, index],
    [index, index, index],
    ...
];

where, for each inner array, you put the three board array-indexes needed to complete a win. For example, if your board array is 0-based, the first win combination would be [0, 1, 2] - the top row.

Then, you could easily iterate over this array and check each combination all in a single loop.

Sorry if this isn't clear; you can ask for clarification in a comment, and/or check some of my other tic tac toe answers for more detail.

Simplify more conditions/code

function isMoveValid(index){
  // So space is not take
  return !board[index];
}

Remove unused code

The variable messageToUser is never used, and isGameOver is completely empty.

Improve this answer
\$\endgroup\$

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.