Tic Tac Toe in plain JavaScript

Question 1

I have created Tic Tac Toe game using plain JavaScript. Can any javascript expert review it and let me know whether the way I have written JavaScript code that is right (preferable)?

I am unsure that I wrote the OnLoad function in a preferable way. I wanted wrap all my code in an immediate invoking function but that didn't work out well.

I just want to improve my JavaScript coding knowledge. I want to write JS code more professional way.

CodePen

HTML

<html>
<title>Tic Tac Toe</title>
<head>
<link rel="stylesheet" type="text/css" href="ticTac.css" />
<script type="text/javascript" src="ticTac.js"></script> 
</head>
<body onload="fnLoad()">
 <div class="container">
 <div>
 <select id="grid">
 </select>
 <button name="NewGame" class="newGame" value="Start a New Game" onClick="fnNewGame()">Start a New Game</button>
 </div>
 <div class="minContainer">
 <table class="row" id="game"></table>
 </div>
 </div>
</body>
</html>

JS

var turn = 'X';
var score = {
 'X': 0,
 'O': 0
};
var gridValue = 0;
function fnLoad() {
 var select = document.getElementById("grid");
 for (i = 3; i <= 100; i += 1) {
 var option = document.createElement('option');
 select.options[select.options.length] = new Option(i + ' X ' + i, i);
 }
 addEvent(document.getElementById("game"), "click", fnChoose);
 fnNewGame();
}
function addEvent(element, eventName, callback) {
 if (element.addEventListener) {
 element.addEventListener(eventName, callback, false);
 } else if (element.attachEvent) {
 element.attachEvent("on" + eventName, callback);
 }
}
function fnChoose(e) {
 if (e.target && e.target.nodeName == "TD") {
 var targetElement = document.getElementById(e.target.id);
 var prevTurn;
 if ((targetElement.className).indexOf("disabled") == -1) {
 targetElement.innerHTML = turn;
 targetElement.classList.add('disabled');
 targetElement.classList.add(turn);
 score[turn] += 1;
 prevTurn = turn;
 turn = turn === "X" ? "O" : "X";
 if (fndecide(targetElement, prevTurn)) {
 alert(prevTurn + ' has won the game');
 fnNewGame();
 } else if ((score['X'] + score['O']) == (gridValue * gridValue)) {
 alert('Draw!');
 fnNewGame();
 }
 }
 }
}
function fndecide(targetElement, prevTurn) {
 var UL = document.getElementById('game');
 var elements, i, j, cnt;
 if (score[prevTurn] >= gridValue) {
 var classes = targetElement.className.split(/\s+/);
 for (i = 0; i < classes.length; i += 1) {
 cnt = 0;
 if (classes[i].indexOf('row') !== -1 || classes[i].indexOf('col') !== -1 || classes[i].indexOf('dia') !== -1) {
 elements = UL.getElementsByClassName(classes[i]);
 for (j = 0; j < elements.length; j += 1) {
 if (elements[j].innerHTML == prevTurn) {
 cnt += 1;
 }
 }
 if (cnt == gridValue) {
 return true;
 }
 }
 }
 }
 return false;
}
function fnNewGame() {
 var gameUL = document.getElementById("game");
 if (gameUL.innerHTML !== '') {
 gameUL.innerHTML = null;
 score = {
 'X': 0,
 'O': 0
 };
 turn = 'X';
 gridValue = 0;
 }
 var select = document.getElementById("grid");
 gridValue = select.options[select.selectedIndex].value;
 var i, j, li, k = 0,
 classLists;
 var gridAdd = +gridValue + 1;
 for (i = 1; i <= gridValue; i += 1) {
 tr = document.createElement('tr');
 for (j = 1; j <= gridValue; j += 1) {
 k += 1;
 li = document.createElement('td');
 li.setAttribute("id", 'li' + k);
 classLists = 'td row' + i + ' col' + j;
 if (i === j) {
 classLists = 'td row' + i + ' col' + j + ' dia0';
 }
 if ((i + j) === gridAdd) {
 classLists = 'td row' + i + ' col' + j + ' dia1';
 }
 if (!isEven(gridValue) && (Math.round(gridValue / 2) === i && Math.round(gridValue / 2) === j))
 classLists = 'td row' + i + ' col' + j + ' dia0 dia1';
 li.className = classLists;
 tr.appendChild(li);
 }
 gameUL.appendChild(tr);
 }
}
function isEven(value) {
 if (value % 2 == 0)
 return true;
 else
 return false;
}

CSS

*{
 -webkit-box-sizing: border-box; 
 -moz-box-sizing: border-box; 
 box-sizing: border-box; 
}
select {
 padding:3px;
 margin: 0;
 -webkit-border-radius:4px;
 -moz-border-radius:4px;
 border-radius:4px;
 -webkit-box-shadow: 0 3px 0 #ccc, 0 -1px #fff inset;
 -moz-box-shadow: 0 3px 0 #ccc, 0 -1px #fff inset;
 box-shadow: 0 3px 0 #ccc, 0 -1px #fff inset;
 background: #f8f8f8;
 color:#888;
 border:none;
 outline:none;
 display: inline-block;
 -webkit-appearance:none;
 -moz-appearance:none;
 appearance:none;
 cursor:pointer;
 width:100px;
}
.minContainer {
 padding: 20px;
 padding-right: 30px;
 position: absolute;
}
table {
 border-collapse:collapse;
 table-layout: fixed;
 border-spacing: 0;
}
.td {
 border: 2px solid #cccccc;
 font-size:20px;
 font-family:"Helvetica Neue", Helvetica, Arial, sans-serif;
 color:#ccc;
 line-height: 1.428571429;
 width: 30px;
 height: 32px;
 min-width: 32px;
 max-width: 32px;
 min-height: 32px;
 max-height: 32px;
 text-align: center;
}
.X{
 background-color: #FF8362; 
}
.O{
 background-color: #BB7365;
}
</style>

Question 2

I'm not a JavaScript expert and you might want to wait for one. But here are a few things that stand out.

Variable declaration

You forgot to declare your loop variables. Instead of:

for (i = 3; i <= 100; i += 1) {
 // ... 
}

You should write like this:

for (var i = 3; i <= 100; ++i) {
 // ...
}

That is, declare the variable with var i. You should correct this in all your loops. And a tr variable in fnNewGame, should have been:

var tr = document.createElement('tr');

Also, it's customary to write ++i or i++ instead of i += 1.

Simplify `isEven`

There's no need to wrap a boolean expression in an if-else just to return a boolean:

function isEven(value) {
 if (value % 2 == 0)
 return true;
 else
 return false;
}

You can use the result of a boolean expression directly:

function isEven(value) {
 return value % 2 == 0;
}

Simplify notation

You don't have to, but you can write some expressions in a simpler way:

var score = {
 'X': 0,
 'O': 0
};

You can omit the quoting of the key names and write simpler as:

var score = {
 X: 0,
 O: 0
};

And you can access score.X and score.O instead of score['X'] and score['O'].

Unused variables

Remove unused variables, for example this:

var option = document.createElement('option');

Check yourself

To avoid common mistakes, simply copy-paste your code on http://jshint.com/ and fix the warnings it detects.

Question 3

Nice thing about CR is that we don't need to read the question to learn from the answers :D

Question 4

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

~~(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで)~~ (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>};.

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

janos janosjanos 113k15 gold badges154 silver badges396 bronze badges · Answer 1 · 2014-08-23 13:35:11Z

I'm not a JavaScript expert and you might want to wait for one. But here are a few things that stand out.

Variable declaration

You forgot to declare your loop variables. Instead of:

for (i = 3; i <= 100; i += 1) {
 // ... 
}

You should write like this:

for (var i = 3; i <= 100; ++i) {
 // ...
}

That is, declare the variable with var i. You should correct this in all your loops. And a tr variable in fnNewGame, should have been:

var tr = document.createElement('tr');

Also, it's customary to write ++i or i++ instead of i += 1.

Simplify `isEven`

There's no need to wrap a boolean expression in an if-else just to return a boolean:

function isEven(value) {
 if (value % 2 == 0)
 return true;
 else
 return false;
}

You can use the result of a boolean expression directly:

function isEven(value) {
 return value % 2 == 0;
}

Simplify notation

You don't have to, but you can write some expressions in a simpler way:

var score = {
 'X': 0,
 'O': 0
};

You can omit the quoting of the key names and write simpler as:

var score = {
 X: 0,
 O: 0
};

And you can access score.X and score.O instead of score['X'] and score['O'].

Unused variables

Remove unused variables, for example this:

var option = document.createElement('option');

Check yourself

To avoid common mistakes, simply copy-paste your code on http://jshint.com/ and fix the warnings it detects.

Nice thing about CR is that we don't need to read the question to learn from the answers :D

tim timtim 25.3k3 gold badges31 silver badges76 bronze badges · Answer 2 · 2014-08-24 11:39:31Z

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

~~(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで)~~ (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>};.

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

Stack Exchange Network

Tic Tac Toe in plain JavaScript

2 Answers 2

Variable declaration

Simplify `isEven`

Simplify notation

Unused variables

Check yourself

JavaScript

CSS

HTML

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Hot Network Questions

Tic Tac Toe in plain JavaScript

2 Answers 2

Variable declaration

Simplify isEven

Simplify notation

Unused variables

Check yourself

JavaScript

CSS

HTML

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Related

Hot Network Questions

Simplify `isEven`