Tic-Tac-Toe Game

I want recommendations on how to improve this code.

JSFiddle for you to play in (It's laggy, so it would be better if you download the images and try it in your own PC).

```


Let's play Tic Tac Toe!


$(document).ready( function() {
var f1;
var f2;
var f3;
var f4;
var f5;
var f6;
var f7;
var f8;
var f9;
var count = 0;

$('#field1').click( function() {
if(f1 != "X" && f1 != "O") {
$(this).css('background-image', 'url(x.png)');
f1 = "X";
count++;
checkVictory();
randomize();
}
});
$('#field2').click( function() {
if(f2 != "X" && f2 != "O") {
$(this).css('background-image', 'url(x.png)');
f2 = "X";
count++;
checkVictory();
randomize();
}
});
$('#field3').click( function() {
if(f3 != "X" && f3 != "O") {
$(this).css('background-image', 'url(x.png)');
f3 = "X";
count++;
checkVictory();
randomize();
}
});
$('#field4').click( function() {
if(f4 != "X" && f4 != "O") {
$(this).css('background-image', 'url(x.png)');
f4 = "X";
count++;
checkVictory();
randomize();
}
});
$('#field5').click( function() {
if(f5 != "X" && f5 != "O") {
$(this).css('background-image', 'url(x.png)');
f5 = "X";
count++;
checkVictory();

Don't Repeat Yourself

There's too much repetition in the code. That's never good. If later you want to change the fields, you will have to make the change in all the places it was duplicated.

Fixing the CSS

You have this kind of CSS repeated for every #field1 ... #field9:

#field1 {
    background: #FFFFFF;
    width: 100px;
    height: 100px;
}

Instead of copy-pasting this 8 times, you could give the containing table a class like this:

And replace the CSS of all those fields with simply this:

.grid td {
    background: #FFFFFF;
    width: 100px;
    height: 100px;
}

Fixing the JavaScript

You have this repeated logic for each field:

$('#field1').click( function() {
    if(f1 != "X" && f1 != "O") {
        $(this).css('background-image', 'url(http://i61.tinypic.com/faz9g3.png)');
        f1 = "X";
        count++;
        checkVictory();
        randomize();
    }
});

There can be better ways, without so much duplication. Basically what you're doing here is:

• Check if the field is available (neither X nor O)

• Set the field to represent X

• Increment the move counter and check the game state

• Let the other player move

Maybe something that will help you here is storing the state of a field inside the dom itself. In HTML5, it's common to use attributes named with the prefix data- to store information. You could store the state of a field inside the attribute data-move (with values X or O), with helper functions like this:

function isAvailableField($this) {
    return ! $this.attr('data-move');
}

function doMove($this, move) {
    $this.attr('data-move', move);
    $this.addClass(move);
}

$('.grid td').click(function() {
    if (isAvailableField($(this))) {
        doMove($(this), 'move-x');
        count++;
        checkVictory();
        doAiMove();
    }
});

Notice the $this.addClass(move); in the doMove helper. To make it work, add this CSS:

.grid td.move-x {
    background-image: url(http://i61.tinypic.com/faz9g3.png);
}
.grid td.move-o {
    background-image: url(http://i61.tinypic.com/ounjtu.png);
}

In this setup, you can toggle the visual state of a field by adding and removing CSS classes. The good thing about this is that it let's you separate the logic from the user interface.

Since $('.grid td').click sets up the click listener for all of the fields, you don't need anymore to handle #field1 ... #field9 individually.

The doMove method sets the internal state of the fields using the data-move attribute of the dom, and it sets the visual state using CSS. Of course, in the reset function we have to clear all that:

function reset() {
    $('.move-x').removeClass('move-x');
    $('.move-o').removeClass('move-o');
    $('.grid td').attr('data-move', '');
    count = 0;
}

I also renamed the randomize method to doAiMove, as that's what it really does: performs the move by the computer. This method should now make use of the isAvailableField and doMove methods:

function doAiMove() {
    var found = false;
    while (!found) {
        var Random = Math.floor(Math.random() * 9) + 1;
        var field = $('#field' + Random);
        if (isAvailableField(field)) {
            found = true;
            doMove(field, 'move-o');
            count++;
            checkVictory();
        }
    }
}

And of course you need to check the victory condition differently. One way to do it is to define in an array the possible winning positions, and using a loop to check if a player is in one of these positions:

var lines = [
    [ 1, 2, 3 ],
    [ 4, 5, 6 ],
    [ 7, 8, 9 ],
    [ 1, 4, 7 ],
    [ 2, 5, 8 ],
    [ 1, 5, 9 ],
    [ 7, 5, 3 ]
];

function hasWon(move) {
    for (var i = 0; i < lines.length; ++i) {
        var line = lines[i];
        var j = 0;
        for (; j < line.length; ++j) {
            var num = line[j];
            if ($('#field' + num).attr('data-move') != move) {
                break;
            }
        }
        if (j == line.length) {
            return true;
        }
    }
}

function checkVictory() {
    if (count < 5) {
        return;
    }
    if (hasWon('move-x')) {
        victory();
    } else if (hasWon('move-o')) {
        defeat();
    } else if (count == 9) {
        draw();
    }
}

You can see it in action here: http://jsfiddle.net/z9axM/

This is far from perfect, but at least the duplications are much reduced. Perhaps you can build on this and make it better.

StackExchange Code Review Q#57823, answer score: 7