2
\$\begingroup\$

At work I am switching from AS3 to HTML5 for mobile game development, I have always hated JavaScript and developing for web but I have denied this transition for far too long and now I have to make the switch.

I would like for some of you to give me a little feedback on my simple Tic Tac Toe game. I only spent my lunch time writing this so ignore the rubbish AI or the fact that this should not all be on one HTML page (I know how to include a JavaScript file and CSS, but just wanted everything done quickly and ploughed in).

Not using classes or declaring variable types is so strange to me that the code just looks incomplete. I'm not sure if it has just been too long since I last coded a lot of JavaScript or I am to use to type casting but when I look at this code I feel like I am missing something obvious a junior would notice. Is the code fine or are there specific areas I should look into to improve?

<html>
<style>
  #body {
    display: block;
  }
  .grid {
    position: absolute;
    width: 300px;
    height: 300px;
    margin: auto;
    left: 0px;
    right: 0px;
    top: 0px;
    bottom: 0px;
  }
  .squareButton {
    width: 95px;
    height: 95px;
    border-radius: 20px;
    background-color: #DDDDDD;
    font-size: 40px;
    color: #3333AA;
  }
</style>
<script>
  var gridArray = [0, 0, 0, 0, 0, 0, 0, 0, 0];
  var aiSequence = [4, 0, 2, 6, 8, 1, 3, 5, 7];

  var aiPlaying = true;
  var aiPlayerId = 2;

  var playerIdentifiers = ["-", "O", "X"];
  var playersTurn = 1;

  function onLoad() {
    clearGrid();
  }

  function squareClicked(buttonId) {
    if (gridArray[buttonId] == 0) {
      setSquareToCurrentPlayer(buttonId);
    } else {
      alert("that square has been taken, please choose a different one :)");
    }
  }

  function setSquareToCurrentPlayer(squareId) {
    gridArray[squareId] = playersTurn;
    endTurn();
  }

  function endTurn() {
    updateGridDisplay();

    if (endGame() == true) {
      if (winCheck() == true) {
        alert("GAME OVER!!! player " + playersTurn + " WINS!!! - hope you had fun :)");
      } else {
        alert("GAME OVER!!! No more spaces left :( Hope you had fun ;)");
      }
    } else {
      playersTurn == 1 ? playersTurn = 2 : playersTurn = 1;

      if (aiPlaying == true) {
        if (playersTurn == aiPlayerId) {
          runAi();
        }
      }
    }
  }

  function runAi() {
    var aiCanWin = false;
    var playerCanWin = false;

    for (var i = 0; i < 9; i++) {
      if (gridArray[i] == 0) {
        gridArray[i] = playersTurn;

        if (winCheck()) {
          aiCanWin = true;
          break;
        } else {
          gridArray[i] = 0;
        }
      }
    }

    if (aiCanWin == false) {
      for (i = 0; i < 9; i++) {
        if (gridArray[i] == 0) {
          gridArray[i] = playersTurn == 1 ? 2 : 1;

          if (winCheck()) {
            playerCanWin = true;
            gridArray[i] = playersTurn;
            break;
          } else {
            gridArray[i] = 0;
          }
        }
      }

      if (playerCanWin == false) {
        for (i = 0; i < aiSequence.length; i++) {
          if (gridArray[aiSequence[i]] == 0) {
            gridArray[aiSequence[i]] = playersTurn;
            break;
          }
        }
      }
    }

    endTurn();
  }

  function endGame() {
    if (winCheck() == true) {
      return true;
    } else {
      if (allSquaresTaken() == true) {
        return true;
      } else {
        return false;
      }
    }
  }

  function allSquaresTaken() {
    var allSquaresHaveBeenTaken = true;

    for (var i = 0; i < 9; i++) {
      if (gridArray[i] == 0) {
        allSquaresHaveBeenTaken = false;
        break;
      }
    }

    return allSquaresHaveBeenTaken;
  }

  function winCheck() {
    if (
      gridArray[0] == gridArray[1] && gridArray[1] == gridArray[2] && gridArray[0] > 0 ||
      gridArray[3] == gridArray[4] && gridArray[4] == gridArray[5] && gridArray[3] > 0 ||
      gridArray[6] == gridArray[7] && gridArray[7] == gridArray[8] && gridArray[6] > 0 ||
      gridArray[0] == gridArray[3] && gridArray[3] == gridArray[6] && gridArray[0] > 0 ||
      gridArray[1] == gridArray[4] && gridArray[4] == gridArray[7] && gridArray[1] > 0 ||
      gridArray[2] == gridArray[5] && gridArray[5] == gridArray[8] && gridArray[2] > 0 ||
      gridArray[0] == gridArray[4] && gridArray[4] == gridArray[8] && gridArray[0] > 0 ||
      gridArray[2] == gridArray[4] && gridArray[4] == gridArray[6] && gridArray[2] > 0
    ) {
      return true;
    } else {
      return false;
    }
  }

  function clearGrid() {
    for (var i = 0; i < 9; i++) {
      gridArray[i] = 0;
    }

    updateGridDisplay();
  }

  function updateGridDisplay() {
    for (var i = 0; i < 9; i++) {
      document.getElementById("" + i).innerHTML = playerIdentifiers[gridArray[i]];
    }
  }
</script>

<body onload="onLoad()">
  <div class="grid" id="gridContainer">
    <button class="squareButton" onclick="squareClicked(this.id)" id="0">-</button>
    <button class="squareButton" onclick="squareClicked(this.id)" id="1">-</button>
    <button class="squareButton" onclick="squareClicked(this.id)" id="2">-</button>
    <button class="squareButton" onclick="squareClicked(this.id)" id="3">-</button>
    <button class="squareButton" onclick="squareClicked(this.id)" id="4">-</button>
    <button class="squareButton" onclick="squareClicked(this.id)" id="5">-</button>
    <button class="squareButton" onclick="squareClicked(this.id)" id="6">-</button>
    <button class="squareButton" onclick="squareClicked(this.id)" id="7">-</button>
    <button class="squareButton" onclick="squareClicked(this.id)" id="8">-</button>
  </div>
</body>

</html>

\$\endgroup\$
  • \$\begingroup\$ If you're missing types, you can use TypeScript - it's like JavaScript (and gets compiled to JavaScript) but offery type saftey \$\endgroup\$ – maja May 9 '15 at 15:20
  • \$\begingroup\$ if (endGame() == true) (1) You don't need to compare with true. (2) If you do, use === instead (also compares the types). Same for false, 1, 0. In fact, it's best practice to always use === instead of conventional == \$\endgroup\$ – maja May 9 '15 at 15:22
  • \$\begingroup\$ I'd rather use a 2D array for the grid than a 1D one. It's easier to figure out what gridArray[2][2] means versus gridArray[8]. \$\endgroup\$ – Timothy Gu May 9 '15 at 19:42
  • \$\begingroup\$ @maja, I will look into TypeScript, seems like what I would like, also I never use ===, very rarely compare to true if I want to check a boolean for that matter but I have developed that habit lately for some reason, I think I will take your advice and try to use === instead, I can see how that will help. \$\endgroup\$ – steven davies May 10 '15 at 16:23
  • \$\begingroup\$ @Timothy Gu, Thank you, I was going to use a 2d array because it can be expanded a lot easier if I ever wanted to but the board its self was not a focus and once I had written it and used it I didn't think I had time over lunch to go back and change it so just ran with it, bad choice I know and you're right and I probably should have just used a 2d array instead. \$\endgroup\$ – steven davies May 10 '15 at 16:28
1
\$\begingroup\$

I think there are some areas in which this program can be improved to make it more modular an OO-like. Keep in mind, however, that some of them may be overkill for such a simple program. The "improved" version that I wrote is almost 50% longer.

Global scope

you should not declare so many global variables or functions. It's best to create a context or scope to contain everything game-related and expose to external code only those elements that need to be exposed. For example, I have created a scope with all the code in it and have exposed a GAME object which has the only properties onLoad and squareClicked.

AI search

This bit of code is a bit confusing to me:

for (var i = 0; i < 9; i++) {
  if (gridArray[i] == 0) {
    gridArray[i] = playersTurn;

    if (winCheck()) {
      aiCanWin = true;
      break;
    } else {
      gridArray[i] = 0;
    }
  }
}

Essientally you are modifying the board (which is a kind of global variable in the context of the game), then checking if the state of the board is a win and, if it isn't, reverting the board to the previous state. I think it makes more sense to create a copy of the board, modify the copy, and check that copy for wins. In that way you don't modify the "real" board unless a move is really made. That makes more sense to me, although it can be subjective.

Encapsulation

You can use a "class" Board to encapsulate the state of a board and the operations on this state. Ok, there are no classes in JavaScript, but you get what I mean... a constructor or whatever it's called. The point is to create (nested) scopes so that we can put related code together and isolated from other code. The same goes for AI, except in this case you don't need a constructor because you only need one AI object.

Complete refactored version:

<html>
<style>
  #body {
    display: block;
  }
  .grid {
    position: absolute;
    width: 300px;
    height: 300px;
    margin: auto;
    left: 0px;
    right: 0px;
    top: 0px;
    bottom: 0px;
  }
  .squareButton {
    width: 95px;
    height: 95px;
    border-radius: 20px;
    background-color: #DDDDDD;
    font-size: 40px;
    color: #3333AA;
  }
</style>
<script>

  var GAME = (function(){

    // Numbering of buttons:
    //    0 1 2
    //    3 4 5
    //    6 7 8

    var board;
    var aiPlaying = true;
    var aiPlayer = "X";

    var currentPlayer = "O";

    function onLoad() {
      board = new Board();
      updateGridDisplay();
    }

    function squareClicked(squareId) {
      if (board.squareAvailable(squareId)) {
        makeMove(squareId, currentPlayer);
      } else {
        alert("that square has been taken, please choose a different one :)");
      }
    }

    function makeMove(squareId, player){
      board.setSquare(squareId, player);
      updateGridDisplay();
      endTurn();
    }

    function endTurn() {
      if (board.isEndGame()) {
        if (board.isWin()) {
          alert("GAME OVER!!! player " + currentPlayer + " WINS!!! - hope you had fun :)");
        } else {
          alert("GAME OVER!!! No more spaces left :( Hope you had fun ;)");
        }
      } else {
        currentPlayer = otherPlayer();
        if (aiPlaying && (currentPlayer === aiPlayer)) {
          AI.play();
        }
      }
    }

    function otherPlayer(){
      return {
        "X" : "O",
        "O" : "X"
      }[currentPlayer];
    }

    function updateGridDisplay() {
      for (var i = 0; i < 9; i++) {
        var squareValue = board.getSquares()[i].getValue();
        var text = squareValue || "-";
        document.getElementById("" + i).innerHTML = text;
      }
    }

    var AI = (function(){

      var playingSequence = [4, 0, 2, 6, 8, 1, 3, 5, 7];

      function play() {
        var move = decideBestMove();
        makeMove(move, aiPlayer);
      }

      function decideBestMove(){

        // Play a win move if there is one, else block the opponent
        // if possible, else play according to sequence.

        var availablePositions = board.getAvailablePositions();

        var winMoves = availablePositions.filter(function(pos){
          return playerWouldWin(currentPlayer, pos);
        });

        var blockMoves = availablePositions.filter(function(pos){
          return playerWouldWin(otherPlayer(), pos);
        });

        var allMoves = playingSequence.filter(function(pos){
          return (availablePositions.indexOf(pos) !== -1);
        });

        if(winMoves.length){
          return winMoves[0];
        } else if (blockMoves.length) {
          return blockMoves[0];
        } else {
          return allMoves[0];
        }
      }

      function playerWouldWin(player, position){
        var clone = board.clone();
        clone.setSquare(position, player);
        return clone.isWin();
      }

      return {
        play : play
      }

    }());

    var Board = (function(){

      // Static

      var STRAIGHTS = [
                [0, 1, 2],
                [3, 4, 5],
                [6, 7, 8],
                [0, 3, 6],
                [1, 4, 7],
                [2, 5, 8],
                [0, 4, 8],
                [2, 4, 6]
            ];

      function makeEmptySquares(){
        var arr = [], i;
        for(i=0; i<9; i++){
          arr.push(new Square());
        }
        return arr;
      }

      // Constructor
      var constr = function(squares){

        var mySquares = squares || makeEmptySquares();

        function squareAvailable(squareId){
          return mySquares[squareId].isAvailable();
        }

        function isEndGame() {
          return isWin() || allSquaresTaken();
        }

        function allSquaresTaken() {
          return mySquares.every(function(square){
            return !square.isAvailable();
          });
        }

        function isWin(board) {
          return STRAIGHTS.some(straightWins);
        }

        function straightWins(straight){

          var sq0 = mySquares[straight[0]];
          var sq1 = mySquares[straight[1]];
          var sq2 = mySquares[straight[2]];

          return (
              (sq0.getValue() === sq1.getValue())
              && (sq1.getValue() === sq2.getValue())
              && (!sq0.isAvailable())
            );
        }

        function setSquare(squareId, playerId){
          mySquares[squareId].setValue(playerId);
        }

        function clone(){
          var clonedSquares = [];
          for(var i=0; i<9; i++){
            clonedSquares[i] = mySquares[i].clone();
          }
          return new Board(clonedSquares);
        }

        function getAvailablePositions(){
          var arr=[];
          mySquares.forEach(function(square, index){
            if(square.isAvailable()){
              arr.push(index);
            }
          });
          return arr;
        }

        // Public
        this.squareAvailable = squareAvailable;
        this.isWin = isWin;
        this.isEndGame = isEndGame;
        this.setSquare = setSquare;
        this.clone = clone;
        this.getAvailablePositions = getAvailablePositions;
        this.getSquares = function(){ return mySquares; };
      }

      return constr;

    }());

    function Square(value){

      var myValue = value || null;

      function isAvailable(){
        return (myValue === null);
      }

      // Public
      this.isAvailable = isAvailable;
      this.getValue = function(){ return myValue; };
      this.setValue = function(v){ myValue = v; };
      this.clone = function(){ return new Square(myValue); }
    }

    // Public GAME object
    return {
      onLoad : onLoad,
      squareClicked : squareClicked
    };

  }());

</script>

<body onload="GAME.onLoad()">
  <div class="grid" id="gridContainer">
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="0">-</button>
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="1">-</button>
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="2">-</button>
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="3">-</button>
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="4">-</button>
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="5">-</button>
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="6">-</button>
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="7">-</button>
    <button class="squareButton" onclick="GAME.squareClicked(this.id)" id="8">-</button>
  </div>
</body>

</html>
\$\endgroup\$
  • \$\begingroup\$ Thank you for your response, I hope it didn't take you to long to write that new code, it has given me a lot to think about but there is a lot there so I will have to go over it in more detail when I have time, with regards to the board I can see why you would say I should create a copy and adjust that but the reason I just manipulated the "real" board is because I thought that making a copy would take longer for processing and use more ram, of course irrelevant for such a small program but you see my point if it was a massive array, I could be wrong but that was a reflex to conserve resources \$\endgroup\$ – steven davies May 10 '15 at 16:42
1
\$\begingroup\$

Ill give an answer a shot since I recently wrote my own tic-tac-toe game in javascript. I believe an improvement would be to create different classes for the different aspects of the game. This will add to the readability of your code and more importantly allow you to easily swap out aspects if you make changes, like updating your AI. I will use the class Board as an example with comments for some code I would change:

var Board = function(){
  this.gridArray = [0, 0, 0, 0, 0, 0, 0, 0, 0],
  this.aiPlayerId = 2,
  this.playerTurn = 1,
  //my addition for ease later
  this.empty = 0,
  this.wins = [
    [0,1,2],
    [3,4,5],
    [6,7,8],
    [0,3,6],
    [1,4,7],
    [2,5,8],
    [0,4,8],
    [2,4,6]
    ];
}

Board.prototype = {
  winCheck: function(){
    //Using Board.wins allows the win check to read easier and will return the winner as 2 (AI) or 1 (Player)

    for(var i=0; i<this.wins.length; i++){
      var a, b, c;
      a = this.gridArray[this.wins[i][0]];
      b = this.gridArray[this.wins[i][1]];
      c = this.gridArray[this.wins[i][2]];

      if(a == b && a == c && a != this.empty){
        return a;
      }
    }
    return this.empty;
  },
  allSquaresTaken: function(){
    //Because the function is well named returning "false" clearly means all squares are not taken. I drop the variable because of this
    for(var i=0; i<9; i++){
      if(this.gridArray[i] == 0) {
        //in javascript returning ends the function, break; is never reached.
        return false;
      }
    }
    return true;
  },
  setSquareToCurrentPlayer: function(squareId, player){
    gridArray[squareId] = player;
  }
  //additional method possibilities are getMoves (returns possible moves), copy (copy board)

}

I prefer setting onClick listeners for buttons in javascript and believe it leads to less coupling but cannot find a source right now.

(function init(){
  for(var i=0; i<9; i++){
    var btn = document.getElementById(i);
    btn.addEventListener('click', squareClicked);
  }
  //self calling function
})();

You don't need to give up classes and OOP to write js, it is just loosely typed. My implementation is here: https://github.com/boatsoap/tic-tac-io and the logic is run on the server via socket.io.

Cheers.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the comment, I was not really looking to improve the game itself but rather my coding in terms of syntax and/or structure but I do like your win check, far more elegant then my silly if statement and I had forgotten about prototype, clearly been to long and I need to revisit some of the basics. \$\endgroup\$ – steven davies May 10 '15 at 16:18
  • \$\begingroup\$ also didn't know enter will finish my comment, :) but I avoided adding the listeners via JavaScript because I wanted to pass the button id so I only has a single handler but thinking about it you can do that anyway and I should have so I agree with you there too. \$\endgroup\$ – steven davies May 10 '15 at 16:20

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.