2
\$\begingroup\$

I wanted to try out classes in Javascript and implemented a snake game. You can control the snake by using the arrow keys.

I would be interested to know if the source code can be improved in terms of class structure or source code structure in general. I never implemented snake in an other language and also didnt look into examples for doing that - so maybe this code seems a little bit odd to game programmers.

Class Structure

There are four Classes.

  • Table: Manages the graphical display of a table and provides fundamental functions for changing the look of cells.
  • Position: Represents a position with an x and an y coordinate.
  • Snake: Manages "body parts" that are just a list of positions and can move and grow.
  • GameBoard: Puts all these classes together to a game and also manages the graphical display via a Table instance.

There is also a function for checking which arrow key is pressed, and a function that repeatedly calls the mainloop of the gameboard-object.

<html>
    <head>
        <title>Snake</title>
        <style>
          table {
            border: 1px solid;
          }
        
          td {
              width: 25px;
              height: 25px;
          }
        </style>
    </head>
    <body>
        <div id="table"></div>
    </body>
    <script>
      /* classes */

      class Table {
        rows;
        columns;
        tableNode;

        constructor(rows, columns) {
          this.rows = rows;
          this.columns = columns;
        }

        initTableNode() {
          this.tableNode = document.createElement("table");
          for (let i = 0; i < this.rows; i++) {
            let tr = document.createElement("tr");
            for (let j = 0; j < this.columns; j++) {
              let td = document.createElement("td");
              tr.appendChild(td);
            }
            this.tableNode.appendChild(tr);
          }
        }

        changeColor(x, y, color) {
          if (x >= 0 && x < this.rows && y >= 0 && y < this.columns)
            this.tableNode.rows[x].cells[y].style.background = color;
        }

        makeWhite() {
          for (let i = 0; i < this.rows; i++) {
            for (let j = 0; j < this.columns; j++) {
              this.tableNode.rows[i].cells[j].style.background = "white";
            }
          }
        }
      }

      class GameBoard {
        table;
        snake;
        applePosition;
        gameOver;

        constructor(height, width) {
          this.table = new Table(height, width);
          this.snake = new Snake();
          this.makeRandomApple();
        }

        makeRandomApple() {
          start: while (true) {
            let x = randomInt(0, this.table.rows-1);
            let y = randomInt(0, this.table.columns-1);
            this.applePosition = new Position(x, y);

            // check if snake is already there
            for (let i = 0; i < this.snake.bodyParts.length; i++) {
              if (this.snake.bodyParts[i].x == x && this.snake.bodyParts[i].y == y) {
                continue start;
              }
            }
            return;
          }
        }

        checkIfAppleIsEaten() {
          if (this.snake.head().x == this.applePosition.x && this.snake.head().y == this.applePosition.y) {
            this.snake.grow();
            this.makeRandomApple();
          }
        }

        gameLoop() {
          this.snake.moveForward(direction);
          this.checkIfAppleIsEaten();

          // check if snake crashes into border
          let head = this.snake.head();
          if (head.x < 0 || head.x >= this.table.rows || head.y < 0 || head.y >= this.table.columns) {
            this.gameOver = true;
          }

          // check if snakes crashes into himself
          let bodyParts = 0;
          for (let i = 0; i < this.snake.bodyParts.length; i++) {
            if (head.x == this.snake.bodyParts[i].x && head.y == this.snake.bodyParts[i].y) {
              bodyParts += 1;
            }
            if (bodyParts > 1) {
              this.gameOver = true;
              break;
            }
          }

          this.table.makeWhite();

          this.table.changeColor(this.applePosition.x, this.applePosition.y, "green");

          // draw snake
          for (let i = 0; i < this.snake.bodyParts.length; i++) {
            this.table.changeColor(this.snake.bodyParts[i].x, this.snake.bodyParts[i].y, "red");
          }
        }
      }

      class Snake {
        bodyParts;
        // can contain strings: left, up, right, down
        directionCurrently;
        grown;

        constructor() {
          let p1 = new Position(1, 1);
          let p2 = new Position(1, 2);
          let p3 = new Position(1, 3);
          let p4 = new Position(1, 4);
          let p5 = new Position(1, 5);
          this.bodyParts = [p1, p2, p3, p4, p5];
          this.directionCurrently = "right";
          this.grown = false;
        }

        grow() {
          this.grown = true;
        }

        // direction can contain strings: left, up, right, down
        moveForward(direction) {
          let head = this.bodyParts[this.bodyParts.length-1]
          let newHead;

          if ((this.directionCurrently == "right" && direction != "left") ||
            (this.directionCurrently == "left" && direction != "right") ||
            (this.directionCurrently == "up" && direction != "down") ||
            (this.directionCurrently == "down" && direction != "up")) {
            this.directionCurrently = direction;
          }

          if (this.directionCurrently == "left") {
            newHead = new Position(head.x, head.y-1);
          } else if (this.directionCurrently == "up") {
            newHead = new Position(head.x-1, head.y);
          } else if (this.directionCurrently == "right") {
            newHead = new Position(head.x, head.y+1);
          } else if (this.directionCurrently == "down") {
            newHead = new Position(head.x+1, head.y);
          }

          if (this.grown) {
            this.grown = false;
          } else {
            this.bodyParts.shift();
          }

          this.bodyParts.push(newHead);
        }

        head() {
          return this.bodyParts[this.bodyParts.length-1]
        }
      }

      class Position {
        x;
        y;

        constructor(x, y) {
          this.x = x;
          this.y = y;
        }

        equals(position) {
          return this.x == x.position && this.y == y.position;
        }
      }

      /* functions */

      function randomInt(min, max) { // min and max included 
        return Math.floor(Math.random() * (max - min + 1) + min)
      }

      /* global variables */

      let gameBoard = new GameBoard(10, 20);
      let direction = "right"

      const LEFT_KEY_CODE = 37;
      const TOP_KEY_CODE = 38;
      const RIGHT_KEY_CODE = 39;
      const BOTTOM_KEY_CODE = 40;

      /* commands, event listeners and timed routines */

      // init html display
      var divArea = document.getElementById("table");
      gameBoard.table.initTableNode();
      divArea.append(gameBoard.table.tableNode);

      // start game
      let gameLoop = window.setInterval(function() {
        gameBoard.gameLoop();
        if (gameBoard.gameOver) {
          alert("Game over.");
          divArea.removeChild(gameBoard.table.tableNode)
          gameBoard = new GameBoard(10, 20);
          gameBoard.table.initTableNode();
          divArea.append(gameBoard.table.tableNode);
          direction = "right";
        }
      }, 500);

      // change direction by keyPress
      document.onkeydown = function (event) {
        if (event.keyCode == LEFT_KEY_CODE) {
          direction = "left";
        } else if (event.keyCode == TOP_KEY_CODE) {
          direction = "up";
              } else if (event.keyCode == RIGHT_KEY_CODE) {
          direction = "right";
              } else if (event.keyCode == BOTTOM_KEY_CODE) {
          direction = "down";
              }
      }
    </script>
</html>

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Overall, OOP feels a bit heavy for the size of the application but I'll approach the review without tossing out the design.

Clarify and tighten interfaces

One staple of OOP design is the concept of a public interface that provides clearly-labeled methods for the user to call to manipulate an object. On a physical device, these might be knobs or buttons. These controls should be clearly defined, intuitive to manipulate and shouldn't be able to put the object into an illegal state.

However, in your design, there's client code like:

gameBoard.table.initTableNode();
divArea.append(gameBoard.table.tableNode);

This accesses a property table on gameBoard to call a method on it, then dips through nested properties to access what should probably be an internal piece of data with gameBoard.table.tableNode. No getters or setters are used here, so these properties can be easily corrupted by an unwitting client. There's no clear interface--the client has to somehow know about all of these nested properties.

GameBoard is basically a namespace and not much more than that. It can't be refactored internally without breaking the contract with the calling code. This leads to tight coupling between client and class, something that OOP is supposed to help you avoid.

Due to the tight coupling, the client code and classes can't be broken out into separate files or modules, a next step if you were to expand this app.

Here, gameBoard.table.initTableNode(); could be an internal sub-step of a call gameBoard.initialize() that's exposed to the client.

divArea.append(gameBoard.table.tableNode); could provide access to the tableNode with a top-level getter directly on gameBoard. Alternately, divArea (the root element for the board to be injected into) into the gameBoard constructor.

Getters and setters can help clarify and tighten the public interfaces for your classes. You can use the _ prefix character to denote private data that isn't part of the interface. The rule of thumb is that as little should be exposed as possible: keeping Table as an implementation detail of GameBoard simplifies the client code by hiding complexity.

As an exercise, write out the public interface for each class in a comment. As a rule for the exercise, any chaining like foo.getInternalObject().callMethodOnInternalObject() (with or without methods) should be forbidden. Only use foo.takeActionOnThisObject().

Avoid globals

In keeping with the previous point, the Snake class is tightly coupled to the direction global variable. This makes it extremely difficult for a client to reuse the class--they would have to know to create a direction variable globally and change its value to a magic string "up", "down" (etc). This is a very brittle situation that's extremely hard to generalize and debug.

The rule I suggest applying is that the class is not allowed to access mutable variables outside of its scope (and a bare minimum of immutable variables as well, usually library dependencies, configuration and constants [although these should be class-scoped or constructor parameters when possible]).

The solution is the same as above: create class methods that allow you to change the snake's direction, then eliminate the shared global state and call those methods.

Stick with OOP throughout

Your code seemed very committed to OOP, but then sort of dropped the ball when it came to writing the global logic. The client still has too much work to do to use the GameBoard instance.

Imagine if you had to come up with all of this setup boilerplate to use a typical popular JS library. The holy grail for your top-level class should be:

const game = new SnakeGame(someRootDiv);
game.play();

or something like that, possibly with a configuration object if more options were available. This involves moving the game loop and key listeners into the game class.

If this seems to bake too much UI into the game, you could move the key listener out and use methods to inform the game of any keypresses.

Design your classes as typical black-box library calls you might use--assume the client/calling code has no idea (or need to know) how the internals of the class work and wants to have the simplest set of options available to achieve what they want.

Turn comments into methods

gameLoop is too long and has comments like // check if snake crashes into border. That's an easy refactor to a new method snake.checkBorderCollision(). Also, this method doesn't run a whole game loop, it just executes a single frame rerender. Name accordingly.

Other

  • Use a code formatter like prettier. Your CSS has random indentation as well.

  • Separate the .css and .js files. As mentioned earlier, separating the classes into separate files and modules will expose the dirty global practices for what they are and keep you honest.

  • Keep CSS out of your JS: .style.background = "white" should be refactored to toggle/add/remove CSS classes that have the actual style properties.

  • Follow W3 guidance on <!DOCTYPE html> and other important head tags to keep your HTML validated.

  • keyCode is deprecated; use code.

  • Use const instead of let except for loop counters and occasional variables you need to reassign, and avoid var unless there's some scoping need (unlikely) or legacy compatibility need (use a transpiler).

  • Prefer document.addEventListener("keydown", ...) over document.onkeydown = .

  • Try to avoid booleans like grown--usually, method return values are enough to report state changes without adding class variables. In this case, checkIfAppleIsEaten could return true/false.

  • Maybe make a class for Apple with a Position or x/y (be consistent) and a reposition() method that repositions the apple. The caller could check for a collision with the snake and repeat the reposition() call until valid, or the snake could be passed as a parameter.

  • Follow the single responsibility principle: checkIfAppleIsEaten does more than check that condition; it actually grows the snake and repositions the apple if the condition is true, complex and surprising side effects. The title should be complete and honest about what it does, growSnakeAndRepositionAppleIfEaten() (or checkAndHandleEatingApple()). Once you see a name this long, then the method is clearly overburdened and should be split into multiple steps to simplify its contract.

  • Use requestAnimationFrame() rather than setInterval.

  • Use arrow functions for callbacks unless you need this.

  • Position.equals(position) is unused. Be consistent about whether you want to use loose x/y pairs or Position objects.

  • Never use ==, always ===. == is broken and can introduce extremely subtle bugs.

  • I'd prefer randomInt to be exclusive at the end so it works with array indexing naturally and you won't have to subtract 1 from the second parameter when you call it.

  • this.bodyParts[this.bodyParts.length-1] can be this.bodyParts.at(-1) if supported or transpiled.

  • Maybe premature optimization, but keep in mind that allocating a new Position on every frame is a bit expensive. You might want to move the last position to the head and set its x and y to the new location.

  • Rather than all of the (this.directionCurrently == "right" && direction != "left") ... checks, use an object:

    const opposites = Object.freeze({
      up: "down",
      down: "up",
      left: "right",
      right: "left",
    });
    // ...
    if (direction !== opposites[this.currentDirection]) {
      // move is OK
    }
    

    To actually make the move, you can avoid if chains once again by pre-declaring an object that maps the directions to the positions:

    const steps = Object.freeze({
      up: [0, -1],
      down: [0, 1],
      left: [-1, 0],
      right: [1, 0],
    });
    // ...
    newHead = new Position(...steps[direction]);
    
  • alert("Game over."); is poor UX. Set text in an element instead.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.