2
\$\begingroup\$

I'm a beginner in OOP in JavaScript. I have programmed a Tic-Tac-Toe game. The program works, but I don't know if I followed the principles of OOP in this case. I know, that's functions in methods - that's wrong. How can I get rid of functions in methods? Thank you.

var ceil = document.getElementsByClassName("game-item"),
    reset = document.getElementById("reset-game"),
    message = document.getElementById("message"),
    stepCount = 0,
    winCombinations = [
        [1, 2, 3],
        [1, 4, 7],
        [1, 5, 9],
        [2, 5, 8],
        [3, 6, 9],
        [3, 5, 7],
        [4, 5, 6],
        [7, 8, 9]
    ],
    dataX = [],
    dataO = [];

class Player {

    constructor(name) {
        this.name = name;
    }
}


class Game {

    constructor() {
        this.playerX = new Player("X");
        this.playerO = new Player("O");
        this.currentPlayer = this.playerX.name;

        console.log(this.currentPlayer);
    }

    addX() {
        for (var i = 0; i < ceil.length; i++) {
            ceil[i].addEventListener("click", currentStep);
        }

        var z = this.currentPlayer;

        function currentStep() {


            var num = +this.getAttribute("data-ceil");
            if (!this.textContent) {
                this.innerText = z;

            z === "X"
                  ? dataX.push(num) && this.classList.add("x")
                  : dataO.push(num) && this.classList.add("o");
                if (
                  (dataO.length > 2 || dataX.length > 2) &&
                  (checkWin(dataO, num) || checkWin(dataX, num))
                ) {
                  for (var i = 0; i < ceil.length; i++) {
                    ceil[i].removeEventListener("click", currentStep);
                  }
                  return (message.innerText = "Win player " + z);
                }

                changePlayer();
                stepCount++;
                (stepCount == 9) ? (message.innerText = 'Tie') : (message.innerText = 'The player ' + z);
            }
        }

        function changePlayer() {
            z === 'X' ? (z = "O") : (z = "X");
        }


        function checkWin(arr, number) {

        for (var w = 0, wLen = winCombinations.length; w < wLen; w++) {
                var someWinArr = winCombinations[w],
                  count = 0;
                if (someWinArr.indexOf(number) !== -1) {
                  for (var k = 0, kLen = someWinArr.length; k < kLen; k++) {
                    if (arr.indexOf(someWinArr[k]) !== -1) {
                      count++;
                      if (count === 3) {
                        return true;
                      }
                    }
                  }
                  count = 0;
                }
            }
        }
    }

    reset() {
        reset.addEventListener("click", function() {
          for (var i = 0; i < ceil.length; i++) {
            ceil[i].innerText = "";
          }
          dataO = [];
          dataX = [];
          this.currentPlayer = "X";
          stepCount = 0;
          message.innerText = "The player " + this.currentPlayer;
          for (var i = 0; i < ceil.length; i++) {
            ceil[i].classList.remove("x", "o");
            // ceil[i].addEventListener("click", currentStep);
          }
        });
    }


}

const game = new Game();
game.addX();
game.reset();
* {
  box-sizing: border-box;
  outline: none;
}
.game-title {
  display: block;
  margin-bottom: 30px;
  font-size: 35px;
  font-weight: bold;
  text-align: center;
}
.game {
  display: flex;
  justify-content: space-between;
  flex-wrap: wrap;
  width: 152px;
  margin: 0 auto 50px;
  border: 1px solid #000;
}
.game-item {
  position: relative;
  width: 50px;
  height: 50px;
  line-height: 48px;
  border: 1px solid #000;
  cursor: pointer;
  text-align: center;
  font-size: 30px;
  font-weight: bold;
  transition: all linear 0.3s;
}
.game-item:hover {
  background-color: #ccc;
}
.game-item.o {
  background-color: #abfdab;
}
.game-item.x {
  background-color: #f99;
}
#reset-game {
  display: block;
  margin: 0 auto;
  border: 1px solid #000;
  border-radius: 20px;
  background-color: #fff;
  padding: 10px 20px;
  cursor: pointer;
  transition: all linear 0.3s;
}
#reset-game:hover {
  background-color: #ccc;
}
<span class="game-title" id="message">The player X</span>
<div class="game">
  <div class="game-item" data-ceil="1"></div>
  <div class="game-item" data-ceil="2"></div>
  <div class="game-item" data-ceil="3"></div>
  <div class="game-item" data-ceil="4"></div>
  <div class="game-item" data-ceil="5"></div>
  <div class="game-item" data-ceil="6"></div>
  <div class="game-item" data-ceil="7"></div>
  <div class="game-item" data-ceil="8"></div>
  <div class="game-item" data-ceil="9"></div>
</div>
<button id="reset-game">Clear</button> 

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

OOP is about Encapsulation.

Function placement

I know, that's functions in methods - that's wrong.

OMDG who would tell you such. Javascript works best with functions in functions. A function inside function is protected. It can not be called with bad variables mutating your containing function's state. It can not be replaced by 3rd party code. It is the best place to put any function.

OOP in Javascript

In Javascript an object is anything you create. To code in the OO way is to write Javascript, so you have created an OO program. But that said there are bad ways and good ways to write OO code.

Classes are bad.

Currently the syntactic sugar that lets you define objects using the class, constructor, super, and extends, tokens should be avoided as it encourages some very bad OO habits particularly the concept of encapsulation in terms of isolating data.

Encapsulation

To protect the state of an object and to be able to rely on that state you need to protect it from outside mutation and or modification.

In languages like C++ you have specific tokens that define the visibility and access to properties private and public. JavaScript does not have these tokens but uses closure to create public and private properties.

Using the class syntax makes it impossible to define hidden / protected variables and forces you to make public all data encapsulated by the object. This completely undermines the ideal of OOP

There is no one Object type.

Unlike languages like Java/C++ Javascript is a loosely classed language. There is no fixed object type, you can extend or modify an object at any time and you can not trust that an object created via a particular invocation eg const foo = new Animal() will in future behave or have the properties of an Animal This make protecting properties even more important in Javascript.

Special note you can use Object.seal and Object.defineProperty to lock in properties.

Looking at you code.

A list of general notes regarding your code.

  • Don't use class it's just bad...

  • Use strict mode by adding "use strict"; to the top of the JS file or code section.

  • Use const for constants, let for block scoped variables and var for function scoped variables.

  • Don`t use multi line variable declarations

// bad
var a = 0,
    b = 1    // << missing and easily overlooked comma 
    c = 2;   // << accidental ; before end also allowed 
    d = 1;

// good
var a = 0;
var b = 1;
var c = 2;
var d = 1;

// best
var a, b, c, d;
a = 0;
b = 1;
c = 2;
d = 1;
  • It is bad style to define a length variable inside a for statement
// bad
for (var w = 0, wLen = winCombinations.length; w < wLen; w++) {

// good    
for (var w = 0; w < winCombinations.length; w++) {

// best
var w;
for (w = 0; w < winCombinations.length; w += 1) {
// or
for (let w = 0; w < winCombinations.length; w += 1) {
  • Using aliases is common practice but be careful not to make the alias too obscure.

var z = this.currentPlayer; // z ???? I use the first letter of each word // so cp could be better

  • Use spread operator to convert array like objects to arrays. Also use plurals when naming arrays
var ceil = document.getElementsByClassName("game-item");

// as const and as an array and a better name than ceil 
const cells = [...document.getElementsByClassName("game-item")];
  • Empty arrays via the length property. This makes sure that you dont have to change other references to the array
var a = [1,2,3];
var b = a;
a = [];  // has a new array
         // but be still holds the original array

const a = [1,2,3];  // you can now use const
const b = a;
a.length = 0;  // array is empty
               // and so is b as it is still the same array reference
  • When changing text content of an element use its textContent property, it is quicker and prevents a re-flow that will happen if you use innerHTML or innerText

  • Rather than create and remove event handlers, create one event handler and use the game state to determine what to do with an event.

Your implementation

You have created "Game" as a class, but it is clear that "Game" can only exist by itself, what if I do the following:

const game1 = new Game();
const game2 = new Game();

This won't work so you should make sure that can not happen. Use a singleton to invoke only a single instance of the Game object

// There can be only one game.
const game = (() => {
    // protected and hidden properties go here
    const game = {
        // exposed game properties go here

    };
    return game;
})();

But looking further into your code the whole thing is self contained and there is no need to create a public object so you encapsulate the whole thing inside an anonymous singleton.

 ;(()=>{
     // all code in here safe from outside meddling. 
 })();

I have encapsulated the board and the player as single instances, exposing only what is needed. I use getters and setters to ensure that a valid state is maintained and that interpretation of the state is as needed.

Also the win test can be simplified using Array.some and Array.every. If "some" of the win combos have "every" item in the players moves array then that player wins.

Update looking back at the rewrite I saw that I had stuffed up the player. The player is what controls the game, so I have encapsulated the player providing an interface to control access to the player state. The player represents the current player.

A rewrite of your code

;(() => {  // anonymous singleton should have ; to stop error if code above
           // does not correctly put a ; at end of line
  "use strict";
  // you are not using forms or naming any elements so use direct reference
  // rather than indirect reference.
  resetGame.addEventListener("click", reset);

  var gameInPlay = false; // true and game is playing
  
  // encapsulate current player
  const player = (()=>{
    const winCombs = [
      [1, 2, 3], [1, 4, 7], [1, 5, 9], [7, 8, 9],
      [2, 5, 8], [3, 6, 9], [3, 5, 7], [4, 5, 6],
    ];
    var current = 0;
    const moves = [[],[]]; // two arrays to hold player move
    const tokens = "XO";   // token indexed by player value
    // interface 
    return {
      get tokens() { return tokens },
      get token() { return tokens[current] },
      set move(index) { moves[current].push(index) },
      get draw() { return moves[0].length + moves[1].length === 9 }
      next() { current = (current + 1) % 2 },
      newGame(starter = 0) {
          moves[0].length = moves[1].length = 0;
          current = starter === 0 ? 0 : 1;
      },
      get win() {
        return moves[current].length > 2 &&
          winCombs.some(combo => combo.every(pos => moves[current].indexOf(pos) > -1 ));
      },
    };
  })();


  // encapsulate the board using a singleton
  const board = (() => {
    const cells = [...document.getElementsByClassName("game-item")];
    gameBoard.addEventListener("click", (event) => {
      if (gameInPlay && event.target.textContent === "") {
        const cell = event.target;
        cell.textContent = player.token;
        cell.classList.add(player.token);
        move(Number(cell.dataset.cellIndex));
      }
    });
    return {
      clear() {
        cells.forEach(cell => {
          cell.classList.remove(...player.tokens);
          cell.textContent = "";
        })
      }
    };
  })();
  
  // Smart message replaces the char # in the text argument with
  // the player.
  const showMessage = (text) => {
    messageDisplay.textContent = text.replace("#", player.token);
  }

  // resets the game. this is called from reset click and at start
  function reset() {
    board.clear();
    player.newGame();
    showMessage("Player # move");
    gameInPlay = true;
  }

  // move takes cellIndex and add a move for the player
  // checks for win or draw ending game if so or switches player
  function move(cellIndex) {
    player.move = cellIndex;
    if (player.win) {
      showMessage("Player # wins");
      gameInPlay = false; 
    } else if (player.draw) {
      showMessage("It is a draw...");
      gameInPlay = false; 
    } else {
      player.next()
      showMessage("Player # move");
    }
  }
  // Call reset to start the first game.
  reset();

})();
* {
  box-sizing: border-box;
  outline: none;
}
.game-title {
  display: block;
  margin-bottom: 30px;
  font-size: 35px;
  font-weight: bold;
  text-align: center;
}
.game {
  display: flex;
  justify-content: space-between;
  flex-wrap: wrap;
  width: 152px;
  margin: 0 auto 50px;
  border: 1px solid #000;
}
.game-item {
  position: relative;
  width: 50px;
  height: 50px;
  line-height: 48px;
  border: 1px solid #000;
  cursor: pointer;
  text-align: center;
  font-size: 30px;
  font-weight: bold;
  transition: all linear 0.3s;
}
.game-item:hover {
  background-color: #ccc;
}
.game-item.O {
  background-color: #abfdab;
}
.game-item.X {
  background-color: #f99;
}
#reset-game {
  display: block;
  margin: 0 auto;
  border: 1px solid #000;
  border-radius: 20px;
  background-color: #fff;
  padding: 10px 20px;
  cursor: pointer;
  transition: all linear 0.3s;
}
#reset-game:hover {
  background-color: #ccc;
}
<!-- using JS friendly ids for direct referencing -->
<span class="game-title" id="messageDisplay"></span>
<div class="game" id="gameBoard">
  <div class="game-item" data-cell-index="1"></div>
  <div class="game-item" data-cell-index="2"></div>
  <div class="game-item" data-cell-index="3"></div>
  <div class="game-item" data-cell-index="4"></div>
  <div class="game-item" data-cell-index="5"></div>
  <div class="game-item" data-cell-index="6"></div>
  <div class="game-item" data-cell-index="7"></div>
  <div class="game-item" data-cell-index="8"></div>
  <div class="game-item" data-cell-index="9"></div>
</div>
<button id="resetGame">Clear</button>

\$\endgroup\$
  • \$\begingroup\$ Why is var a, b, c, d; a = 0; b = 1; c = 2; d = 1;"best"? Why is it better than you "good" suggestion? It seems to break the "declare variables where they are used" rule. And what is wrong with for (var w = 0, wLen = winCombinations.length; w < wLen; w++)? It seems to avoid an unnecessarily repeated property access. \$\endgroup\$ – RoToRa Jan 3 '18 at 9:17
  • 1
    \$\begingroup\$ @RoToRa Because variables declared withvar are hoisted to the top of the function and have function scope. adripofjavascript.com/blog/drips/… Placing variables together allows for better organization. Every javascript engine in common use has optimised iteration depth, there is no advantage declaring a second variable to hold length, and only adds unneeded noise to the source code and has an ever so slight performance hit.. \$\endgroup\$ – Blindman67 Jan 3 '18 at 9:33
  • \$\begingroup\$ It's a very annoying problem that class syntax does not offer private methods and properties. But when learning OOP, I would still recommend class syntax over writing nested objects and anonymous functions within functions. That approach - while enabling private members - quickly becomes unreadable and hard to debug. Other possible approaches are using Symbol inside a class, or using Typescript combined with modules. \$\endgroup\$ – Kokodoko Jan 3 '18 at 14:16
  • \$\begingroup\$ @Blindman67 Ok, I understand the second part, but I'm unsure about the variable declarations. I understand function scope and hoisting, but why does that make var a; a = 0;better than var a = 0;. \$\endgroup\$ – RoToRa Jan 3 '18 at 14:31
  • 1
    \$\begingroup\$ @Blindman67 But then you know where JS came from. JS was a complete mess when it was first introduced. There is no other language that has changed so fast and so often as JS, so why stop here? I'm not even saying JS should go full OOP, but I am saying that syntax like ;(() => {}) and func = () => {} does not clearly express the intention of the code.... JS needs clearer language expressions, be it functional or OOP :) Class syntax attempts to do that :) \$\endgroup\$ – Kokodoko Jan 4 '18 at 11:42
1
\$\begingroup\$

How can I get rid of functions in methods?

You can start by making those in-line functions methods. For example, currentStep (the click handler) can be a method that accepts a click event argument:

currentStep(clickEvent) {

Then use Function.bind() to bind the click handlers to that method:

addX() {
    this.boundCurrentStep = this.currentStep.bind(this);
    for (var i = 0; i < ceil.length; i++) {
      ceil[i].addEventListener("click", this.boundCurrentStep);
    }

So in currentStep the this argument will refer to the current Game instance. Replace z with this and then any place this was previously used to access the div element, refer to clickEvent.target. Also, call changePlayer and checkWin on `this, since we will be converting those to methods as well (see below).

currentStep(clickEvent) {
    var num = +clickEvent.target.getAttribute("data-ceil");
    if (!clickEvent.target.textContent) {
      clickEvent.target.innerText = this.currentPlayer;

      this.currentPlayer === "X" ?
        dataX.push(num) && clickEvent.target.classList.add("x") :
        dataO.push(num) && clickEvent.target.classList.add("o");
      if (
        (dataO.length > 2 || dataX.length > 2) &&
        (this.checkWin(dataO, num) || this.checkWin(dataX, num))
      ) {
        for (var i = 0; i < ceil.length; i++) {
          ceil[i].removeEventListener("click", this.boundCurrentStep);
        }
        return (message.innerText = "Win player " + this.currentPlayer);
      }

      this.changePlayer();
      stepCount++;
      (stepCount == 9) ? (message.innerText = 'Tie') : (message.innerText = 'The player ' + this.currentPlayer);
    }
  }

And also make those other functions methods: checkWin and changeWin, and replace z with this.currentPlayer there as well.

Then when the game is over, use removeEventListener() passing the function bound to the method currentStep() back in the add method (i.e. this.boundCurrentStep).

Also, the click event listener function for the reset button can also use .bind(this) to properly reference the Game instance:

reset.addEventListener("click", function() {
    //..reset data on this game
}.bind(this));

Ternary simplification:

There is a function to change the player:

function changePlayer() {
        z === 'X' ? (z = "O") : (z = "X");
    }

making that into a method and replacing z with `this.currentPlayer would change this to:

changePlayer() {
    this.currentPlayer === 'X' ? (this.currentPlayer = "O") : (this.currentPlayer = "X");
}

This can be simplified by moving the assignment out of the ternary operator:

this.currentPlayer = this.currentPlayer === 'X' ? "O" : "X";

Other suggestion

I would advise you to use an event delegate - instead of adding a click handler to each square, add a single click handler for the board and check the target of the event. This can simplify the event registration on elements greatly.

var ceil = document.getElementsByClassName("game-item"),
  reset = document.getElementById("reset-game"),
  message = document.getElementById("message"),
  stepCount = 0,
  winCombinations = [
    [1, 2, 3],
    [1, 4, 7],
    [1, 5, 9],
    [2, 5, 8],
    [3, 6, 9],
    [3, 5, 7],
    [4, 5, 6],
    [7, 8, 9]
  ],
  dataX = [],
  dataO = [];

class Player {

  constructor(name) {
    this.name = name;
  }
}


class Game {

  constructor() {
    this.playerX = new Player("X");
    this.playerO = new Player("O");
    this.currentPlayer = this.playerX.name;

    console.log(this.currentPlayer);
  }

  addX() {
    this.boundCurrentStep = this.currentStep.bind(this);
    for (var i = 0; i < ceil.length; i++) {
      ceil[i].addEventListener("click", this.boundCurrentStep);
    }
  }
  changePlayer() {
    this.currentPlayer = this.currentPlayer === 'X' ? "O" : "X";
  }


  checkWin(arr, number) {

    for (var w = 0, wLen = winCombinations.length; w < wLen; w++) {
      var someWinArr = winCombinations[w],
        count = 0;
      if (someWinArr.indexOf(number) !== -1) {
        for (var k = 0, kLen = someWinArr.length; k < kLen; k++) {
          if (arr.indexOf(someWinArr[k]) !== -1) {
            count++;
            if (count === 3) {
              return true;
            }
          }
        }
        count = 0;
      }
    }
  }
  currentStep(clickEvent) {
    var num = +clickEvent.target.getAttribute("data-ceil");
    if (!clickEvent.target.textContent) {
      clickEvent.target.innerText = this.currentPlayer;

      this.currentPlayer === "X" ?
        dataX.push(num) && clickEvent.target.classList.add("x") :
        dataO.push(num) && clickEvent.target.classList.add("o");
      if (
        (dataO.length > 2 || dataX.length > 2) &&
        (this.checkWin(dataO, num) || this.checkWin(dataX, num))
      ) {
        for (var i = 0; i < ceil.length; i++) {
          ceil[i].removeEventListener("click", this.boundCurrentStep);
        }
        return (message.innerText = "Win player " + this.currentPlayer);
      }

      this.changePlayer();
      stepCount++;
      (stepCount == 9) ? (message.innerText = 'Tie') : (message.innerText = 'The player ' + this.currentPlayer);
    }
  }
  reset() {
    reset.addEventListener("click", function() {
      for (var i = 0; i < ceil.length; i++) {
        ceil[i].innerText = "";
      }
      dataO = [];
      dataX = [];
      this.currentPlayer = "X";
      stepCount = 0;
      message.innerText = "The player " + this.currentPlayer;
      for (var i = 0; i < ceil.length; i++) {
        ceil[i].classList.remove("x", "o");
         ceil[i].addEventListener("click", this.boundCurrentStep);
      }
    }.bind(this));
  }
}

const game = new Game();
game.addX();
game.reset();
* {
  box-sizing: border-box;
  outline: none;
}

.game-title {
  display: block;
  margin-bottom: 30px;
  font-size: 35px;
  font-weight: bold;
  text-align: center;
}

.game {
  display: flex;
  justify-content: space-between;
  flex-wrap: wrap;
  width: 152px;
  margin: 0 auto 50px;
  border: 1px solid #000;
}

.game-item {
  position: relative;
  width: 50px;
  height: 50px;
  line-height: 48px;
  border: 1px solid #000;
  cursor: pointer;
  text-align: center;
  font-size: 30px;
  font-weight: bold;
  transition: all linear 0.3s;
}

.game-item:hover {
  background-color: #ccc;
}

.game-item.o {
  background-color: #abfdab;
}

.game-item.x {
  background-color: #f99;
}

#reset-game {
  display: block;
  margin: 0 auto;
  border: 1px solid #000;
  border-radius: 20px;
  background-color: #fff;
  padding: 10px 20px;
  cursor: pointer;
  transition: all linear 0.3s;
}

#reset-game:hover {
  background-color: #ccc;
}
<span class="game-title" id="message">The player X</span>
<div class="game">
  <div class="game-item" data-ceil="1"></div>
  <div class="game-item" data-ceil="2"></div>
  <div class="game-item" data-ceil="3"></div>
  <div class="game-item" data-ceil="4"></div>
  <div class="game-item" data-ceil="5"></div>
  <div class="game-item" data-ceil="6"></div>
  <div class="game-item" data-ceil="7"></div>
  <div class="game-item" data-ceil="8"></div>
  <div class="game-item" data-ceil="9"></div>
</div>
<button id="reset-game">Clear</button>

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you very mutch! But in your code there is a small bug. When you clicked on the square twice - X change to O with red color. It's becouse we used clickEvent. How can you fix this moment? Thank you! \$\endgroup\$ – jollasf Jan 3 '18 at 22:13
  • \$\begingroup\$ Ah - good catch - I forgot to update the conditional that checks the contents of the cell to see if it already has something (i.e. if (!this.textContent) { should become: if (!clickEvent.target.textContent) {) \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 3 '18 at 22:24

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.