7
\$\begingroup\$

I wrote a 2048 game in JavaScript with an object-oriented paradigm. The game board is presented with a two-dimensional array and each tile holds an integer.

Here is the implementation:

class Game {
  SIZE = 4
  constructor() {
    this.board = Array.from({ length: this.SIZE * this.SIZE }, () => 0).reduce(
      (arrays, curr) => {
        const lastArray = arrays[arrays.length - 1]
        if (lastArray.length < this.SIZE) lastArray.push(curr)
        else arrays.push([curr])
        return arrays
      },
      [[]]
    )
    this.isWin = false
    this._init()
  }

  _init() {
    const pickedTiles = this._randomlyPick(2)
    for (const [row, col] of pickedTiles) {
      this.board[row][col] = Game.generateTile()
    }
  }

  static generateTile() {
    if (Math.random() > 0.5) return 2
    return 4
  }

  _getEmptyTiles() {
    const emptyTiles = []
    for (let row = 0; row < this.SIZE; row++) {
      for (let col = 0; col < this.SIZE; col++) {
        if (this.board[row][col] === 0) emptyTiles.push([col, row])
      }
    }
    return emptyTiles
  }

  _randomlyPick(numOfItems) {
    const emptyTiles = this._getEmptyTiles()
    for (let i = 0; i < numOfItems; i++) {
      const toSwap = i + Math.floor(Math.random() * (emptyTiles.length - i))
      ;[emptyTiles[i], emptyTiles[toSwap]] = [emptyTiles[toSwap], emptyTiles[i]]
    }
    return emptyTiles.slice(0, numOfItems)
  }

  spawn() {
    // randomly spawn empty tiles with 2 or 4
    const [emtpyTile] = this._randomlyPick(1)
    this.board[emtpyTile[0]][emtpyTile[1]] = Game.generateTile()
  }

  play(dir) {
    if (this.canPlay()) {
      switch (dir) {
        case Game.moveUp:
          this._mergeUp()
          break
        case Game.moveRight:
          this._mergeRight()
          break
        case Game.moveLeft:
          this._mergeLeft()
          break
        case Game.moveDown:
          this._mergeDown()
          break
      }
      this.spawn()

      return true
    }
    return false
  }
  checkIsWin() {
    return this.isWin
  }

  static peek(array) {
    return array[array.length - 1]
  }

  static zip(arrays) {
    const result = []
    for (let i = 0; i < arrays[0].length; ++i) {
      result.push(arrays.map((array) => array[i]))
    }
    return result
  }

  _mergeRowRight(sparseRow) {
    const row = sparseRow.filter((x) => x !== 0)
    const result = []
    while (row.length) {
      let value = row.pop()
      if (Game.peek(row) === value) value += row.pop()
      result.unshift(value)
    }

    while (result.length < 4) result.unshift(0)
    return result
  }

  _mergeRowLeft(row) {
    return this._mergeRowRight([...row].reverse()).reverse()
  }

  _mergeUp() {
    this.board = Game.zip(Game.zip(this.board).map(row => this._mergeRowLeft(row)))
  }
  _mergeDown() {
    this.board = Game.zip(Game.zip(this.board).map(row => this._mergeRight(row)))
  }

  _mergeRight() {
    this.board = this.board.map((row) => this._mergeRowRight(row))
  }
  _mergeLeft() {
    this.board = this.board.map((row) => this._mergeRowLeft(row))
  }

  canPlay() {
    const dirs = [
      [0, 1],
      [1, 0],
      [-1, 0],
      [0, -1],
    ]
    const visited = new Set()
    for (let row = 0; row < this.SIZE; row++) {
      for (let col = 0; col < this.SIZE; col++) {
        if (visited.has([row, col].toString())) continue
        const tile = this.board[row][col]
        if (tile === 2048) {
          this.isWin = true
          return false
        }
        if (tile === 0) return true
        for (const [dx, dy] of dirs) {
          if (this.board[row + dx]?.[col + dy] === tile) return true
        }
        visited.add([row, col].toString())
      }
    }
    return false
  }
}

Game.moveUp = Symbol('moveUp')
Game.moveDown = Symbol('moveUp')
Game.moveLeft = Symbol('moveUp')
Game.moveRight = Symbol('moveUp')

const game = new Game()
console.log(game.board);
game.play(Game.moveUp)
console.log(game.board);
game.play(Game.moveRight)
console.log(game.board);

Any feedback is welcomed. Specifically, I would like to know:

  1. Is it idiomatic in OO to use static methods to store util functions such as zip to flip the board along its diagonal?
  2. Is there a better way to structure the class? I feel like I am not doing a great job at dividing some of the logic of different actions.
  3. Is there any specific design pattern that I can use to improve the class?
  4. Lastly, I am using symbol to present the direction of the move the user can make and attach them as the instance variable to the class. Again I am not sure if this is idiomatic in OO since I am kinda new to this paradigm.
\$\endgroup\$
5
+25
\$\begingroup\$

Symbol

A quick answer in regard to the use of Symbol.

"Lastly, I am using symbol to present the direction of the move the user can make and attach them as the instance variable to the class. Again I am not sure if this is idiomatic in OO since I am kinda new to this paradigm."

There are 2 reasons to create a Symbol

  1. To guarantee uniqueness Symbol("foo") === Symbol("foo"); is always false.
  2. To share unique values between isolated scopes. Eg Symbol.for("foo") === Symbol.for("foo"); is true across the global scope. Useful when using modules.

Pros

See above

Cons

  • Symbol comes with a very slight overhead approx 5% slower (chrome) than accessing Number so in applications where performance is highly critical they should be avoided.

  • Creating a new symbol Symbol() is 2 orders of magnitude slower than creating a unique number i++

  • Symbol can not be serialized. eg JSON.stringify({a: symbol("foo")}) creates "{}"

  • Due to the nature of global symbols Symbol.for each symbol created thus, represents an unrecoverable memory assignment of approx 50bytes plus the name length (Unicode) per symbol.

    The same applies to scoped symbols however that memory can be recovered when the scope is de-referenced.

    Care must be taken to not create symbols in loops.

I mention the cons, though they are not applicable in this situation, but rather that using them may become habitual and one can easily get caught out if unaware of the problems.

Answering the question

Is your use of symbol idiomatic?

No. The requirement for uniqueness is not there to warrant the added complexity. The need to identify a move can easily be achieved using a set of numbers

Game.moveUp = 0;
Game.moveDown = 1;
Game.moveLeft = 2;
Game.moveRight = 3;

There is no possibility of a clash as the domain of uniqueness is limited to the 4 entities.

The first rule of coding. With all things equal the simplest solution is the best.

BTW

Poor naming.

If you find a set of variable names all starting with the same name/prefix its a sign that it should be an object.

Example enumeration using block scoped counter.

{
    let i = 0;
    Game.moves = {
        up: i++,
        down: i++,
        left: i++,
        right: i++,
    };
}

Or better as constants

{
    let i = 0;
    Game.moves = Object.freeze({
        UP: i++,
        DOWN: i++,
        LEFT: i++,
        RIGHT: i++,
    });
}
\$\endgroup\$
3
\$\begingroup\$

Initial feedback

The code looks quite clean. It is a very easy to read. There are only a few suggestions, which I have ingrained in the question responses below.

Question responses

  1. Is it idiomatic in OO to use static methods to store util functions such as zip to flip the board along its diagonal.

Yes AFAIK that is true.

Static methods are often used to create utility functions for an application, whereas static properties are useful for caches, fixed-configuration, or any other data you don't need to be replicated across instances.1

  1. Is there any specific design pattern that I can use to improve the class?

    It appears that some of the code uses functional programming techniques - e.g. in the static zip method the map() array method is used.

      static zip(arrays) {
        const result = []
        for (let i = 0; i < arrays[0].length; ++i) {
          result.push(arrays.map((array) => array[i]))
        }
        return result
      }
    

    That could be taken one step further to use map instead of the outer for loop:

    static zip(arrays) {
      return arrays[0].map((firstCol, i) => arrays.map(array => array[i]));
    }
    

    There are some places where functional programming is used when it doesn't need to be - for example, in the constructor:

    this.board = Array.from({ length: this.SIZE * this.SIZE }, () => 0).reduce(
    

    This could simply use the Array method .fill()

    this.board = Array(this.SIZE * this.SIZE).fill(0).reduce(
    

    And the multiplication could be simpler using ES6 exponentiation operator

    this.board = Array(this.SIZE ** 2).fill(0).reduce(
    
  2. Lastly, I am using symbol to present the direction of the move the user can make and attach them as the instance variable to the class. Again I am not sure if this is idiomatic in OO since I am kinda new to this paradigm.

    I honestly haven't seen much usage of Symbol but perhaps that is likely only because it is a newer feature introduced with ES6 2.

    While they are not technically invalid, perhaps these three lines should be updated

    Game.moveDown = Symbol('moveUp')
    Game.moveLeft = Symbol('moveUp')
    Game.moveRight = Symbol('moveUp')
    

    like this:

    Game.moveDown = Symbol('moveDown')
    Game.moveLeft = Symbol('moveLeft')
    Game.moveRight = Symbol('moveRight')
    

    If those lines were updated, then the switch statement in method play() can be replaced by this:

    const methodName = '_merge' + dir.description.replace('move', '')
    if (typeof this[methodName === 'function') {
      this[methodName]()
    } 
    

Other feedback

Line endings

Perhaps you are using a compiler (e.g. BableJS) or other tool that handles it but unless you fully understand the ways Automatic semicolon insertion can be broken by certain statements, consider adding semi-colons to terminate the lines.

2048 is a Magic Number

One could argue that numbers like 0.5 and 2048, even though it is the name of the game, may not have much meaning to anyone reading your code. While those values may never change, consider making them a constant or value that could be supplied via a parameter, in case there would be a need to support such features.

\$\endgroup\$
2
\$\begingroup\$

Programming paradigms, like OOP, can be thought of like an art style. Take the impressionism art style for example. This art style is characterized by traits such as visible brush strokes, a tendency to depict ordinary subject matters, etc. The more impressionism traits a work of art has, the more impressionist it is.

Similarly, a program will lean more towards OOP if it commonly exhibits traits such as encapsulation, polymorphism, and inheritance.

With that said, let's measure up your program to these three traits I just mentioned.


Encapsulation

Encapsulation is when we hide implementation details from the outside world, and only allow users to interact with our instance through the means we've provided. For example:

class EventEmitter {
  #listeners = []
  subscribe(fn) {
    this.#listeners.push(fn)
  }
  trigger(...args) {
    this.#listeners.forEach(fn => fn(...args))
  }
}

The fact that we're using a list to store the listeners is hidden from the outside world, and can safely be swapped for a set at any without affecting any other code.

In this example, I used a class, but it's good to recognize that the exact same amount of encapsulation could be achieved using a factory function. the class syntax is not needed at all to make OOP code.

It's good to note that there are different levels of encapsulation that you can achieve. In the example above, the following things would weaken that class's encapsulation:

  • Changing this.#listeners to this.listeners, exposing the data publicly, and allowing direct read and mutation of this internal data.
  • Exposing a read-only version of this.#listeners - this is better than allowing mutations, but still not ideal.
  • creating a getListeners() function, that simply returns this.#listeners. Many beginner programmers hear that they're never supposed to expose their private members publicly, so they just create a bunch of functions that effectively does the same thing. This does not achieve more encapsalation.

Note that doing these things isn't necessarily bad (depending on the scenario), they're just not OOP.

Now let's take a look at your program.

Your entire program resides in one class. This means the only encapsulation layer that exists is between this class and the little snippet of driver code that uses it. In other words, the internal details to almost the entire program are available to almost the entire program.

This single encapsulation layer could be improved too. You're exposing a lot of things publicly that could really be private (by which I mean, more things ought to be named with a leading underscore, to indicate that it's an internal detail).

Here are some members I might consider marking as private with a leading underscore:

  • generateTile()
  • spawn()
  • isWin
  • peek()
  • zip()

Polymorphism

A polymorphic function is a function that can take any object that follows a specific interface, and operate on it. An example polymorphic function:

function attack(enemy, damage) {
  enemy.health -= damage
  if (enemy.health <= 0) enemy.die()
}

const slime = { health: 2, die() { console.log('Slime dies') } }
const immortalRabbit = { health: 2, die() { this.health = 10 } }
attack(slime, 3)
attack(immortalRabbit, 3)

As your program only has one class, then there can't be any kind of polymorphism going on, at least with your own custom classes of objects.

Inheritance

I won't dwell on this too much, as it often gets overused. Inheritance is unique to OOP, which is why it's an important trait to mention, but you can write perfectly good OOP code without ever touching it. Just as an art style can change over time, so too does the OOP paradigm.

Making this code more OOP

Each of these traits discussed above are tale-tale signs of OO code. The more of them your code has, the more OO it is. However, don't artificially pump this into your program to achieve OOP - there's a right time, and a wrong time to use these them.

Encapsulation, for example, is a powerful tool that can be used to greatly simplify a program by hiding away implementation details. But, when used in the wrong places, this same tool can create annoying barriers, and force some nasty workarounds to crop up in the codebase.

I worked really hard at trying to make your code more OOP, but each time I tried to add encapsulation, or make some aspect exhibit polymorphic behavior, all I ended up doing was complicating the code without any real benefits. In all honesty, I think OOP is just the wrong tool for this job.

As I've been working at this answer, I've also learned a lot about when it's good to use OOP and when it's not so great. One thing I've come to realize is that the traditional OOP examples aren't actually the examples where OOP really shines. i.e. there's really not private, implementation details to a "square" or a "triangle". Modeling a rectangle as a class with getWidth(), getHeight(), and getArea() doesn't provide much more encapsulation then an object with a { width: ..., height: ... } shape and a separate function called calculateAreaOfRectangle().

UI components, on the other hand, tend to need a lot of encapsulation. It's not a webpage's header's business whether or not its child logo component is in a mouse-hover state or not, nor should it care whether or not its child search-box component has text in it or not.

Answering questions

Is it idiomatic in OO to use static methods to store util functions such as zip to flip the board along its diagonal?

For utility functions such as zip(), you certainly can make them static methods of your class (though mark it as private! It's an implementation detail to the class). This is common to do in java, as there's really no other option there. In javascript though, you have other options which can be cleaner, like, just making those into module-level functions.

Is there a better way to structure the class? I feel like I am not doing a great job at dividing some of the logic of different actions.

There is one larger simplification I can think of. You could make all of your _mergeLeft/right/up/down functions return the new board, instead of assigning it to this._board - the caller of these merge functions can do that assignment if needed. Doing this would let you redefine canPlay() and checkIsWin() into something like this:

const WIN_CONDITION = 2048

const compare2dArray = (arrays1, arrays2) => arrays1.every((row, i) => (
  row.every((value, j) => value === arrays2[i][j])
))

class Game {
  // ...
  canPlay() {
    if (this.checkIsWin()) return false
    return (
      compare2dArray(this._mergeLeft(), this._board) &&
      compare2dArray(this._mergeRight(), this._board) &&
      compare2dArray(this._mergeUp(), this._board) &&
      compare2dArray(this._mergeDown(), this._board)
    )
  }

  checkIsWin() {
    return this._board.some(row => (
      row.some(value => value >= WIN_CONDITION)
    ))
  }
}

Is there any specific design pattern that I can use to improve the class?

None of the OOP design patterns that I know of will really help much with this problem.

I am using symbol to present the direction of the move the user can make and attach them as the instance variable to the class. Again I am not sure if this is idiomatic in OO since I am kinda new to this paradigm.

You're effectively emulating an enum with these symbols. I know some javascript programmers like to use symbols this way to make their enums. It doesn't matter too much what you set the values to, as long as they're unique from each other - I normally just set them to different strings (i.e. 'UP' or 'DOWN').

I would recommend grouping the directions together into a single directions object, as they're all part of the same pseudo-enum.

In this specific scenario, you're creating this pseduo-enum to distinguish between different directions, then you're using a switch() later on to do different behaviors depending on the chosen direction. You could instead just attach behaviors to the enum values directly, getting rid of the switch all-together. For example:

class Game {
  static move = {
    up: { _merge: board => Game.zip(Game.zip(board).map(row => Game._mergeRowLeft(row))) },
    down: { _merge: board => Game.zip(Game.zip(board).map(row => Game._mergeRowRight(row))) },
    left: { _merge: board => board.map(row => Game._mergeRowLeft(row)) },
    right: { _merge: board => board.map(row => Game._mergeRowRight(row)) },
  }

  play(dir) {
    if (!this.canPlay()) return false
    this._board = dir._merge(this._board)
    this.spawn()
    return true
  }
}

Summary

It's great that you keep working on the same program, trying to apply different techniques on it to improve its quality. This is one of the best ways to improve your craft, so keep up the good work :).

(Note that because I've already answered a similar question from the O.P. here, I've chosen to just focus on the OOP aspect of their code, as that's the primary reason they're asking the question)

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