1
\$\begingroup\$

I am a beginner at web development and I write code as a hobby. Would you please critique this code? I created a Tic-Tac-Toe game using HTML/CSS and jQuery. I did it for fun as a personal challenge. An image of a game board shows up initially. Each empty square can be clicked. Depending on if the turn is "even" or "odd", an X or an O image will appear in the chosen square. There is an X and an O image hidden in each square that only appears when it is activated with a click.

HTML - 52 lines

<!DOCTYPE html>
<html>
    <head>
        <title>Tic Tac Toe</title>
        <link rel='stylesheet' type='text/css' href='stylesheet.css'/>
    </head>
    <body>
        <div id='gameSpace'>
            <img id='board' src='assets/board.jpg' />
            <div id='pieces'>
                <div class='spot' id='spot1' >
                    <img id='O1' hidden src='assets/O.jpg' />
                    <img id='X1' hidden src='assets/X.jpg' />
                </div>
                <div class='spot' id='spot2' >
                    <img id='O2' hidden src='assets/O.jpg' />
                    <img id='X2' hidden src='assets/X.jpg' />
                </div>
                <div class='spot' id='spot3' >
                    <img id='O3' hidden src='assets/O.jpg' />
                    <img id='X3' hidden src='assets/X.jpg' />
                </div>
                <div class='spot' id='spot4' >
                    <img id='O4' hidden src='assets/O.jpg' />
                    <img id='X4' hidden src='assets/X.jpg' />
                </div>
                <div class='spot' id='spot5' >
                    <img id='O5' hidden src='assets/O.jpg' />
                    <img id='X5' hidden src='assets/X.jpg' />
                </div>
                <div class='spot' id='spot6' >
                    <img id='O6' hidden src='assets/O.jpg' />
                    <img id='X6' hidden src='assets/X.jpg' />
                </div>
                <div class='spot' id='spot7' >
                    <img id='O7' hidden src='assets/O.jpg' />
                    <img id='X7' hidden src='assets/X.jpg' />
                </div>
                <div class='spot' id='spot8' >
                    <img id='O8' hidden src='assets/O.jpg' />
                    <img id='X8' hidden src='assets/X.jpg' />
                </div>
                <div class='spot' id='spot9' >
                    <img id='O9' hidden src='assets/O.jpg' />
                    <img id='X9' hidden src='assets/X.jpg' />
                </div>
            </div>
        </div>
        <script src='bower_components/jquery/dist/jquery.js' ></script>
        <script src='script.js' ></script>
    </body>
</html>

CSS - 150 lines

* {
    margin:0px;
    padding:0px;
    font-family:Helvetica;
}

body {
    background-color:#4facd8;
    color:#0b2633;
}

header {
    position:fixed;
    width:100%;
    height:3.5em;
    margin: 0px;
    background-color:#7bc0e2;
    text-shadow: 0 1px 2px black;
    padding: 3px 0 3px 0;
    box-shadow: 0 3px 6px black;
    z-index:2;
}

header > h1 {
    font-size:2.5em;
    z-index:20;
    padding-left:.2em;
    color:white;
}

h2 {
    font-size:1.5em;
}

.listManager {
    position:fixed;
    margin-top:3.5em;
    padding-top:3em;
    width:18em;
    height:100%;
    background-color:#a7d5ec;
    z-index:0;
    box-shadow: 0 3px 6px black;
}

.task {
    width:15em;
    height:2em;
    display:block;
    margin-left:auto;
    margin-right:auto;
    font-size:1em;
}

.priorityTable {
    margin-top:2em;
    display:block;
    padding:0px;
}

.urgent {
    color:red;
}

.important {
    color:green;
}

tbody {
    display:block;
}

.trow {
    display:block;
    width:100%;
    height:6em;
}

.tdata {
    display:inline-block;
    width:49%;
    height:3.5em;
    font-size:1.2em;
    border-radius:10px;
    box-shadow: 0 2px 4px black;
    text-align:center;
    padding-top:15px;
    background-color:white;
    text-shadow: 0 .3px .6px black;
}

.selected {
    box-shadow: 0 0 0 black;
    background-color:yellow;
}

.time {
    margin-top:.5em;
    height:3.5em;
    text-align:center;
    font-size:2em;
    text-shadow: 0 1px 2px black;
    color:white;
}

.time > li {
    font-size:4em;
    text-align:center;
    height:.6em;
}

.selectedTime {
    color:yellow;
}

.addTask {
    display:block;
    margin:auto;
    margin-top:5em;
    font-size:2.5em;
    text-shadow: 0 1px 2px black;
    color:#0b2633;
}

.mainList {
    position:absolute;
    left:10em;
    top:3em;
    font-size:2.2em;
    text-shadow: 0 1px 2px black;
    color:#d3eaf5;
}

.mainList > ol {

}

.mainList > ol > li {

}

.completed {
    text-decoration:line-through;
}

a {
    text-decoration:none;
    text-shadow: 0 1px 2px black;
    color:#d3eaf5;
}

jQuery - 125 lines

var sign = [0, 1];
var turn = 0;
var topLeftSquare = 0;
var topMiddleSquare = 0;
var topRightSquare = 0;
var middleLeftSquare = 0;
var middleMiddleSquare = 0;
var middleRightSquare = 0;
var bottomLeftSquare = 0;
var bottomMiddleSquare = 0;
var bottomRightSquare = 0;

if (turn%2 === 0){

}
$(document).ready(function(){
   $('#spot1').click(function(){
        if(topLeftSquare === 0){
            if (turn%2 === 0){
                $('#X1').fadeIn('fast');
           }else{
                $('#O1').fadeIn('fast');
           };
           turn++;
            topLeftSquare++;
        }
   });

    $('#spot2').click(function(){
        if(topMiddleSquare === 0){
            if (turn%2 === 0){
                $('#X2').fadeIn('fast');
           }else{
                $('#O2').fadeIn('fast');
           };
           turn++;
            topMiddleSquare++;
        }
   });

    $('#spot3').click(function(){
        if(topRightSquare === 0){
            if (turn%2 === 0){
                $('#X3').fadeIn('fast');
           }else{
                $('#O3').fadeIn('fast');
           };
           turn++;
            topRightSquare++;
        }
   });

    $('#spot4').click(function(){
        if(middleLeftSquare === 0){
            if (turn%2 === 0){
                $('#X4').fadeIn('fast');
           }else{
                $('#O4').fadeIn('fast');
           };
           turn++;
            middleLeftSquare++;
        }
   });

    $('#spot5').click(function(){
        if(middleMiddleSquare === 0){
            if (turn%2 === 0){
                $('#X5').fadeIn('fast');
           }else{
                $('#O5').fadeIn('fast');
           };
           turn++;
            middleMiddleSquare++;
        }
   });

    $('#spot6').click(function(){
        if(middleRightSquare === 0){
            if (turn%2 === 0){
                $('#X6').fadeIn('fast');
           }else{
                $('#O6').fadeIn('fast');
           };
           turn++;
            middleRightSquare++;
        }
   });

    $('#spot7').click(function(){
        if(bottomLeftSquare === 0){
            if (turn%2 === 0){
                $('#X7').fadeIn('fast');
           }else{
                $('#O7').fadeIn('fast');
           };
           turn++;
            bottomLeftSquare++;
        }
   });

    $('#spot8').click(function(){
        if(bottomMiddleSquare === 0){
            if (turn%2 === 0){
                $('#X8').fadeIn('fast');
           }else{
                $('#O8').fadeIn('fast');
           };
           turn++;
            bottomMiddleSquare++;
        }
   });

    $('#spot9').click(function(){
        if(bottomRightSquare === 0){
            if (turn%2 === 0){
                $('#X9').fadeIn('fast');
           }else{
                $('#O9').fadeIn('fast');
           };
           turn++;
            bottomRightSquare++;
        }
   });

});
\$\endgroup\$
2
\$\begingroup\$

Whenever you have repeated blocks of very similar code, you should always seek to merge them into one generalized block. Sometimes this exercise is hard, sometimes simple, depending on the nature of the algorithms/paradigms embodied in the code.

Here, without changing the HTML or the CSS, the javascript will simplify to :

$(document).ready(function() {
    var turn = 0;
    $('.spot').click(function() {
        if(!$(this).data('filled')) {
            $(this).data('filled', 1).find('img').eq(turn++%2).fadeIn('fast');
        }
    });
});

Very often a generalization is hard to spot when you are developing code. It is often easier, while developing, to write the everything out in its long form, then seek to generalize as part of a tidying exercise when it is working.

The more experience you acquire, the more likely you will become at spotting a generalization early, but even experienced programmers will often write verbose code before boiling it down to something more manageable.

\$\endgroup\$
  • \$\begingroup\$ This simplification works as intended. The only issue is that now the O image is the first piece that is used instead of the X image. I set the initial value of the turn variable from 0 to 1. This causes the X image to be the first piece used. \$\endgroup\$ – tmachado Dec 31 '15 at 4:41
  • 1
    \$\begingroup\$ Yes, that's the simplest way to make "X" to play first. Another way would be to swap each pair of <img> tags in the HTML, and revert turnto a starting value of `0. \$\endgroup\$ – Roamer-1888 Dec 31 '15 at 5:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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