4
\$\begingroup\$

Me and my friend are trying to build a Connect4 game with a twist. The twist requires us to have a 7*7 board and to be always able to check all possibilities for a win for both players after each move.

Currently, we are just filling the board with a random distribution of 0,1 and 2 (in the final game it will be 0 for empty, 1 for player 1, 2 for player 2) and then check if there are four of the same kind (even 0) connected, which means someone wins.

What I would like to be interested in:

  • Can this part of the checkForWin function be any cleaner?

    if (winRow || winColumn || winDiagonal) {
        if (winRow && !winColumn && !winDiagonal) {
            document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY ROW";
        }
        if (!winRow && winColumn && !winDiagonal) {
            document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY COLUMN";
        }
        if (!winRow && !winColumn && winDiagonal) {
            document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY DIAGONAL";
        }
        if (winRow && winColumn && !winDiagonal) {
            document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY ROW AND COLUMN";
        }
        if (winRow && !winColumn && winDiagonal) {
            document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY ROW AND DIAGONAL";
        }
        if (!winRow && winColumn && winDiagonal) {
            document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY COLUMN AND DIAGONAL";
        }
        if (winRow && winColumn && winDiagonal) {
            document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY ROW, COLUMN AND DIAGONAL";
        }
    }
    
  • Some testing to check if the checkForWin function really always does what we want it to. I tried a lot of random distributions, but I'm just not sure if everything works out the right way.

  • I really had to mess around with the CSS until it looked like it does now. Please help me to clean this up.
  • General comments, style advice and hints for HTML, CSS and JS.

// jshint esversion:6
// jshint browser: true
// jshint devel: true
var matrix = Array(7)
  .fill(null)
  .map(() => Array(7)
    .fill(0));
var fieldtoken = [0, 1, 2]; // possible states a field can have. 0 for empty, 1 for player 1, 2 for player 2
var coordinates = false; // switch to true to see the coordinates in the fields
// we are currently just filling the board with a random distribution of the field tokens
function randomEntry() {
  return fieldtoken[Math.floor(Math.random() * 3)];
}

function fillMatrix() {
  for (var i = 6; i > -1; i--) {
    for (var j = 6; j > -1; j--) {
      if (coordinates) {
        matrix[i][j] = "column" + i + "<br>row" + j;
      } else {
        matrix[i][j] = randomEntry();
      }
    }
  }
}
fillMatrix();
//convert javascript matrix into html matrix
//note that we create columns from left to right and rows from bottom to top
function drawMatrix() {
  var gameArea = document.getElementsByClassName("gameArea")[0];
  for (var i = 0; i < 7; i++) {
    //create columns
    var column = document.createElement("div");
    column.className = "column";
    gameArea.appendChild(column);
    for (var j = 6; j > -1; j--) {
      //create fields			
      var field = document.createElement("div");
      field.className = "field";
      var textfield = document.createElement("div");
      textfield.className = "textfield";
      textfield.innerHTML = matrix[i][j];
      column.appendChild(field);
      field.appendChild(textfield);
    }
  }
}
drawMatrix();
var winRow = false;
var winColumn = false;
var winDiagonal = false;

function checkForWin() {
  checkRows();
  checkColumns();
  checkDiagonals();
  if (winRow || winColumn || winDiagonal) {
    if (winRow && !winColumn && !winDiagonal) {
      document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY ROW";
    }
    if (!winRow && winColumn && !winDiagonal) {
      document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY COLUMN";
    }
    if (!winRow && !winColumn && winDiagonal) {
      document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY DIAGONAL";
    }
    if (winRow && winColumn && !winDiagonal) {
      document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY ROW AND COLUMN";
    }
    if (winRow && !winColumn && winDiagonal) {
      document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY ROW AND DIAGONAL";
    }
    if (!winRow && winColumn && winDiagonal) {
      document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY COLUMN AND DIAGONAL";
    }
    if (winRow && winColumn && winDiagonal) {
      document.getElementsByClassName("statusArea")[0].innerHTML = "WIN BY ROW, COLUMN AND DIAGONAL";
    }
  }
}

function checkRows() {
  //four in a row
  for (var j = 0; j < 7; j++) {
    var currentCount = 1;
    var currentToken = matrix[0][j];
    for (var i = 1; i < 7; i++) {
      if (currentToken === matrix[i][j]) {
        currentCount++;
        if (currentCount === 4) {
          winRow = true;
          break;
        }
      } else {
        currentToken = matrix[i][j];
        currentCount = 1;
      }
    }
  }
}

function checkColumns() {
  //four in a column
  for (var i = 0; i < 7; i++) {
    var currentCount = 1;
    var currentToken = matrix[i][0];
    for (var j = 1; j < 7; j++) {
      if (currentToken === matrix[i][j]) {
        currentCount++;
        if (currentCount === 4) {
          winColumn = true;
          break;
        }
      } else {
        currentToken = matrix[i][j];
        currentCount = 1;
      }
    }
  }
}

function checkDiagonals() {
  //four in a diagonal
  //bottom left to top right diagonals
  //idea: search from start points for these diagonals. start points are the fields in the bottom left 4*4 square
  checkBLTRdiagonals();
  //top left to bottom right diagonals
  //idea: search from start points for these diagonals. start points are the fields in the top left 4*4 square
  checkTLBRdiagonals();
}

function checkBLTRdiagonals() {
  for (var j = 0; j < 4; j++) {
    for (var i = 0; i < 4; i++) {
      var currentCount = 1;
      var currentToken = matrix[i][j];
      for (var step = 1; step < 4; step++) {
        if (currentToken === matrix[i + step][j + step]) {
          currentCount++;
        } else {
          break;
        }
        if (currentCount === 4) {
          winDiagonal = true;
        }
      }
    }
  }
}

function checkTLBRdiagonals() {
  for (var j = 4; j < 7; j++) {
    for (var i = 0; i < 4; i++) {
      var currentCount = 1;
      var currentToken = matrix[i][j];
      for (var step = 1; step < 4; step++) {
        if (currentToken === matrix[i + step][j - step]) {
          currentCount++;
        } else {
          break;
        }
        if (currentCount === 4) {
          winDiagonal = true;
        }
      }
    }
  }
}
checkForWin();
/*csslint  order-alphabetical: false*/

.container {
  position: absolute;
  top: 50%;
  left: 50%;
  margin-right: -50%;
  transform: translate(-50%, -50%)
}

.gameArea {
  display: flex;
  justify-content: center;
  align-items: center;
}

.column {
  float: left;
}

.field {
  width: 50px;
  height: 50px;
  background: darkred;
  padding: 5px;
  margin: 1px;
}

.textfield {
  color: lightyellow;
  text-align: center;
  display: flex;
  justify-content: center;
  align-items: center;
  width: inherit;
  height: inherit;
}

.statusArea {
  text-align: center;
  width: 350px;
  margin: auto;
  color: darkred;
  font-weight: bold;
  font-family: monospace;
}

html {
  background: linear-gradient(135deg, #0fb8ad 0%, #1fc8db 50%, #2cb5e8 100%) no-repeat center center fixed;
  background-size: cover;
}

body {
  background-size: cover;
}
<body>
  <div class="container">
    <div class="gameArea">

    </div>
    <div class="statusArea">Nothing happened.
    </div>

  </div>

\$\endgroup\$
  • \$\begingroup\$ Thank you for your edit, @Jamal. Maybe you also want to answer my questions? :) \$\endgroup\$ – Tweakimp Apr 15 '17 at 3:42
  • \$\begingroup\$ IMO this would be easier to review if the fiddle was embedded in the post itself (Ctrl+M), as an in-place executable Stack Snippet. The more on-site context, the better! Note that only the code that's embedded in your Code Review post is subject to review - off-site code is for additional context only. \$\endgroup\$ – Mathieu Guindon Apr 15 '17 at 5:49
  • \$\begingroup\$ I have rolled back your last code edits. Please see What should I do when someone answers my question? \$\endgroup\$ – Phrancis Apr 15 '17 at 20:01
4
\$\begingroup\$

I recommend separating your logic from UI by introducing a function getWinMessage and assigning the innerHTML or even better textContent later on:

function getWinMessage(winRow, winColumn, winDiagonal) {
  if ( winRow && !winColumn && !winDiagonal) return "WIN BY ROW";
  if (!winRow &&  winColumn && !winDiagonal) return "WIN BY COLUMN";
  if (!winRow && !winColumn &&  winDiagonal) return "WIN BY DIAGONAL";
  if ( winRow &&  winColumn && !winDiagonal) return "WIN BY ROW AND COLUMN";
  if ( winRow && !winColumn &&  winDiagonal) return "WIN BY ROW AND DIAGONAL";
  if (!winRow &&  winColumn &&  winDiagonal) return "WIN BY COLUMN AND DIAGONAL";
  if ( winRow &&  winColumn &&  winDiagonal) return "WIN BY ROW, COLUMN AND DIAGONAL";
}

if (winRow || winColumn || winDiagonal) {
  document.getElementsByClassName("statusArea")[0].textContent = getWinMessage(winRow, winColumn, winDiagonal);
}

For brevity, you could encode your messages in a three dimensional array with the dimensions representing row, column and diagonal (no-win = 0, win = 1). This is shorter but slightly less readable:

const winMessages = [[["", "WIN BY DIAGONAL"], ["WIN BY COLUMN", "WIN BY COLUMN AND DIAGONAL"]], [["WIN BY ROW", "WIN BY ROW AND DIAGONAL"], ["WIN BY ROW AND COLUMN", "WIN BY ROW, COLUMN AND DIAGONAL"]]];

function getWinMessage(winRow, winColumn, winDiagonal) {
  return winMessages[+winRow][+winColumn][+winDiagonal];
}

Regarding CSS and HTML: Please add those to your question, e.g. as a stack snippet.

\$\endgroup\$
4
\$\begingroup\$

My javascript is a little rusty, so doublecheck any code suggestions given, but I'm going to focus on more of the general coding style and overall structure, so hopefully it'll be useful to you even with some minor coding error.

Some coding style comments

  • Don't intermix function definitions and function calling – Your code intermixes these two quite a lot:

    function applyGravity() {
            ...
    }
    if (gravity) {
       applyGravity();
    }
    function drawMatrix() {
       ...
    }
    drawMatrix();
    

    It is much better to define all the function at the top of your script, and then call the functions at the bottom. Intermixing like this is going to give you problems at some point in time.

  • Add more space – Quite a few of your code blocks are very condensed, and this hinders readability. Add vertical space in front of logical blocks, like a for block, or if block. Or after the var block in start of a function.

    Your horizontal spacing is good though, I like the spacing around operators, and after parameters, and so on. 

  • CAPS for constants – This is based on personal preferences, but I do prefer that constants are written like ENTRIES or GRAVITY. And some would even like to enhance the globals you're using like GLOB_matrix or something similar. The point I'm trying to make is that giving yourself visual cues to what is constants and globals, helps understanding the program a lot, and can avoid some pitfalls related to changing stuff you shouldn't change.

Code refactoring

  • Let functions return something – Your checkXxxx() sets global variables instead of returning whether it has won or not. This is bad practice! In most cases, you shouldn't have to rely on global variable at all.

    A much better variant would be to let it return this bool, so that your checkForWin() would look like:

    function checkForWin() {
        winRow = checkRows();
        winCol = checkColumns();
        winDiagonal = checkDiagonals(); // or checkBLTRdiagonals() || checkTLBRdiagonals();
        ...
    }
    
  • Avoid repeated if statements – One of your questions is related to the series of if statements for definint the win sentence. Here is some food for thought: Make an array of the various win conditions, and join them all by , before replacing the last , with and.

    See this thread, or do something like this jsfiddle:

    winTxt += (winRow ? "row" : "");
    winTxt += (winTxt && winColumn ? ", " : "") + (winColumn ? "column" : "");
    winTxt += (winTxt && winDiagonal ? ", " : "") + (winDiagonal ? "diagonal" : "");
    winTxt = winTxt.replace(/,(?=[^,]*$)/, ' and');
    
  • Avoid traversing the entire board checking for wins – In general gameplay, you would know which piece was placed last, and could check if that triggered a winning condition. This would reduce the time spent checking for wins.

    When using a randomized board, you could still benefit from not looping the entire board, and rather check only parts of the board. For example when checking the first row, you don't need to start checking for a win when your start column is 5 as there is only 3 columns left. That can't be a winning condition.

    In other words, when placing pieces in normal gameplay, only check corresponding rows, columns or diagonals. And if checking the entire board, limit your for loops from 1 to dimension to 1 to (dimension - 4).

  • Avoid unnecessary double loops – For each row, column and diagonal you are currently using a double loop which are uncalled for. A simple loop where you keep track of previous piece, and count how many of those you have, potentially resetting count when changing pieces would eliminate one loop.

    A similar approach can be used when checking diagonals, where you only use one loop, but for each iteration checks both the "BLTR" and "TLBR" diagonals. It'll require a few more variables, but eliminates a loop.

    Actually I once wrote a win check, based on a placed piece, which only used one single loop to check for any row, column or diagonal win.

  • Who won? – In your current code you state that somebody has won, but who did actually win? And in addition your current code could also have a win by either party in rows, and a win by the other party in columns or diagonals. Not good...

  • Consider pre-generating the game board – You do loads of document.getElementsByClassName calls, which are somewhat expensive. If you pre-generated the game board, and gave each cell an id based upon the coordinates and possible a prefix, i.e. cell_2_3, you could change the content based on the less expensive id-calls.

    In addition, it could possibly be better to maintain the game table as an javascript array, and not as shown html code. And then just change the html whenever a piece changes.

\$\endgroup\$
  • \$\begingroup\$ Hi holroy, thank you for your answer. Ill try to change my code to your suggested style, all points seem reasonable. I cant avoid checking the entire board: I needed a function to check everything because of the little twist the game has. It can change the whole board in one move. Avoiding the double loops is possible, but harder to do. Im happy almost everything is working currently. But Ill consider your advice. For who won: we are just building the first parts of the code, we needed that function like it is to test other stuff. Fixing what you meint is on our to do list. Thanks! :) \$\endgroup\$ – Tweakimp Apr 15 '17 at 20:55
2
\$\begingroup\$

Building on top of @le_m's answer:

function getWinMessage(row, column, diagonal) {
  const winType = new Array(3);

  if (row)      winType.push('ROW');
  if (column)   winType.push('COLUMN');
  if (diagonal) winType.push('DIAGONAL');

  return 'WIN BY ' + winType.join(', ').replace(/,(?= [A-Z]+$)/, ' AND');
}

if (winRow || winColumn || winDiagonal) {
  document.getElementsByClassName('statusArea')[0].textContent = getWinMessage(winRow, winColumn, winDiagonal);
}

Word of explanation: this code adds appropriate word to an array and then joins that array into string by . Then regex replaces the last comma with  AND and we have our ready string.

More specifically, regex matches comma followed by space, 1 or more uppercase letter(s) and an end of the string.

\$\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.