First app with JS in OOP

Question

I've already started to creating apps in JS with OOP. I made simple TicTacToe Game in OOP and I want to ask you to review my code and point out everything that I can change about OOP. What is bad practice and what should I pay attention for? Thank you!

class Game {
    constructor(figures, boxes) {
        this.firstPlayer = '';
        this.buttons = figures;
        this.boxes = boxes;
        this.winingCombinations=[
            [0, 1, 2],
            [3, 4, 5],
            [6, 7, 8],
            [0, 3, 6],
            [1, 4, 7],
            [2, 5, 8],
            [0, 4, 8],
            [2, 4, 6]
        ];
        this.arrO = [];
        this.arrX = [];
    }

    disableButtons(){
        this.buttons.forEach((btn) => {
            btn.classList.add('disactive');
        })
    }

    drawFigure(e){
        e.innerHTML = `<span>${this.firstPlayer}</span>`;
    }
    
    disableBox(e){
        e.classList.add('placeholder-disactive');
    }

    showPopup(winner){
        let popup = document.querySelector('.bg');
        if(winner === "Remis"){
            popup.querySelector('.winner-is').innerHTML=winner;
        } else {
            popup.querySelector('.winner-is').innerHTML="Wygrywa: " + winner;
        }
        popup.classList.add('bg-active');
    }
    
    toggleFigure(){
        if(this.firstPlayer === "O"){
            this.firstPlayer = "X";
        } else {
            this.firstPlayer = "O";
        }this.firstPlayer
        return this.firstPlayer;
    }

    addToArr(target,label){
        let arr = eval('this.arr' + this.firstPlayer);
        arr.push(target.getAttribute('data-number')*1);
        this.checkArr(arr,this.firstPlayer,label);
    }
    
    checkArr(arr,winner){
        let allFounded = false;
        if(arr.length >= 3){
            for(let i = 0;i < this.winingCombinations.length; i++){
                allFounded = this.winingCombinations[i].every( ai => arr.includes(ai));
                if(allFounded === true){
                    this.endOfGame(this.winingCombinations[i],winner);
                    return;
                }
                 else if(arr.length === 5 && allFounded === false){
                    console.log('draw');
                    this.endOfGame([],'Remis');
                }
            }
        }
    }

    endOfGame(winnerSet,winner){
        this.boxes.forEach((box) => {
            box.classList.add('endGame');
            box.classList.add('placeholder-disactive');
            for(let i = 0; i < winnerSet.length; i++){
                box.getAttribute('data-number')*1 == winnerSet[i] ? box.classList.add('win') : 0;

            }
        })
        this.showPopup(winner);
    }

    setStartPlayer(player = this.firstPlayer) {
        this.firstPlayer = player;
        document.querySelector('.select__player .player').innerHTML = player;
    }

    disablePopup(){
        document.querySelector('.bg').classList.remove('bg-active');
    }

    activeBoxes(action){
        this.boxes.forEach((box) => {
            eval(`box.classList.${action}('placeholder-disactive')`);
        })
    }

    activeBtns(){
        this.buttons.forEach((figure) => {
            figure.classList.remove('disactive');
        })
    }
    clearArrays(){
        this.arrO = [];
        this.arrX = [];
    }

    clearBoxes(){
        this.boxes.forEach((box) => {
            box.innerHTML = '';
            box.className = 'placeholder';
        })
    }

    reset(){
        this.disablePopup();
        this.clearBoxes();
        this.activeBtns();
        this.clearArrays();
        this.setStartPlayer('');
    }

    boxHandle(event){
        const label = document.querySelector('.select__player .text');
        label.innerHTML = 'Tura gracza: ';
        this.drawFigure(event.target);
        this.disableBox(event.target);
        this.addToArr(event.target,label);
        this.toggleFigure();
        this.setStartPlayer();
    }

    popupHandle(event){
        if(event.getAttribute('data-answear') == "Tak"){
            this.reset();
            this.activeBoxes('add');
        } else {
            this.disablePopup();
        }
    }

    init() {;
        this.boxes.forEach((box) => {
            box.addEventListener('click', (event) => {
                this.boxHandle(event);
            })
        })
        this.buttons.forEach((button) => {
            button.addEventListener('click', () => {
                this.firstPlayer = button.innerHTML;
                this.setStartPlayer();
                this.disableButtons();
                this.activeBoxes('remove');
            })
        })
    }
}

document.addEventListener('DOMContentLoaded', (e) => {
    const figures = document.querySelectorAll('.select__box');
    const boxes = document.querySelectorAll('.placeholder');
    const popup = document.querySelector('.btns');
    newGame = new Game (figures, boxes);
    newGame.init();
    popup.addEventListener('click', (e) => {
        newGame.popupHandle(e.target);
    })
    
})
```

Blindman67 · Accepted Answer · 2023-06-03 23:14:01Z

Few review points.

Messy formating, inconsistent use of spacing, newlines, and some poor indentation.

if(...){                     // use spaces. eg if ( ) {
}
else if(                    // same line as closing }. eg } else if (

('...number')*1              // space between operators. eg ('...number') * 1

endGame(winSet,wins){        // spaces. eg endGame(winSet, wins) {

Use modern JS

Example from your code

for(let i = 0; i < winnerSet.length; i++){
  box.getAttribute('data-number')*1 == winnerSet[i] ? box.classList.add('win') : 0;
}

Can be written as

for (const idx of winnerSet) {
  box.dataset.number == idx && box.classList.add("win");
}

Why need the second required call to Game.init() outside the object Game? Better to do that in the constructor.
Define constants associated with the Object as static. Eg
```
class Game {
  static winingCombinations = [[0, 1, 2], [3, 4, 5], [6, 7, 8], .... ];
```
Accessible via Game.winingCombinations

Note that it can not easily be made constant using class syntax.
There is no state protection using private properties or functions.

Using class syntax you can create private members using the prescript # for example.
```
class Game {
  static #winingCombinations = [[0, 1, 2], [3, 4, 5], [6, 7, 8], .... ];
```
Accessible only from within the class via Game.#winingCombinations

Almost all properties and function should be private. This ensures that your code can trust the integrity of the objects state.

Singleton

Your object Game can (and as your example shows) be a singleton.

Your Game can ONLY be instantiated once, creating a second Game object would interfere with the existing instances.

The class syntax may not be the best option in this case.

Defining Objects.

The class syntax is not the only way to define Objects in JS.

Your Game object would be much better suited to a object factory like IIFE totally eliminating the need to fill the code with this. (51 times in 150 lines of your code, that a lot of code noise).

An IIFE that uses closure to encapsulate state can totally trust its state. No outside code has access or can make changes.

It can't be accidentally instanced more than once. As an IIFE in the global scope it will only ever be instantiated once

thumbs up for IIFE suggestion. Class syntax is just "syntax sugar" anyway (i.e. JS does not have classes, only objects) and does not deter the brain dump linear stream of consciousness that is typical JS coding. Oh, Objects. Wherefore art thou? — radarbob
– radarbob, Commented Jun 4, 2023 at 3:34
Thank you very much for such an comprehensive reply. I have read a little about IIFE and will also try to make a simple game like hangman using IIFE and will share the code here. I thought that you should use new class syntax due to the fact that it is new, but I see now that I should learn in which cases what syntax to use. Do you know any good site or repository that describes when to use available objects? — Szymon Mińko
– Szymon Mińko, Commented Jun 17, 2023 at 13:22
@SzymonMińko The links in my answer link to MDN which has a comprehensive web related references, tutorials, and links. — Blindman67
– Blindman67, Commented Jun 17, 2023 at 22:32

Kokodoko · Accepted Answer · 2023-06-20 11:12:34Z

Just some tips: if you want to execute your code after the DOM content has loaded you can use the defer attribute. Then you can put all your logic in the game class instead of in the DOMContentLoaded handler:

<script defer src="mygame.js"></script>

You have coded one single huge class, which kind of defeats the purpose of OOP. You could think about creating a separate class to draw the Game Board. In this example I show the "domcontentloaded" code and the board class:

class Game() {

   board
   figures
   boxes
   popup

   constructor() {
      this.figures = document.querySelectorAll('.select__box');
      this.boxes = document.querySelectorAll('.placeholder');
      this.popup = document.querySelector('.btns');
      this.popup.addEventListener('click', (e) => this.popupHandle(e.target))

      this.board = new Board()
      this.init()
   }

   init() {
   }

   popupHandle(target){
   }

   stateWasChanged() {
      this.board.updateState()
   }
}

class Board() {
   updateState() {
       console.log("draw the board")
   }
}

new Game()

Stack Exchange Network

First app with JS in OOP

2 Answers 2

Few review points.

Singleton

Defining Objects.

You must log in to answer this question.

Hot Network Questions

2 Answers 2

Few review points.

Defining Objects.

You must log in to answer this question.

Related