Connect Four variant, allowing only horizontal and vertical wins

By Posted on

Problem

I’m brand new in JavaScript (and in programming in general). I’ve made a Connect Four game that checks for the winner vertically and horizontally (I haven’t figured out how to solved it diagonally).

Could any of you point out any mistakes I’ve made in this program, or/and how to improve the code?

HTML:

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Fire på Stribe</title>
</head>
<center>
<br/>
<div style="font-family:Arial,sans-serif; font-size:14pt; font-weight:bold; color: black" id="p1">Velkommen til fire på stribe</div><br/>
<body bgcolor="#87cefa">
<div style="font-family:Arial,sans-serif; font-size:14pt; font-weight:bold; color: blue" id="spiller1"></div>
<div style="font-family:Arial,sans-serif; font-size:14pt; font-weight:bold; color: red" id="spiller2"></div><br/>
<button style="background-color: antiquewhite; border-radius: 16px" id = "col1" onclick = "putPieceInColumnNo(0);"> - - 1 - -</button>
<button style="background-color: antiquewhite; border-radius: 16px" id = "col2" onclick = "putPieceInColumnNo(1);"> - - 2 - -</button>
<button style="background-color: antiquewhite; border-radius: 16px" id = "col3" onclick = "putPieceInColumnNo(2);"> - - 3 - -</button>
<button style="background-color: antiquewhite; border-radius: 16px" id = "col4" onclick = "putPieceInColumnNo(3);"> - - 4 - -</button>
<button style="background-color: antiquewhite; border-radius: 16px" id = "col5" onclick = "putPieceInColumnNo(4);"> - - 5 - -</button>
<button style="background-color: antiquewhite; border-radius: 16px" id = "col6" onclick = "putPieceInColumnNo(5);"> - - 6 - -</button>
<button style="background-color: antiquewhite; border-radius: 16px" id = "col7" onclick = "putPieceInColumnNo(6);"> - - 7 - -</button><br>
<canvas id="mycanvas" width="420" height="420"></canvas>
</center>
<center><button style="border-radius:16px; background-color: antiquewhite" id="restart" onclick="reloadPage();">Genstart spillet</button></center><br/>
<script src="projektfps.js"></script>
</body>
</html>

JS:

let context = document.getElementById("mycanvas").getContext('2d');
context.fillStyle = "antiqueWhite";
context.fillRect(0,0,420,375);

let col = 7; 
let row = 6; 
let x = 28; 
let y = 35; 
let r = 25; 
let activePlayer =1;

let column0 = [0,0,0,0,0,0];
let column1 = [0,0,0,0,0,0];
let column2 = [0,0,0,0,0,0];
let column3 = [0,0,0,0,0,0];
let column4 = [0,0,0,0,0,0];
let column5 = [0,0,0,0,0,0];
let column6 = [0,0,0,0,0,0];
let columns = [column0,column1,column2, column3, column4, column5, column6];
console.log(columns);

//Players:
let name1 = prompt("Indtast spiller 1's navn:");
document.getElementById("spiller1").innerHTML = "Spiller 1: " + name1;

let name2 = prompt("Indtast spiller 2's navn:");
document.getElementById("spiller2").innerHTML = "Spiller 2: " + name2;

drawBoard();

//-------Functions--------

function drawBoard() {
    for(let i = 0; i < col ; i++){
        for (let j = 0 ; j < row ; j++){
            drawCircle(x + i *60, y + j*60, r);
            context.strokeStyle = "black";
            context.stroke();
            context.fillStyle = "white";
            context.fill();
        }
    }
}

function putPieceInColumnNo(no){
    let activeColumn = columns[no];
    let height = 0;

    for (let i = 0 ; i < activeColumn.length ; i++){
        if (activeColumn[i] === 0){
            activeColumn[i] = activePlayer;
            drawPieceAt(no,5-i,activePlayer);
            height = i;
            i = activeColumn.length
        }
    }
    let winner = checkWinnerVertical(activePlayer,no);
    if (winner===true){
        publishWinner();
    }
    winner = checkWinnerHorizontal(activePlayer,height);
    if (winner===true){
        publishWinner();
    }
    swapPl();
}

function checkWinnerVertical(activePlayer, no){
    let activeCol = columns[no];
    let inARow = 0;
    for(let i=0;i<activeCol.length;i++){
        if(activeCol[i]===activePlayer){
            inARow++;
        } else {
            inARow=0;
        }
        if(inARow>=4){
            return true;
        }
    }
    return false;
}

function checkWinnerHorizontal(activePlayer, height) {
    let inACol = 0;
    for (let i = 0; i < columns.length ; i++){
        if (columns[i][height] === activePlayer){
            inACol++;
        } else {
            inACol = 0;
        }
        if (inACol >= 4){
            return true;
        }
    }
    return false;
}

function swapPl() {
    if (activePlayer===1){
        activePlayer=2;
    } else {
        activePlayer=1;
    }
}

function reloadPage(){
    window.location.reload();
}

function drawPieceAt(x, y, activePlayer) {
    let color = 'red';
    if (activePlayer === 1){
        color = 'blue'
    }
    drawCircle(28+x*60, 35+y*60, r, color);
}

function drawCircle(x, y, r, fill) {
    //Fill the circle if fill is set
    if (fill){
        //debugger;
        context.beginPath();
        context.arc(x, y, r, 0, Math.PI*2);
        context.fillStyle = fill;
        context.fill();
    } else {
        context.beginPath();
        context.arc(x, y, r, 0, Math.PI*2);
        context.stroke();
    }
}

function publishWinner(){
    if (activePlayer ===1){
        alert("Tillykke " + name1 +" du har vundet!!");
    }else{
        alert("Tillykke " + name2 +" du har vundet!!")
    }
}

Solution

Overall design

I noticed that if an alert is displayed with a message that a given user wins, then the column buttons can still be clicked, which could lead to an alert displaying a message that the other user won. Normally once a user is declared a winner, the game should be reset…

Code feedback

HTML

<center>
<br/>
<div style="font-family:Arial,sans-serif; font-size:14pt; font-weight:bold; color: black" id="p1">Velkommen til fire på stribe</div><br/>
<body bgcolor="#87cefa">

With this markup, the body tag is a child of the <center> tag (which is “also considered obsolete”1)… this appears to be invalid. The <center> tag (as well as the <div> before it) should be a child of the body tag.

<body bgcolor="#87cefa">
    <center>
    <br/>
    <div style="font-family:Arial,sans-serif; font-size:14pt; font-weight:bold; color: black" id="p1">Velkommen til fire på stribe</div><br/>

Put redundant styles in classes and use CSS

Many of the buttom elements have redundant style attributes (i.e. background-color: antiquewhite; border-radius: 16px) – much of that redundancy could be eliminated with CSS. For example a class name could be added, or even the selector of any button that is a child element of a new container, could have that style applied. The same is true for the elements which have an id attribute containing the word spiller.

JavaScript

Use const instead of let for anything that doesn’t get re-assigned

Variables like context, activeColumn and winner in putPieceInColumnNo(), activeCol in checkWinnerVertical(), etc. don’t get re-assigned and thus are good candidates for const instead of let. While properties of constants can be re-assigned, the object/value itself cannot be re-assigned.

Simplifying drawCircle()

The code in drawCircle() – i.e. 

function drawCircle(x, y, r, fill) {
//Fill the circle if fill is set
    if (fill){
        //debugger;
        context.beginPath();
        context.arc(x, y, r, 0, Math.PI*2);
        context.fillStyle = fill;
        context.fill();
    } else {
        context.beginPath();
        context.arc(x, y, r, 0, Math.PI*2);
        context.stroke();
    }
}

could be simplified in several ways, including the following, where redundant lines are removed (follows the D.R.Y. principle):

function drawCircle(x, y, r, fill) {
    //Fill the circle if fill is set
    context.beginPath();
    context.arc(x, y, r, 0, Math.PI*2);
    if (fill){
        context.fillStyle = fill;
        context.fill();
    } else {
        context.stroke();
    }
}

Move the logic out of onclick in HTML (calling putPieceInColumnNo()) and use a JavaScript delegate instead

Instead of mixing the JavaScript calls in with the HTML, add JavaScript code to call the function when appropriate. 

document.addEventListener('click', function(event) {
    if(event.target.id === 'restart') {
        reloadPage();
    }
    else if (event.target.id.substr(0, 3) === 'col') { //could also use data-attributes
        const parts = event.target.id.split('col');
        putPieceInColumnNo(parts[1] - 1);
    }
});

One could also use data attributes to specify the column passed to the function.

Reducing redundancy with column creation:

The following setup code:

let column0 = [0,0,0,0,0,0];
let column1 = [0,0,0,0,0,0];
let column2 = [0,0,0,0,0,0];
let column3 = [0,0,0,0,0,0];
let column4 = [0,0,0,0,0,0];
let column5 = [0,0,0,0,0,0];
let column6 = [0,0,0,0,0,0];
let columns = [column0,column1,column2, column3, column4, column5, column6];

Can be simplified using Array.fill() and Array.forEach()

const columns = new Array(7).fill('');
columns.forEach(function(column, index) {
    columns[index] = [0,0,0,0,0,0];
});

Initially I was thinking .fill() could be used with the array of zeroes, but then each nested array would have the same memory location.

Rewrite

See updated code with advice taken into consideration below in the snippet

Show code snippet

Leave a Reply

Your email address will not be published. Required fields are marked *