Suggestions for improving my Tic-Tac-Toe code in JavaScript

Question

I have some hack-ey programming experience and recently made a Tic-Tac-Toe game in JavaScript. The game seems to work fine. I am looking for some constructive criticism to help understand what can be better.

(full code in Github repo)

function Player(name, selector, color) {
 this.name = name || "";
 this.moves = [];
 this.chances = [];
 this.selector = selector || "x";
 this.color = color;
}
// Setup
window.players = [];
var user = new Player('user', 'O', 'green');
var computer = new Player('computer', 'X', 'pink');
players.push(user);
players.push(computer);
// Trackers for the board and moves
var board = ['A1', 'B1', 'C1', 'A2', 'B2', 'C2', 'A3', 'B3', 'C3'];
var wins = ['A1A2A3', 'B1B2B3', 'C1C2C3', 'A1B1C1', 'A2B2C2', 'A3B3C3', 'A1B2C3', 'C1B2A3'];
var moves = 1;
var currentPlayer = players[0];
var game = 1;
// Handle clicks
$(".box").click(function (event) {
 if (getCurrentPlayer() === user && game == 1) usersMove(event.target.id);
});
// *** MOVES *** 
// With every move, these functions update the state of the board
function usersMove(boxId) {
 if (validMove(boxId) === true) {
 makeMove(user, boxId);
 } else alert('taken');
}
function computersMove() {
 makeMove(computer, getBestNextMove(computer));
}
function validMove(boxId) {
 return (board.indexOf(boxId) != -1);
}
function markBox(player, boxId) {
 $("#" + boxId).text(player.selector).addClass(player.color);
}
function updateBoard(boxId) {
 board.splice(board.indexOf(boxId), 1);
}
function updateMoves(player, boxId) {
 moves = moves + 1;
 player.moves.push(boxId);
}
function makeMove(player, boxId) {
 if (moves < 10) {
 markBox(player, boxId);
 updateBoard(boxId);
 updateMoves(player, boxId);
 addChances(player, boxId);
 removeChances(getOtherPlayer(), boxId);
 if (checkWin(getCurrentPlayer(), boxId) === false) { 
 getBestNextMove(player);
 setTimeout(function () { nextTurn() }, 500);
 }
 } else endGame("Nobody");
}
// *** TRACK AND UPDATE USER'S CHANCES *** 
// Add to current player chances array
function addChances(player, boxId) {
 for (var i = 0; i < wins.length; i++) {
 if (wins[i].indexOf(boxId) !== -1 && player.chances.indexOf(wins[i]) === -1 && getOtherPlayer().chances.indexOf(wins[i]) === -1) {
 player.chances.push(wins[i]);
 wins.splice(wins.indexOf(wins[i]), 1);
 i--;
 }
 }
}
// Eliminate arrays from other player chances array
function removeChances(player, boxId) { 
 for (var i = 0; i < player.chances.length; i++) {
 if (player.chances[i].indexOf(boxId) !== -1) {
 player.chances.splice(player.chances.indexOf(player.chances[i]), 1);
 i--;
 }
 }
}
// *** FIND THE BEST NEXT MOVE *** 
function getBestNextMove(player) {
 var c = player.chances;
 var m = player.moves;
 for (var i = 0; i < c.length; i++) {
 for (var j = 0; j < m.length; j++) {
 if (c[i].indexOf(m[j]) != -1) {
 c[i] = c[i].replace(m[j], "");
 }
 }
 }
 if (c.length > 0 && c[0].length == 2) { 
 return c[0];
 } else return sortMoves();
}
function sortMoves(){
 var scores = [];
 var bestMoves = [];
 // rank moves based on score
 for (var i = 0; i < board.length; i++) {
 scores.push({ 'boxId' : board[i], 'val' : rankMove(board[i]) });
 }
 scores.sort(function(a,b) {
 return b.val - a.val;
 });
 // get all options with the highest score
 for (var i = 0; i< scores.length; i++) {
 if (scores[i].val == scores[0].val) {
 bestMoves.push(scores[i]['boxId']);
 }
 }
 // select a random one from the highest score options
 var moveIndex = Math.floor(Math.random()*bestMoves.length);
 return scores[moveIndex].boxId;
}
function rankMove(boxId) {
 var score = 0;
 for (var p = 0; p < players.length; p++) {
 var c = players[p].chances;
 for (var i = 0; i < c.length; i++) {
 if (c[i].indexOf(boxId) !== -1) {
 score += 1;
 // if user is one step away from winning, increase chances
 if (c[i].length == 2) score += 5;
 }
 }
 }
 return score;
}
function sortByLength(arr) {
 arr.sort(function(a, b){
 return a.length - b.length;
 }); 
 return arr;
}
// *** SETTERS & GETTERS
function getCurrentPlayer() {
 return currentPlayer;
}
function setCurrentPlayer(player) {
 currentPlayer = player;
 return currentPlayer;
}
function getOtherPlayer() {
 if (getCurrentPlayer() == players[0]) {
 return players[1];
 } else return players[0];
}
function nextTurn() {
 if (getCurrentPlayer() === players[0]) {
 setCurrentPlayer(players[1]);
 computersMove();
 } else setCurrentPlayer(players[0]);
}
// *** CHECK FOR WINNERS, CONTINUE/END
function checkWin(player, boxId) {
 var c = player.chances;
 for (var i = 0, tot = c.length; i < tot; i++) {
 if (c[i].length == 2 & c[i] == boxId ) {
 endGame(player);
 return true;
 }
 }
 if (board.length == 0 && game == 1) {
 endGame();
 return true;
 }
 return false;
}
function endGame(winner) {
 game = 0;
 if (winner) {
 alert(winner.name + " wins!");
 } else alert("it's a draw!");
}

• Romantic Electron: I think you should try a search tree with Alpha Beta pruning Here's the wikipedia link to the algorithm en.wikipedia.org/wiki/Alpha%E2%80%93beta_pruning
• Quentin Pradet: Additionally, alpha-beta pruning is a bit overkill for Tic-Tac-Toe, don't you think?
• Quentin Pradet: And he doesn't even use minimax. Sorry if it seems rude - this shouldn't discourage you from participating further to our community. Have a good day!
• Engin Erdogan: @Romantic Electron: Thanks for the pointer, will look into it now.

Answer 1

The code is nice, most comments are minor issues. General issues are:

1. the way you're involving winning conditions with the chance system
2. user interaction: you're over-using alerts when you could slide down messages with jQuery. Avoid modal interfaces.
3. AI is simple, but that may be what you want.

Line-by-line review:

function Player(name, selector, color) {
 this.name = name || "";
 this.moves = [];
 this.chances = [];
 this.selector = selector || "x";
 this.color = color;
}

I don't think silently choosing defaults for selector and name helps.

// Setup
window.players = [];
var user = new Player('user', 'O', 'green');
var computer = new Player('computer', 'X', 'pink');
players.push(user);
players.push(computer);

Thrive for concise code: window.players = [user, computer];

// Trackers for the board and moves
var board = ['A1', 'B1', 'C1', 'A2', 'B2', 'C2', 'A3', 'B3', 'C3'];
var wins = ['A1A2A3', 'B1B2B3', 'C1C2C3', 'A1B1C1', 'A2B2C2', 'A3B3C3', 'A1B2C3', 'C1B2A3'];
var moves = 1;
var currentPlayer = players[0];
var game = 1;

The game name is unclear, and should be a boolean.

// Handle clicks
$(".box").click(function (event) {
 if (getCurrentPlayer() === user && game == 1) usersMove(event.target.id);
});

Silently rejecting click is probably not too intuitive for the user ("where has my click gone?"), but I assume this never occurs in practice since the AI plays instantly.

// *** MOVES *** 
// With every move, these functions update the state of the board
function usersMove(boxId) {
 if (validMove(boxId) === true) {
 makeMove(user, boxId);
 } else alert('taken');
}

Good, you're informing the user. However, alert() is a bit rude.

function computersMove() {
 makeMove(computer, getBestNextMove(computer));
}
function validMove(boxId) {
 return (board.indexOf(boxId) != -1);
}
function markBox(player, boxId) {
 $("#" + boxId).text(player.selector).addClass(player.color);
}
function updateBoard(boxId) {
 board.splice(board.indexOf(boxId), 1);
}
function updateMoves(player, boxId) {
 moves = moves + 1;
 player.moves.push(boxId);
}
function makeMove(player, boxId) {
 if (moves < 10) {
 markBox(player, boxId);
 updateBoard(boxId);
 updateMoves(player, boxId);
 addChances(player, boxId);
 removeChances(getOtherPlayer(), boxId);

I don't find this too readable. Isn't it simpler to put everything into makeMove with comments like // mark chosen box?

 if (checkWin(getCurrentPlayer(), boxId) === false) { 
 getBestNextMove(player);
 setTimeout(function () { nextTurn() }, 500);
 }
 } else endGame("Nobody");
}
// *** TRACK AND UPDATE USER'S CHANCES ***

At this point, even when quickly looking at occurrences of "chance" in the rest of code the code, I have no idea what chances are. This deserves a better comment.

// Add to current player chances array
function addChances(player, boxId) {
 for (var i = 0; i < wins.length; i++) {
 if (wins[i].indexOf(boxId) !== -1 && player.chances.indexOf(wins[i]) === -1 && getOtherPlayer().chances.indexOf(wins[i]) === -1) {

Consider splitting this line so that it fits in ~80 chars. How could wins[i].indexOf(boxId) be -1?

 player.chances.push(wins[i]);
 wins.splice(wins.indexOf(wins[i]), 1);
 i--;

Okay.. got it. Iit looks like that when part of a winning move is done by a player, we add it to the player possible winning moves. Isn't this over-engineered? Why don't you simply check on every move if a winning move as made? You don't need to store wins or chances, just check 8 configurations. (Such a function would also work for larger grids, by the way.)

 }
 }
}
// Eliminate arrays from other player chances array
function removeChances(player, boxId) { 
 for (var i = 0; i < player.chances.length; i++) {
 if (player.chances[i].indexOf(boxId) !== -1) {
 player.chances.splice(player.chances.indexOf(player.chances[i]), 1);
 i--;
 }
 }
}
// *** FIND THE BEST NEXT MOVE *** 
function getBestNextMove(player) {
 var c = player.chances;
 var m = player.moves;
 for (var i = 0; i < c.length; i++) {
 for (var j = 0; j < m.length; j++) {
 if (c[i].indexOf(m[j]) != -1) {
 c[i] = c[i].replace(m[j], "");
 }

This is quite confusing, why don't you do this when adding chances? (If I understood correctly, you're removing already done moves from chances.)

 }
 }
 if (c.length > 0 && c[0].length == 2) { 
 return c[0];
 } else return sortMoves();

(Personal state: I don't really like this pattern where you have braces in the first branch but not in the second.)

}
function sortMoves(){
 var scores = [];
 var bestMoves = [];
 // rank moves based on score
 for (var i = 0; i < board.length; i++) {
 scores.push({ 'boxId' : board[i], 'val' : rankMove(board[i]) });
 }
 scores.sort(function(a,b) {
 return b.val - a.val;
 });
 // get all options with the highest score
 for (var i = 0; i< scores.length; i++) {
 if (scores[i].val == scores[0].val) {
 bestMoves.push(scores[i]['boxId']);
 }
 }
 // select a random one from the highest score options
 var moveIndex = Math.floor(Math.random()*bestMoves.length);
 return scores[moveIndex].boxId;

Learn to use JavaScript collection functions. reduce() can get you the max, and filter() can get you all maxs.

}
function rankMove(boxId) {
 var score = 0;
 for (var p = 0; p < players.length; p++) {
 var c = players[p].chances;
 for (var i = 0; i < c.length; i++) {
 if (c[i].indexOf(boxId) !== -1) {
 score += 1;
 // if user is one step away from winning, increase chances
 if (c[i].length == 2) score += 5;
 }
 }
 }
 return score;
}

You might want to use minimax if you want your AI not to make any mistakes. However, it can be more frustrating to play.

function sortByLength(arr) {
 arr.sort(function(a, b){
 return a.length - b.length;
 }); 
 return arr;
}

Unused.

// *** SETTERS & GETTERS
function getCurrentPlayer() {
 return currentPlayer;
}

Seriously?

function getOtherPlayer() {
 if (getCurrentPlayer() == players[0]) {
 return players[1];
 } else return players[0];
}

See, the pattern mentioned above makes it seem like there's a difference between the two branchs, but they're really symmetrical.

function nextTurn() {
 if (getCurrentPlayer() === players[0]) {
 setCurrentPlayer(players[1]);
 computersMove();
 } else setCurrentPlayer(players[0]);
}

Check your indentation.

 if (board.length == 0 && game == 1) {
 endGame();
 return true;
 }
 return false;
}

Feels quite wrong... but you can't do much about it since endGame() won't return before user has clicked on the pop-up.

function endGame(winner) {
 game = 0;
 if (winner) {
 alert(winner.name + " wins!");
 } else alert("it's a draw!");
}

If the alert() was not intrusive, you could write endGame() as:

function gameEnded(winner) {
 gameIsEnded = board.length == 0 && game == 1;
 if (gameIsEnded) {
 warnUser(winner.name + " wins!");
 } else {
 warnUser("It's a draw!");
 }
 return gameIsEnded;
}

Then, in nextTurn(), you could simply return gameEnded(). When you see return true;/return false;, there's usually something to improve.

• Engin Erdogan: this is immensely helpful, I appreciate your detailed feedback, there are some great learnings for me here! To clarify two points that I did not quite understand: (1) What's wrong with the getCurrentPlayer function? Isn't it standard practice to use getters for game-wide variables? (2) Also, you said endGame function feels quite wrong: how would it be better? Again, thank you so much for taking the time to review my code.
• Quentin Pradet: Getters are taught at school, but are not really useful here, right? They can add value when they hide some logic, as in getOtherPlayer. I'll propose a better endGame at the end of my answer.
• Engin Erdogan: Very helpful, great points. I like the rule of thumb for non-descript returns. Thank you so much!