3
\$\begingroup\$

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!!")
    }
}
\$\endgroup\$
  • \$\begingroup\$ I've removed the stack snippet because it doesn't work. \$\endgroup\$ – Dannnno Apr 2 '18 at 15:26
  • \$\begingroup\$ As-is, your question is incomplete. Can you provide all relevant CSS/HTML as well? \$\endgroup\$ – Dannnno Apr 2 '18 at 15:27
  • \$\begingroup\$ Finally, because your code isn't working (it doesn't solve diagonal) this is off-topic. \$\endgroup\$ – Dannnno Apr 2 '18 at 15:27
  • 4
    \$\begingroup\$ @Dannnno I think that declaring a deficiency is OK as long as the question is not asking for help to implement diagonal checking. We can just treat it as a variant of the game with different rules. \$\endgroup\$ – 200_success Apr 2 '18 at 15:43
  • \$\begingroup\$ I'm also new in here so thank you both so far - I've allready learned something :) The html is there now. \$\endgroup\$ – T. Malm Apr 2 '18 at 16:10
2
\$\begingroup\$

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

const 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;

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

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

const 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();
    }
  }
}
document.addEventListener('click', function(event) {
  if (event.target.id === 'restart') {
    reloadPage();
  } else if (event.target.dataset.id) {
    putPieceInColumnNo(event.target.dataset.id - 1);
  }
});

function putPieceInColumnNo(no) {
  const 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) {
  const 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
  context.beginPath();
  context.arc(x, y, r, 0, Math.PI * 2);
  if (fill) {
    //debugger;
    context.fillStyle = fill;
    context.fill();
  } else {
    context.stroke();
  }
}

function publishWinner() {
  if (activePlayer === 1) {
    alert("Tillykke " + name1 + " du har vundet!!");
  } else {
    alert("Tillykke " + name2 + " du har vundet!!")
  }
}
button[id^='col'] {
  background-color: antiquewhite;
  border-radius: 16px;
}

.spiller {
  font-family: Arial, sans-serif;
  font-size: 14pt;
  font-weight: bold;
}
#spiller1 {
  color: blue;
}
#spiller2 {
  color: red;
}
<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/>
  <div class="spiller" id="spiller1"></div>
  <div class="spiller" id="spiller2"></div><br/>
  <button data-id="1"> - - 1 - -</button>
  <button data-id="2"> - - 2 - -</button>
  <button data-id="3"> - - 3 - -</button>
  <button data-id="4"> - - 4 - -</button>
  <button data-id="5"> - - 5 - -</button>
  <button data-id="6"> - - 6 - -</button>
  <button data-id="7"> - - 7 - -</button><br>
  <canvas id="mycanvas" width="420" height="420"></canvas>
</center>
<center><button style="border-radius:16px; background-color: antiquewhite" id="restart">Genstart spillet</button></center><br/>


\$\endgroup\$

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.