6
\$\begingroup\$

I am interested in getting some feedback on the code for my TicTacToe game, implemented with AngularJS, particularly with respect to the AI logic, which is printed below. (The full code base is available on GitHub.)

I welcome any suggestions for improving the overall code quality, whether that's identifying places where things could be expressed more simply, suggesting reorganization of poorly structured code, or merely style. Tips for increasing the efficiency of my minimax AI would also be greatly appreciated.

Concerns I Am Aware of

  • I'm not completely sure if I pruned the game trees the correct way in the minimax implementation. The concept of pruning game trees felt easy, but I found myself unsure if I understood it correctly when I came to implement it. I have not noticed any problems with the AI in practice.
  • Because the AI would always make the same moves every time, and I found this boring, the return values of the initial calls to the minimax in decideMove are collected and passed to another function that collects all moves with the same, best value and chooses one randomly from them. I am uncertain if this was a good solution.
  • I have divided up the normal minimax into two separate methods on the AiLogic service, imaginaryMoveThem and imaginaryMoveMe. I did this because it was easier for me to conceptualize minimax this way. However, the redundancy is obvious. I did not really mind this redundancy, as I assumed another reader of the source might also find minimax easier to understand divided up this way, but now I am not so sure that this shouldn't be made less redundant.
  • Using Angular for this project, I conceived of the AI logic as a service, as it was more like a kind of data or model than a kind of controller. So there is a an AiLogic service, and a GameCtrl controller that connects it to the UI. Is this interpretation at all the "Angular way"?
  • There is also a GameLogic service, made with a similar idea as the AiLogicservice, but it now seems to me somewhat unnecessary, as it does very little.
  • I consider the Angular controller I have handling communications between the UI and the AI is more than a little ugly.

The AiLogic Code

angular.module('tictactoeApp')
  .factory('AiLogic', function () {
    var ailogic = {};

    ailogic.them = 'x';
    ailogic.me = 'o'; 


    ailogic.formatMoveData = function(move) {
      return [
        {row: 0, column: 0},
        {row: 0, column: 1},
        {row: 0, column: 2},
        {row: 1, column: 0},
        {row: 1, column: 1},
        {row: 1, column: 2},
        {row: 2, column: 0},
        {row: 2, column: 1},
        {row: 2, column: 2} 
      ][move];

    };

    ailogic.flattenBoardData = function(board) {
      var flattenedBoard = [];
      for (var r = 0; r < board.length; r++) {
        var row = board[r];
        for (var c = 0; c < row.length; c++) {
          var cell = row[c];
          flattenedBoard.push(cell.space);
        }
      }
      return flattenedBoard;
    };

    ailogic.max = function(arr) {
      return Math.max.apply(null, arr);
    }

    ailogic.equivalentMoves = function(arr) {
      var maxEl = this.max(arr);
      return arr.map(function(move, idx) {
        if (move === maxEl) {
          return idx; 
        } else {
          return null; 
        }
      }).filter(function(el) {
        return (el !== null); 
      }); 
    };

    ailogic.randomEquivalentMove = function(arr) {
      var equivalentMoves = this.equivalentMoves(arr);
      var randomIndex = Math.floor(Math.random() * equivalentMoves.length);
      return equivalentMoves[randomIndex];
    };

    ailogic.decideMove = function(board) {
      var flatBoard = this.flattenBoardData(board);
      var moves = [];
      for (var c = 0; c < flatBoard.length; c++) {
        var cell = flatBoard[c];
        // if I can move there 
        if (cell === '') {
          // I will imagine 
          var imaginaryBoard = angular.copy(flatBoard); 
          imaginaryBoard[c] = this.me; 
          // what they will do if I move there? 
          var moveValue = this.imaginaryMoveThem(imaginaryBoard);
          // Push my evaluation of this move to the moves array 
          moves.push(moveValue);
        } else {
          moves.push(null);
        }
      }

      // Pick randomly from moves judged to be the best   
      var randomMove = this.randomEquivalentMove(moves);
      return this.formatMoveData(randomMove); 
    };

    ailogic.imaginaryMoveThem = function(board) {
      var draw = this.drawn(board);
      var iWin = this.won(board, this.me);
      var theyWin = this.won(board, this.them); 
      if (iWin) {
        return 1;
      } else if (theyWin) {
        return -1; 
      } else if (draw) {
        return 0; 
      } else {
        var bestMoveValue = 100; 
        for (var c = 0; c < board.length; c++) {
          var cell = board[c];
          // If they can move there 
          if (cell === '') {
            // imagine what it'll be like for me 
            var imaginaryBoard = angular.copy(board);
            imaginaryBoard[c] = this.them;
            // when they move there.  
            var availableMoveValue = this.imaginaryMoveMe(imaginaryBoard);
            // If this move is bad for me 

            if (availableMoveValue < bestMoveValue) {
              // they will do it.  
              bestMoveValue = availableMoveValue;
            }
            // If this move allows them to win,
            if (availableMoveValue === -1) {
              // they will do it. 
              break 
            }
          }
        }
        return bestMoveValue;
      }
    };

    ailogic.imaginaryMoveMe = function(board) {
      var draw = this.drawn(board);
      var iWon = this.won(board, this.me);
      var theyWon = this.won(board, this.them);
      if (iWon) {
        return 1; 
      } else if (theyWon) {
        return -1;
      } else if (draw) {
        return 0; 
      } else {
        var bestMoveValue = -100; 
        for (var c = 0; c < board.length; c++) {
          var cell = board[c];
          // If I move there 
          if (cell === '') {
            var imaginaryBoard = angular.copy(board);
            imaginaryBoard[c] = this.me;
            // what can they do? 
            var availableMoveValue = this.imaginaryMoveThem(imaginaryBoard);
            // Choose the move in which they can do the least. 
            if (availableMoveValue > bestMoveValue) {
              bestMoveValue = availableMoveValue;
            }
            // If I can win w/ this move 
            if (availableMoveValue === 1) {
              // let me win! 
              break 
            }
          }
        }
        return bestMoveValue; 
      }
    };

    ailogic.drawn = function(flatBoard) {
      // it is a draw when there is nowhere left to move 
      return flatBoard.every(function(cell) {
        return (cell !== '');
      });
    };

    ailogic.won = function(flatBoard, player) {
      return (((flatBoard[0] === player) && (flatBoard[1] === player) && (flatBoard[2] === player)) ||
      ((flatBoard[3] === player) && (flatBoard[4] === player) && (flatBoard[5] === player)) ||
      ((flatBoard[6] === player) && (flatBoard[7] === player) && (flatBoard[8] === player)) ||
      ((flatBoard[0] === player) && (flatBoard[3] === player) && (flatBoard[6] === player)) ||
      ((flatBoard[1] === player) && (flatBoard[4] === player) && (flatBoard[7] === player)) ||
      ((flatBoard[2] === player) && (flatBoard[5] === player) && (flatBoard[8] === player)) ||
      ((flatBoard[0] === player) && (flatBoard[4] === player) && (flatBoard[8] === player)) ||
      ((flatBoard[2] === player) && (flatBoard[4] === player) && (flatBoard[6] === player)));
    };

    return ailogic; 
  });
\$\endgroup\$
1
\$\begingroup\$

Unfortunately nobody has reviewed this code since it was posted. Are you still using this code? It doesn't appear that you have been active much here in recent years so I might guess not.

The functions ailogic.imaginaryMoveThem() and ailogic.imaginaryMoveMe() look very redundant. Perhaps it would be simpler to abstract out the common code to a separate function that accepts parameters like the bestMoveValue and the player (e.g. this.me or this.them).


Perhaps it would be wise to pull out the array used in formatMoveData() to a constant (e.g. using the const keyword). That way the function can be much shorter, and that array could be used elsewhere if needed.

\$\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.