Skip to main content
Spelling and grammar
Source Link
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

while you could argue whether it is correct or incorrect to use a col and row attribute here I would state that it semantically isn't.
What you can use without any arguementargument here is the ofare data attributes here:

3.5 if//else-Statment Statement vs switch-Statement Statement

In the very worst case a switch will take as long as 2 if-statements statements. In most cases, it is even faster than that. In any case, a switch-statement statement will be faster than an if-statement statement combined with an else if statement. The more statements are combined the larger the performance difference will be.

The whole block can be factorized with a switch-statement tatement as such (which IMHO is also more readable):

Styling of a document should for various reasons, such as performance by caching and reusability, always be a task for CSS in modern programming!

  • manipulates the style by using inline-style with the highest specificity weightweight, which overwrites all other styles.
  • cannot be cached (performance loss)
  • Harder to maintain and debug, as the specificity weight is hard to track
  • Harder to read
  • extend the required JS lines
  • ...

With classList.toggle() you can apply a class to an element if it does n to the classdoesn't have it already. Inc aseIn case that the element already has that specific class, it will remove that class:

You also have the option to add a second parameter with a clean or if-statement statement to apply a class only if a certain condition is met:

3.7 Minor Logic AdvisesAdvice

Good job for using a ternary conditional operatorternary conditional operator here instead of an if//else statement. However there is a smarter way of solving it with the help of the modulusmodulus:

It works by simple calculationscalculation where you always add + 1 to the current player. Then you divide it by 2 and look for the remainder (modulus): player = (player + 1) % 2

To win you need to have 3 in a row which requires that a player has at least done 3 moves. So the earlierearliest turn where a player could win is at the end of round 5. Only then can the starting player can achieve a win.

Running functions costcosts computing power. In other words, time and performance. This might not matter much to you at this point but will help you to develop good habits and focus on performance. That is a very important skill to have if you develop more complex applications or games, especially for limited hardware such as mobile devices.

The fasterfastest way to restart a game, especially if you do not track wins and losses, is by simply reloading the document: location.reload();

while you could argue whether it is correct or incorrect to use a col and row attribute here I would state that it semantically isn't.
What you can use without any arguement here is the of data attributes here:

3.5 if/else-Statment vs switch-Statement

In the very worst case a switch will take as long as 2 if-statements. In most cases, it is even faster than that. In any case, a switch-statement will be faster than an if-statement combined with an else if statement. The more statements are combined the larger the performance difference will be.

The whole block can be factorized with a switch-statement as such (which IMHO is also more readable):

Styling of a document should for various reasons such as performance by caching and reusability always be a task for CSS in modern programming!

  • manipulates the style by using inline-style with the highest specificity weight, which overwrites all other styles.
  • cannot be cached (performance loss)
  • Harder to maintain and debug as the specificity weight is hard to track
  • Harder to read
  • extend the required JS lines
  • ...

With classList.toggle() you can apply a class to an element if it does n to the class already. Inc ase that the element already has that specific class, it will remove that class:

You also have the option to add a second parameter with a clean or if-statement to apply a class only if a certain condition is met:

3.7 Minor Logic Advises

Good job for using a ternary conditional operator here instead of an if/else statement. However there is a smarter way of solving it with the help of the modulus:

It works by simple calculations where you always add + 1 to the current player. Then you divide it by 2 and look for the remainder (modulus): player = (player + 1) % 2

To win you need to have 3 in a row which requires that a player has at least done 3 moves. So the earlier turn where a player could win is at the end of round 5. Only then the starting player can achieve a win.

Running functions cost computing power. In other words time and performance. This might not matter much to you at this point but will help you to develop good habits and focus on performance. That is a very important skill to have if you develop more complex applications or games, especially for limited hardware such as mobile devices.

The faster way to restart a game, especially if you do not track wins and losses, is by simply reloading the document: location.reload();

while you could argue whether it is correct or incorrect to use a col and row attribute here I would state that it semantically isn't.
What you can use without any argument here are data attributes:

3.5 if/else Statement vs switch Statement

In the very worst case a switch will take as long as 2 if statements. In most cases, it is even faster than that. In any case, a switch statement will be faster than an if statement combined with an else if statement. The more statements are combined the larger the performance difference will be.

The whole block can be factorized with a switch- tatement as such (which IMHO is also more readable):

Styling of a document should for various reasons, such as performance by caching and reusability, always be a task for CSS in modern programming!

  • manipulates the style by using inline-style with the highest weight, which overwrites all other styles.
  • cannot be cached (performance loss)
  • Harder to maintain and debug, as the weight is hard to track
  • Harder to read
  • extend the required JS lines
  • ...

With classList.toggle() you can apply a class to an element if it doesn't have it already. In case that the element already has that specific class, it will remove that class:

You also have the option to add a second parameter with a clean or if statement to apply a class only if a certain condition is met:

3.7 Minor Logic Advice

Good job for using a ternary conditional operator here instead of an if/else statement. However there is a smarter way of solving it with the help of the modulus:

It works by simple calculation where you always add 1 to the current player. Then you divide it by 2 and look for the remainder (modulus): player = (player + 1) % 2

To win you need to have 3 in a row which requires that a player has at least done 3 moves. So the earliest turn where a player could win is at the end of round 5. Only then can the starting player achieve a win.

Running functions costs computing power. In other words, time and performance. This might not matter much to you at this point but will help you to develop good habits and focus on performance. That is a very important skill to have if you develop more complex applications or games, especially for limited hardware such as mobile devices.

The fastest way to restart a game, especially if you do not track wins and losses, is by simply reloading the document: location.reload();

added 5 characters in body
Source Link
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

Your HTML Markup that you posted is invalid and would not pass the W3C Markup Validator. My educated guess is, that you only posterposted the parts of your body content, which for a Code Review is a no-go. Always paste the full code for review!

A label is a semantic element to label an interactive element such as input and output elements.
  
In your case, you're not labeling anything. You use that element to output information.

You use an onclick attribute to trigger the reset function. In modern programming, you should split HTML, CSS, and JS always from each other for various reasons. One of them is an economic reason: Maintenance. YOuYou should always write code (at least in industrial standard) to be easily maintained and for other programmers to take over. As such you should use the addEventListener in JS button instead as otherwise, a function renaming would require to be fixed both in JS and HTML instead of just one place (JS).

You are creating a grid layout here. We could argue that it is a table in which case you also should use <table>. There could be also an argumentationargument that it is a game board that has a table-like layout and does not contain strictly tabular data. In such case, you should use a CSS Grid:

You working here with an element list. Specifically you're working with a *HTML Collection Object. In the next step you convert the object to an array.
  
Why are you doing it? Because you want to use forEach as an iteration method instead of a for loop. forEach does not work with that kind of object:

The innerText method (good that you didn't use innerHTML here) has one major downside:
  
It requires the document to prepare which is time-consuming. Even though it is just msmilliseconds, it is still inefficient and it will scale up.

While this if-statementif statement is not wrong and obviously works, it is unclean. true and false are boolean values while an if-statementif statement also looks for a boolean statementvalue. As such there is no need for a boolean comparison as you can simply just use the variable as a comparison. However, since you looking for a false boolean and not a true boolean you have to use ! in front: !gameWon:

let gameWon = false;

// expecting statmentstatement to be true 
if (!gameWon) {
  console.log('Statement "!gameWon" is true')
} else {
  console.log('Statement "!gameWon" is false')
}

// expecting statmentstatement to be false 
if (gameWon) {
  console.log('Statement "gameWon" is true')
} else {
  console.log('Statement "gameWon" is false')
}

while you could argue ifwhether it is correct or incorrect to use a col and row attribute here I would state that it semantically isn't.
  
What you can use without any arguearguement here is the usage of data attributes here:

Your HTML Markup that you posted is invalid and would not pass the W3C Markup Validator. My educated guess is, that you only poster the parts of your body content which for a Code Review is a no-go. Always paste the full code for review!

A label is a semantic element to label an interactive element such as input and output elements.
  In your case, you're not labeling anything. You use that element to output information.

You use an onclick attribute to trigger the reset function. In modern programming, you should split HTML, CSS, and JS always from each other for various reasons. One of them is an economic reason: Maintenance. YOu should always write code (at least in industrial standard) to be easily maintained and for other programmers to take over. As such you should use the addEventListener in JS button instead as otherwise, a function renaming would require to be fixed both in JS and HTML instead of just one place (JS).

You are creating a grid layout here. We could argue that it is a table in which case you also should use <table>. There could be also an argumentation that it is a game board that has a table-like layout and does not contain strictly tabular data. In such case, you should use a CSS Grid:

You working here with an element list. Specifically you're working with a *HTML Collection Object. In the next step you convert the object to an array.
  Why are you doing it? Because you want to use forEach as an iteration method instead of a for loop. forEach does not work with that kind of object:

The innerText method (good that you didn't use innerHTML here) has one major downside:
  It requires the document to prepare which is time-consuming. Even though it is just ms it is still inefficient and it will scale up.

While this if-statement is not wrong and obviously works, it is unclean. true and false are boolean values while an if-statement also looks for a boolean statement. As such there is no need for a boolean comparison as you can simply just use the variable as a comparison. However, since you looking for a false boolean and not a true boolean you have to use ! in front: !gameWon:

let gameWon = false;

// expecting statment to be true 
if (!gameWon) {
  console.log('Statement "!gameWon" is true')
} else {
  console.log('Statement "!gameWon" is false')
}

// expecting statment to be false 
if (gameWon) {
  console.log('Statement "gameWon" is true')
} else {
  console.log('Statement "gameWon" is false')
}

while you could argue if it is correct or incorrect to use a col and row attribute here I would state that it semantically isn't.
  What you can use without any argue is the usage of data attributes here:

Your HTML Markup that you posted is invalid and would not pass the W3C Markup Validator. My educated guess is that you only posted the parts of your body content, which for a Code Review is a no-go. Always paste the full code for review!

A label is a semantic element to label an interactive element such as input and output elements. 
In your case, you're not labeling anything. You use that element to output information.

You use an onclick attribute to trigger the reset function. In modern programming, you should split HTML, CSS, and JS always from each other for various reasons. One of them is an economic reason: Maintenance. You should always write code (at least in industrial standard) to be easily maintained and for other programmers to take over. As such you should use the addEventListener in JS button instead as otherwise, a function renaming would require to be fixed both in JS and HTML instead of just one place (JS).

You are creating a grid layout here. We could argue that it is a table in which case you also should use <table>. There could be also an argument that it is a game board that has a table-like layout and does not contain strictly tabular data. In such case, you should use a CSS Grid:

You working here with an element list. Specifically you're working with a *HTML Collection Object. In the next step you convert the object to an array. 
Why are you doing it? Because you want to use forEach as an iteration method instead of a for loop. forEach does not work with that kind of object:

The innerText method (good that you didn't use innerHTML here) has one major downside: 
It requires the document to prepare which is time-consuming. Even though it is just milliseconds, it is still inefficient and it will scale up.

While this if statement is not wrong and obviously works, it is unclean. true and false are boolean values while an if statement also looks for a boolean value. As such there is no need for a boolean comparison as you can simply just use the variable as a comparison. However, since you looking for a false boolean and not a true boolean you have to use ! in front: !gameWon:

let gameWon = false;

// expecting statement to be true 
if (!gameWon) {
  console.log('Statement "!gameWon" is true')
} else {
  console.log('Statement "!gameWon" is false')
}

// expecting statement to be false 
if (gameWon) {
  console.log('Statement "gameWon" is true')
} else {
  console.log('Statement "gameWon" is false')
}

while you could argue whether it is correct or incorrect to use a col and row attribute here I would state that it semantically isn't. 
What you can use without any arguement here is the of data attributes here:

added 1648 characters in body
Source Link
tacoshy
  • 512
  • 3
  • 13

A full review of the JS will follow shortly within the next day

const CELL = document.querySelector('.cell');
let row = CELL.dataset.row;
let col = CELL.dataset.col;

console.log(`Row: ${row}, Col: ${col}`);
<div class="cell" data-row="0" data-col="1"></div>

3.5 if/else-Statment vs switch-Statement

if (checkWinner()) {
  winner.innerText = `Player ${player} wins!`;
  turn.innerText = "";
  gameWon = true;
  return;
} else if (boardIsFull()) {
  winner.innerText = "Game is a tie!";
  turn.innerText = "";
  cellsArray.forEach(
    cell => cell.style.backgroundColor = playerTieBackgroundColor
  );
  return;
}
player = player == players[0] ? players[1] : players[0];
turn.innerText = `Player ${player}'s turn`;

In the very worst case a switch will take as long as 2 if-statements. In most cases, it is even faster than that. In any case, a switch-statement will be faster than an if-statement combined with an else if statement. The more statements are combined the larger the performance difference will be.

The whole block can be factorized with a switch-statement as such (which IMHO is also more readable):

switch (true) {
  case (checkWinner()):
    winner.innerText = `Player ${player} wins!`;
    turn.textContent = '';
    gameWon = true;
    break;
  case (boardIsFull()):
    winner.innerText = "Game is a tie!";
    turn.textContent = '';
    cellsArray.forEach(
      cell => cell.style.backgroundColor = playerTieBackgroundColor
    );
    break;
  default:
    player = player == players[0] ? players[1] : players[0];
    turn.innerText = `Player ${player}'s turn`;
}

3.6 Styling through JS

Styling of a document should for various reasons such as performance by caching and reusability always be a task for CSS in modern programming!

Disadvantages of the style JS method:

  • manipulates the style by using inline-style with the highest specificity weight, which overwrites all other styles.
  • cannot be cached (performance loss)
  • Harder to maintain and debug as the specificity weight is hard to track
  • Harder to read
  • extend the required JS lines
  • ...

The modern solution to apply styles is by using the classList method which adds, removes, or toggles a CSS class from the element.

3.6.1 Add

With classList.add() you adding a class to an element:

document.querySelector('button').addEventListener('click', function() {
  document.body.classList.add('bg-color');
})
.bg-color {
  background-color: red;
}
<button>Add the class "bg-color" to the body</button>

3.6.2 Remove

With classList.remove() you remove a class from an element:

document.querySelector('button').addEventListener('click', function() {
  document.body.classList.remove('bg-color');
})
.bg-color {
  background-color: red;
}
<body class="bg-color">
  <button>Remove the class "bg-color" from the body</button>
</body>

3.6.3 Toggle

With classList.toggle() you can apply a class to an element if it does n to the class already. Inc ase that the element already has that specific class, it will remove that class:

document.querySelector('button').addEventListener('click', function() {
  document.body.classList.toggle('bg-color');
})
.bg-color {
  background-color: red;
}
<button>toggle to apply/remove the class "bg-color" to/from the body</button>

You also have the option to add a second parameter with a clean or if-statement to apply a class only if a certain condition is met:

let gameWon = true;

// expected to be true and apply 
document.body.classList.toggle('bg-color', gameWon);

// expected to be false and not apply 
document.body.classList.toggle('border-color', !gameWon);
.bg-color {
  background-color: red;
}

.border-color {
  border: 5px dashed blue;
}

3.7 Minor Logic Advises

There are a few parts of the code that could be solved in another way and which I would rate my students for.

3.7.1 Swapping Player by Array

const players = ["X", "O"];
player = player == players[0] ? players[1] : players[0];

Good job for using a ternary conditional operator here instead of an if/else statement. However there is a smarter way of solving it with the help of the modulus:

const PLAYERS = ["X", "O"];
let player = 0;

document.querySelector('button').addEventListener('click', function() {
  player = (player + 1) % 2;
  console.clear();
  console.log(`Player ${PLAYERS[player]}'s turn`);
})
  
<button>Swap Player</button>

It works by simple calculations where you always add + 1 to the current player. Then you divide it by 2 and look for the remainder (modulus): player = (player + 1) % 2

Start: player = 0

Round 1: (0 + 1) % 2 = 1 (0, remains 1)

Round 2: (1 + 1) % 2 = 0 (1, remains 0)

3.7.2 Winning and Draw Logic

You should track the turns or rounds!

To win you need to have 3 in a row which requires that a player has at least done 3 moves. So the earlier turn where a player could win is at the end of round 5. Only then the starting player can achieve a win.

A draw can only appear at the end of round 9.

If you track the round then you do not have to check if all cells are filled because at the end of turn 9, they should all be filled!

Also, you can then limit the execution of the winning logic function as well as the draw function. You only run the winning check function at turn 5 or later and the draw function only at turn 9.

Why is that important?

Running functions cost computing power. In other words time and performance. This might not matter much to you at this point but will help you to develop good habits and focus on performance. That is a very important skill to have if you develop more complex applications or games, especially for limited hardware such as mobile devices.

3.7.3 Restart

The faster way to restart a game, especially if you do not track wins and losses, is by simply reloading the document: location.reload();

A full review of the JS will follow shortly within the next day

const CELL = document.querySelector('.cell');
let row = CELL.dataset.row;
let col = CELL.dataset.col;

console.log(`Row: ${row}, Col: ${col}`);
<div class="cell" data-row="0" data-col="1"></div>
const CELL = document.querySelector('.cell');
let row = CELL.dataset.row;
let col = CELL.dataset.col;

console.log(`Row: ${row}, Col: ${col}`);
<div class="cell" data-row="0" data-col="1"></div>

3.5 if/else-Statment vs switch-Statement

if (checkWinner()) {
  winner.innerText = `Player ${player} wins!`;
  turn.innerText = "";
  gameWon = true;
  return;
} else if (boardIsFull()) {
  winner.innerText = "Game is a tie!";
  turn.innerText = "";
  cellsArray.forEach(
    cell => cell.style.backgroundColor = playerTieBackgroundColor
  );
  return;
}
player = player == players[0] ? players[1] : players[0];
turn.innerText = `Player ${player}'s turn`;

In the very worst case a switch will take as long as 2 if-statements. In most cases, it is even faster than that. In any case, a switch-statement will be faster than an if-statement combined with an else if statement. The more statements are combined the larger the performance difference will be.

The whole block can be factorized with a switch-statement as such (which IMHO is also more readable):

switch (true) {
  case (checkWinner()):
    winner.innerText = `Player ${player} wins!`;
    turn.textContent = '';
    gameWon = true;
    break;
  case (boardIsFull()):
    winner.innerText = "Game is a tie!";
    turn.textContent = '';
    cellsArray.forEach(
      cell => cell.style.backgroundColor = playerTieBackgroundColor
    );
    break;
  default:
    player = player == players[0] ? players[1] : players[0];
    turn.innerText = `Player ${player}'s turn`;
}

3.6 Styling through JS

Styling of a document should for various reasons such as performance by caching and reusability always be a task for CSS in modern programming!

Disadvantages of the style JS method:

  • manipulates the style by using inline-style with the highest specificity weight, which overwrites all other styles.
  • cannot be cached (performance loss)
  • Harder to maintain and debug as the specificity weight is hard to track
  • Harder to read
  • extend the required JS lines
  • ...

The modern solution to apply styles is by using the classList method which adds, removes, or toggles a CSS class from the element.

3.6.1 Add

With classList.add() you adding a class to an element:

document.querySelector('button').addEventListener('click', function() {
  document.body.classList.add('bg-color');
})
.bg-color {
  background-color: red;
}
<button>Add the class "bg-color" to the body</button>

3.6.2 Remove

With classList.remove() you remove a class from an element:

document.querySelector('button').addEventListener('click', function() {
  document.body.classList.remove('bg-color');
})
.bg-color {
  background-color: red;
}
<body class="bg-color">
  <button>Remove the class "bg-color" from the body</button>
</body>

3.6.3 Toggle

With classList.toggle() you can apply a class to an element if it does n to the class already. Inc ase that the element already has that specific class, it will remove that class:

document.querySelector('button').addEventListener('click', function() {
  document.body.classList.toggle('bg-color');
})
.bg-color {
  background-color: red;
}
<button>toggle to apply/remove the class "bg-color" to/from the body</button>

You also have the option to add a second parameter with a clean or if-statement to apply a class only if a certain condition is met:

let gameWon = true;

// expected to be true and apply 
document.body.classList.toggle('bg-color', gameWon);

// expected to be false and not apply 
document.body.classList.toggle('border-color', !gameWon);
.bg-color {
  background-color: red;
}

.border-color {
  border: 5px dashed blue;
}

3.7 Minor Logic Advises

There are a few parts of the code that could be solved in another way and which I would rate my students for.

3.7.1 Swapping Player by Array

const players = ["X", "O"];
player = player == players[0] ? players[1] : players[0];

Good job for using a ternary conditional operator here instead of an if/else statement. However there is a smarter way of solving it with the help of the modulus:

const PLAYERS = ["X", "O"];
let player = 0;

document.querySelector('button').addEventListener('click', function() {
  player = (player + 1) % 2;
  console.clear();
  console.log(`Player ${PLAYERS[player]}'s turn`);
})
  
<button>Swap Player</button>

It works by simple calculations where you always add + 1 to the current player. Then you divide it by 2 and look for the remainder (modulus): player = (player + 1) % 2

Start: player = 0

Round 1: (0 + 1) % 2 = 1 (0, remains 1)

Round 2: (1 + 1) % 2 = 0 (1, remains 0)

3.7.2 Winning and Draw Logic

You should track the turns or rounds!

To win you need to have 3 in a row which requires that a player has at least done 3 moves. So the earlier turn where a player could win is at the end of round 5. Only then the starting player can achieve a win.

A draw can only appear at the end of round 9.

If you track the round then you do not have to check if all cells are filled because at the end of turn 9, they should all be filled!

Also, you can then limit the execution of the winning logic function as well as the draw function. You only run the winning check function at turn 5 or later and the draw function only at turn 9.

Why is that important?

Running functions cost computing power. In other words time and performance. This might not matter much to you at this point but will help you to develop good habits and focus on performance. That is a very important skill to have if you develop more complex applications or games, especially for limited hardware such as mobile devices.

3.7.3 Restart

The faster way to restart a game, especially if you do not track wins and losses, is by simply reloading the document: location.reload();

added 1648 characters in body
Source Link
tacoshy
  • 512
  • 3
  • 13
Loading
added 1648 characters in body
Source Link
tacoshy
  • 512
  • 3
  • 13
Loading
added 1648 characters in body
Source Link
tacoshy
  • 512
  • 3
  • 13
Loading
Source Link
tacoshy
  • 512
  • 3
  • 13
Loading