Code Review

Code Review

Project 0 Tic-Tac-Toe

Tic-Tac-Toe-Wars

I have been told that making Tic-Tac-Toe is a right of passage for all beginner programmers, sort of like a 'Hello World' 2.0. So here I am doing a mid-week review of the code for my assignment due tomorrow. Who doesn't love cranking up the pressure by adding blog writing to the middle of a 1-day project rebuild? So let's get into it before I have to hand in a wireframe prototype, wish me luck.

So, all of my game data is stored in a single object in the global scope. This allows me to pull whatever data I need at any stage in the program. It does add an extra level of digging everywhere, but it still pays off in my books, so I am leaving this formatted as is for the time being. The only change I will make is to better player tokens that will fit my new theme.

const gamestate={
    playerNames:["player-One","player-Two"],
    playerSymbols:['img/x.jpeg','img/0.png'],
    scores:[0,0],
    currentTurn: 0,
    board: ["","","","","","","","",""],
    roundCount:1,
    topLeft: ["",0],
    topCen: ["",1],
    topRight: ["",2],
    midLeft: ["",3],
    midCen: ["",4],
    midRight: ["",5],
    bottomLeft: ["",6],
    bottomCen: ["",7],
    bottomRight: ["",8],
}

The next few functions are just housekeeping and will be left the same for the moment too. buildBoard() fills out the gamestate.board array, and coinFlip() generates the initial turn order and startGame() runs both.

function buildBoard(){
    let board = gamestate.board;
    board[0]= gamestate.topLeft[0];
    board[1]= gamestate.topCen[0];
    board[2]= gamestate.topRight[0];
    board[3]= gamestate.midLeft[0];
    board[4]= gamestate.midCen[0];
    board[5]= gamestate.midRight[0];
    board[6]= gamestate.bottomLeft[0];
    board[7]= gamestate.bottomCen[0];
    board[8]= gamestate.bottomRight[0];
    return board
}

function coinFlip(){
    return Math.floor(Math.random()*2);
}

function startGame(){
    currentTurn=coinFlip();
    gamestate.board = buildBoard();
}

The next few sections of code are nested in $(document).ready(function() for most of my event listeners. Some of this code will be reworked because instead of utilizing pop ups my HTML now has full content sections that will be swapped out, but there is a lot of code that can be worked into functions and made both drier and more readable.

    $('#start').on('click', function(){
        gamestate.currentTurn = coinFlip();
        gamestate.playerNames[0]=$('#playerOneName').val();
        gamestate.playerNames[1]=$('#playerTwoName').val();
        var input = document.getElementById("playerOnePic");
        var fReader = new FileReader();
        fReader.readAsDataURL(input.files[0]);
        fReader.onloadend = function(event){
            gamestate.playerSymbols[0] = event.target.result;
        }
        var input = document.getElementById("playerTwoPic");
        var fReader = new FileReader();
        fReader.readAsDataURL(input.files[0]);
        fReader.onloadend = function(event){
            gamestate.playerSymbols[1] = event.target.result;
        }
        updateScreenInfo();
        $(".container").removeClass('blur');
        $('.start').addClass('hidden')
    })

This code triggered an initialisation once players' names and custom icons had been chosen and loaded. I have scrapped the custom icons for a more thematic approach and am changing it to be a bit more readable.

    $('#destiny').on('click',starwarsScroller)

    function starwarsScroller(){
        // console.log('click')
        gamestate.playerNames[0] = $('#JediInput').val();
        gamestate.playerNames[1]= $('#SithInput').val();
        $('#jediName').html(gamestate.playerNames[0]);
        $('#sithName').html(gamestate.playerNames[1]);
        startGame();
        $('#getNames').addClass('hidden');
        $('#scrollingText').removeClass('hidden');
        setTimeout(fromScroller,30000);
    }
    function startGame(){
        currentTurn= Math.floor(Math.random()*2);
        gamestate.board = buildBoard();
        updateScreenInfo();
    }
    function updateScreenInfo(){
        $('#playerOneName').html(`Player One: ${gamestate.playerNames[0]}`);
        $('#playerTwoName').html(`Player Two: ${gamestate.playerNames[1]}`);
        $('#playerOneScore').html(`Score: ${gamestate.scores[0]}`);
        $('#playerTwoScore').html(`Score: ${gamestate.scores[1]}`);
        $('#currentTurn').html(`${gamestate.playerNames[gamestate.currentTurn]}'s attack`)
        $('#roundCount').html(`Round ${gamestate.roundCount}:`)
    }

The new UI is Star Wars themed because I like Star Wars and lightsabers were about the only thing cool enough to make me play with CSS and have it look pretty. I will be doing next week's code review on the CSS and animation side of this project. It comes with a scrolling text scene complete with custom names in the text. This scene will only ever launch on the first play through, from here players will be able to play more games without having to sit through it again. While that is all happening I will initialise the game screen information so that everything is populated before we start. Depending on time I am planning to have a random generator add some styling to the names and give a profile picture. For the sake of getting a working game I will add all that fluff in later.

This next section is where I begin to feel that my code becomes hard to read. Having everything contained in an object causes a lot of gamestate's to be typed everywhere. So on top of drying up the code, hopefully I will make this section more readable.

    $('.gameSquare').on('click',function(e){
        const gameSquare = gamestate[e.target.id];
        let turn = gamestate.currentTurn
        if(gameSquare[0]===''){ 
            $(e.target).html(`<img src="${gamestate.playerSymbols[turn]}"></img>`) 
            gameSquare[0] = gamestate.playerNames[turn]
            buildBoard();
            checkWin(gameSquare[0], gameSquare[1]); 
            roundCount(); 
            if(turn===0){ 
                gamestate.currentTurn=1;
            }else{
                gamestate.currentTurn=0;
            }
        }else{
            console.log('already taken')
        }
        updateScreenInfo();
    })

Apparently, I did not copy the original, so this is actually the improved version, the turn variable cleaned up a lot of repeated parts made it a little more readable.

This is the section of code that I believe needs a lot of work. The premise is simple, starting from the chosen square the function will check for vertical win, horizontal win and 2 diagonal wins. The code ended up very similar for all the different directions. So my hope is to create a single function that I can call for the 4 different directions. ``` function checkWin(player,index){ checkVert(player,index); checkHori(player,index); checkPosDiag(player,index); checkNegDiag(player,index); }

function checkVert(player,index){ let win = 0; let checkIndex = index+3; const board = gamestate.board if(checkIndex<9 && board[checkIndex]===player){ win++ checkIndex+=3 if (win<2 && checkIndex<9 && board[checkIndex]===player){ win++; }; }; checkIndex = index-3; if(checkIndex>=0 && board[checkIndex]===player){ win++ checkIndex-=3 if (win<2 && checkIndex>=0 && board[checkIndex]===player){ win++; }; }; if(win>=2){ gameOver(); } }

function checkPosDiag(player,index){ let win = 0; let checkIndex = index+4; const board = gamestate.board; if(checkIndex<9 && board[checkIndex]===player){ win++; checkIndex+=4; if (win<2 && checkIndex<9 && board[checkIndex]===player){ win++; }; }; checkIndex = index-4; if(checkIndex>=0 && board[checkIndex]===player){ win++ checkIndex-=4; if (win<2 && checkIndex>=0 && board[checkIndex]===player){ win++; }; }; if(win>=2){ gameOver(); } }

function checkNegDiag(player,index){ let win = 0; let checkIndex = index+2; const board = gamestate.board; if(checkIndex<9 && board[checkIndex]===player){ win++; checkIndex+=2; if (win<2 && checkIndex<9 && board[checkIndex]===player){ win++; }; }; checkIndex = index-2; if(checkIndex>=0 && board[checkIndex]===player){ win++; checkIndex-=2; if (win<2 && checkIndex>=0 && board[checkIndex]===player){ win++; }; }; if(win>=2){ gameOver(); } }

function checkHori(player,index){ let win = 0; const row = Math.floor(index/3) let checkIndex = index+1; let checkRow= Math.floor(checkIndex/3) const board = gamestate.board; if(checkIndex<9 && board[checkIndex]===player && row===checkRow){ win++; checkIndex+=1; checkRow= Math.floor(checkIndex/3) if (win<2 && checkIndex<9 && board[checkIndex]===player && row===checkRow){ win++; }; }; checkIndex = index-1; checkRow= Math.floor(checkIndex/3) if(checkIndex>=0 && board[checkIndex]===player && row===checkRow){ win++ checkIndex-=1; checkRow= Math.floor(checkIndex/3) if (win<2 && checkIndex>=0 && board[checkIndex]===player && row===checkRow){ win++; }; }; if(win>=2){ gameOver(); } } ```

I succeded in making a function to work for 3/4.

function check(player,index,increment){
    let win = 0;
    let checkIndex = index+increment;
    const board = gamestate.board
    if(validate(checkIndex,win,board[checkIndex],player)){
        win++
        checkIndex+=increment
        if (validate(checkIndex,win,board[checkIndex],player)){
            win++;
        };
    };
    checkIndex = index-increment;
    if(validate(checkIndex,win,board[checkIndex],player)){
        win++
        checkIndex-=increment
        if (validate(checkIndex,win,board[checkIndex],player)){
            win++;
        };
    };
    if(win>=2){
        gameOver();
    }
}

function validate(checkIndex,win,square,player){
    return checkIndex <9 && checkIndex>=0&& win<2 && square===player
}

Horizontal has an extra check feature to make sure that the checked squared are on the same row. I could not figure out how to integrate this without causing more headaches, but who knows, by the time I finish the assignment and hand it in I may have figured it out.

The only remaining javascript that I need for the game is a win message and a reset game. I intend to add a lot of randomised quotes, sound bites and win messages, so for the moment these are pretty bland. I will cover all the cool Star Wars fluff with my CSS blog.

function gameOver(){
    alert('win')
    gamestate.scores[gamestate.currentTurn]++  
//random end of game response here
}

function resetGame(){
    console.log('play again')
    gamestate.roundCount=1
    gamestate.topLeft= ["",0]
    gamestate.topCen= ["",1]
    gamestate.topRight= ["",2]
    gamestate.midLeft= ["",3]
    gamestate.midCen= ["",4]
    gamestate.midRight= ["",5]
    gamestate.bottomLeft= ["",6]
    gamestate.bottomCen= ["",7]
    gamestate.bottomRight= ["",8]
    buildBoard();
    updateScreenInfo();
    $('.gameSquare').className='gameSquare';
    $('.gameSquare').html('');
}

Conclusion

At the monthly #RORO event I attended this week, I was told about an engineering philosophy where from the outset you plan to build 3 versions of anything, scrapping and learning from the first two. After doing these code reviews I can appreciate this mindset. When I started code the win conditions originally I did not understand the process well enough to make an efficient code, but there is a little part of me that hates the idea of scrapping my work and starting over. I should try and adopt this mentality when I am working through larger projects. Making something knowing that I am trashing it shortly will hopefully make me more open to reworking my code sources.

Last Minute Updates

So about 90 minutes before presentations I came across a weird anomaly that gave false victories. The incrementation of checkIndex was giving a few false positives where it wasn't on the correct row to be valid. Much like the checkHori() function I just used a Math.floor() method to calculate the row and make sure it was appropriate. This might have actually made it easier to use a single function for all 4, however I was too busy just getting it to work in time for the presentation to worry about refactoring it again.

Conclusion 2.0

This bug was not new, it was a flaw in the logic from the beginning that I missed in testing. So my second learning point for the week is to test more thoroughly, earlier, and to test of false results as well as expected ones.