19
\$\begingroup\$

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>
Improve this question
\$\endgroup\$
0

2 Answers 2

Reset to default
11
\$\begingroup\$

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.

Improve this answer
\$\endgroup\$
1
  • 6
    \$\begingroup\$ Nice thing about CR is that we don't need to read the question to learn from the answers :D \$\endgroup\$
    – brasofilo
    Aug 23, 2014 at 19:36
3
\$\begingroup\$

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

Improve this answer
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.