Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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).

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf8">
    <title>Let's play Tic Tac Toe!</title>
    <script type="text/javascript" src="jquery.js"></script>
    <script type="text/javascript">
        $(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();
                    randomize();
                }
            });
            $('#field6').click( function() {
                if(f6 != "X" && f6 != "O") {
                    $(this).css('background-image', 'url(x.png)');
                    f6 = "X";
                    count++;
                    checkVictory();
                    randomize();
                }
            });
            $('#field7').click( function() {
                if(f7 != "X" && f7 != "O") {
                    $(this).css('background-image', 'url(x.png)');
                    f7 = "X";
                    count++;
                    checkVictory();
                    randomize();
                }
            });
            $('#field8').click( function() {
                if(f8 != "X" && f8 != "O") {
                    $(this).css('background-image', 'url(x.png)');
                    f8 = "X";
                    count++;
                    checkVictory();
                    randomize();
                }
            });
            $('#field9').click( function() {
                if(f9 != "X" && f9 != "O") {
                    $(this).css('background-image', 'url(x.png)');
                    f9 = "X";
                    count++;
                    checkVictory();
                    randomize();
                }
            });
            function checkVictory() {
                if(f1 == "X" && f2 == "X" && f3 == "X") {
                    victory();
                }
                if(f1 == "X" && f5 == "X" && f9 == "X") {
                    victory();
                }
                if(f3 == "X" && f5 == "X" && f7 == "X") {
                    victory();
                }
                if(f4 == "X" && f5 == "X" && f6 == "X") {
                    victory();
                }
                if(f7 == "X" && f8 == "X" && f9 == "X") {
                    victory();
                }
                if(f1 == "X" && f4 == "X" && f7 == "X") {
                    victory();
                }
                if(f2 == "X" && f5 == "X" && f8 == "X") {
                    victory();
                }
                if(f3 == "X" && f6 == "X" && f9 == "X") {
                    victory();
                }
                if(f1 == "O" && f2 == "O" && f3 == "O") {
                    defeat();
                }
                if(f1 == "O" && f5 == "O" && f9 == "O") {
                    victory();
                }
                if(f3 == "O" && f5 == "O" && f7 == "O") {
                    defeat();
                }
                if(f4 == "O" && f5 == "O" && f6 == "O") {
                    defeat();
                }
                if(f7 == "O" && f8 == "O" && f9 == "O") {
                    defeat();
                }
                if(f1 == "O" && f4 == "O" && f7 == "O") {
                    defeat();
                }
                if(f2 == "O" && f5 == "O" && f8 == "O") {
                    defeat();
                }
                if(f3 == "O" && f6 == "O" && f9 == "O") {
                    defeat();
                }
                if(count == 9) {
                    draw();
                }
            }
            function randomize() {
                var Found = false;
                while(Found != true) {
                    var Random = Math.floor(Math.random() * 9) + 1;
                    if(Random == 1 && f1 != "X" && f1 !="O") {
                        f1 = "O";
                        $('#field1').css('background-image', 'url(o.png)');
                        count++;
                        checkVictory();
                        Found = true;
                    }
                    if(Random == 2 && f2 != "X" && f2 !="O") {
                        f2 = "O";
                        $('#field2').css('background-image', 'url(o.png)');
                        count++;
                        checkVictory();
                        Found = true;
                    }
                    if(Random == 3 && f3 != "X" && f3 !="O") {
                        $('#field3').css('background-image', 'url(o.png)');
                        f3 = "O";
                        count++;
                        checkVictory();
                        Found = true;
                    }
                    if(Random == 4 && f4 != "X" && f4 !="O") {
                        f4 = "O";
                        $('#field4').css('background-image', 'url(o.png)');
                        count++;
                        checkVictory();
                        Found = true;
                    }
                    if(Random == 5 && f5 != "X" && f5 !="O") {
                        f5 = "O";
                        $('#field5').css('background-image', 'url(o.png)');
                        count++;
                        checkVictory();
                        Found = true;
                    }
                    if(Random == 6 && f6 != "X" && f6 !="O") {
                        f6 = "O";
                        $('#field6').css('background-image', 'url(o.png)');
                        count++;
                        checkVictory();
                        Found = true;
                    }
                    if(Random == 7 && f7 != "X" && f7 !="O") {
                        f7 = "O";
                        $('#field7').css('background-image', 'url(o.png)');
                        count++;
                        checkVictory();
                        Found = true;
                    }
                    if(Random == 8 && f8 != "X" && f8 !="O") {
                        f8 = "O";
                        $('#field8').css('background-image', 'url(o.png)');
                        count++;
                        checkVictory();
                        Found = true;
                    }
                    if(Random == 9 && f9 != "X" && f9 !="O") {
                        f9 = "O";
                        $('#field9').css('background-image', 'url(o.png)');
                        count++;
                        checkVictory();
                        Found = true;
                    }
                }
                Found = false;
            }
            function reset() {
                f1 = 0;
                f2 = 0;
                f3 = 0;
                f4 = 0;
                f5 = 0;
                f6 = 0;
                f7 = 0;
                f8 = 0;
                f9 = 0;
                $('#field1').css('background-image', 'none');
                $('#field2').css('background-image', 'none');
                $('#field3').css('background-image', 'none');
                $('#field4').css('background-image', 'none');
                $('#field5').css('background-image', 'none');
                $('#field6').css('background-image', 'none');
                $('#field7').css('background-image', 'none');
                $('#field8').css('background-image', 'none');
                $('#field9').css('background-image', 'none');
                count = 0;
            }
            function draw() {
                alert('It is a draw!');
                reset();
            }
            function victory() {
                alert('You win!');
                reset();
            }
            function defeat() {
                alert("You lost! Better luck next time!");
                reset();
            }
        });

    </script>
    <style type="text/css">
        body {
            background: #000;
        }
        #field1 {
            background: #FFFFFF;
            width: 100px;
            height: 100px;
        }
        #field2 {
            background: #FFFFFF;
            width: 100px;
            height: 100px;
        }
        #field3 {
            background: #FFFFFF;
            width: 100px;
            height: 100px;
        }
        #field4 {
            background: #FFFFFF;
            width: 100px;
            height: 100px;

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

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

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

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

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

        }
    </style>
</head>
<body>
    <table width="300" height="300">
        <tr>
            <td><div id="field1"></div></td>
            <td><div id="field2"></div></td>
            <td><div id="field3"></div></td>
        </tr>
        <tr>
            <td><div id="field4"></div></td>
            <td><div id="field5"></div></td>
            <td><div id="field6"></div></td>
        </tr>
        <tr>
            <td><div id="field7"></div></td>
            <td><div id="field8"></div></td>
            <td><div id="field9"></div></td>
        </tr>
    </table>
</body>
</html>
share|improve this question

3 Answers 3

up vote 7 down vote accepted

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:

<table width="300" height="300" class="grid">
    <tr>
        <td id="field1"></td>
        <td id="field2"></td>
        <td id="field3"></td>
    </tr>
    <tr>
        <td id="field4"></td>
        <td id="field5"></td>
        <td id="field6"></td>
    </tr>
    <tr>
        <td id="field7"></td>
        <td id="field8"></td>
        <td id="field9"></td>
    </tr>
</table>

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:

  1. Check if the field is available (neither X nor O)
  2. Set the field to represent X
  3. Increment the move counter and check the game state
  4. 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.

share|improve this answer
    
Quite a fantastic answer, Janos! –  AlexeyMK Jul 23 at 20:33
    
update janos' answer to rely on css for x/o instead of images. and changed background declaration to background-color to reduce browser repaints. if you want to use the images, you can make them into a sprite, so its one less http request jsfiddle.net/z9axM/1 –  albert Jul 23 at 22:46
    
Thank you janos and @albert –  Hassan Althaf Jul 24 at 6:35
    
@albert it makes sense to explain your point in an answer, I'd upvote it! (and bring others too, see: meta.codereview.stackexchange.com/questions/2106/…) –  janos Jul 24 at 6:38
    
What does return ! $this.attr('data-move'); do @janos –  Hassan Althaf Jul 24 at 10:23

Here are some quick comments on your HTML and CSS.

I will leave the JS for someone else.

Move CSS/JS to their own files

You should split your CSS to a css file, and your JavaScript to a JS file.

<script src="tictactoe.js"></script>
<link rel="stylesheet" type="text/css" href="tictactoe.css">

Better CSS

When you're repeating a lot of CSS, it probably can be condensed. Your particular CSS can be reduced to just this:

body {
    background: #000;
}
td>div {
    background: #FFFFFF;
    width: 100px;
    height: 100px;
}

(td>div means all divs that are direct children of table cells.)

Better HTML

Why do you have a div inside the td? I am sure you can work directly with the td.

Additionally, you are setting styles in CSS, which is not good. Move them to CSS. (width and height.)

share|improve this answer
    
Even though it doesn't have much detail, I will still thank you for your effort. Thank you. –  Hassan Althaf Jul 24 at 6:36

@janos gave a superb answer, I'd just like to add a few things. Instead of relying on images for the x and o, you can use css via content property; if you prefer images, then instead of using two, i'd combine them into one (aka a sprite) and use css to show them, so you have one less http request.
i also changed the background declarations to the shorthand background-color, so the browser has less repaints. update janos' answer to rely on css for x/o instead of images. and changed background declaration to background-color to reduce browser repaints.
http://jsfiddle.net/z9axM/1

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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