6
\$\begingroup\$

I just wrote a Tetris game and I'd like to humbly request for code review especially in terms of code transparency, code efficiency (whether something can be coded in faster way using less resources) and bad coding habits. The game is complete and working and hopefully without any serious bugs (at least at this point I'm not aware of any). The game for display uses HTML DOM table, which I realise is very inefficient, but I wrote this only for learning purpose. In code I used ECMAScript 6 classes to code blocks and all their functions (I copied code for that class).

jsFiddle

You can rotate block with "space" key.

class Block {
  constructor(blockType) {
    var that = this;
    this.trace = [];
    this.blockType = blockType;
    this.isSettled = 'false';
    this.color;

    this.init();

    document.addEventListener('keydown', function(ev) {
      ev.preventDefault();
      switch (ev.which) {
        case 37:
          if (that.isSettled != true && that.validateLeftMovement(that) != 'blocked') {
            for (var i = 0; i < that.trace.length; i++) {
              pointColor(that.trace[i].x, that.trace[i].y, 'transparent');
            }
            for (var i = 0; i < that.trace.length; i++) {
              that.trace[i].y--;
            }
            for (var i = 0; i < that.trace.length; i++) {
              pointColor(that.trace[i].x, that.trace[i].y, that.color);

              if(cells[that.trace[i].x + 1][that.trace[i].y].hasBlock == true){
                that.isSettled = true;
              }
            }
          }
          break;

        case 39:
          if (that.isSettled != true && that.validateRightMovement(that) != 'blocked') {
            for (var i = 0; i < that.trace.length; i++) {
              pointColor(that.trace[i].x, that.trace[i].y, 'transparent');
            }
            for (var i = 0; i < that.trace.length; i++) {
              that.trace[i].y++;
            }
            for (var i = 0; i < that.trace.length; i++) {
              pointColor(that.trace[i].x, that.trace[i].y, that.color);

              if(cells[that.trace[i].x + 1][that.trace[i].y].hasBlock == true){
                that.isSettled = true;
              }
            }
          }
          break;

        case 32:
          that.rotate(that);
          break;

      }
    }, true);

    that.move(that)
  }

  init() {
    var start;
    switch (this.blockType) {
      case 't':
        this.color = 'purple';
        start = rand(0, boardWidth - 3);
        for (var i = 0; i < 2; i++) {
          for (var j = 0; j < 3; j++) {
            if (i == 1 && (j == 0 || j == 2)) {
              continue;
            } else {
              this.trace.push({
                x: i,
                y: start + j
              });
              pointColor(i, start + j, this.color);
            }
          }
        }
        break;

      case 'square':
        this.color = 'red';
        start = rand(0, boardWidth - 2);
        for (var i = 0; i < 2; i++) {
          for (var j = 0; j < 2; j++) {
            this.trace.push({
              x: i,
              y: start + j
            });
            pointColor(i, start + j, this.color);
          }
        }
        break;

      case 'el':
        this.color = 'blue';
        start = rand(0, boardWidth - 3);
        for (var i = 0; i < 3; i++) {
          if (i > 1) {
            continue;
          } else {
            for (var j = 0; j < 3; j++) {
              if (i == 1 && j != 0) {
                continue;
              } else {
                this.trace.push({
                  x: i,
                  y: start + j
                });
                pointColor(i, start + j, this.color);
              }
            }
          }
        }
        break;

      case 'stair':
        this.color = 'green';
        start = rand(0, boardWidth - 3);
        for (var i = 0; i < 3; i++) {
          if (i > 1) {
            continue;
          } else {
            for (var j = 0; j < 3; j++) {
              if ((i == 0 && (j == 0 || j == 3)) || (i == 1 && (j > 1))) {
                continue;
              } else {
                this.trace.push({
                  x: i,
                  y: start + j
                });
                pointColor(i, start + j, this.color);
              }
            }
          }
        }
        break;

      case 'line':
        this.color = 'yellow';
        start = rand(0, boardWidth - 4);
        for (var j = 0; j < 4; j++) {
          this.trace.push({
            x: 0,
            y: start + j
          });
          pointColor(0, start + j, this.color);
        }
        break;
    }
  }

  isSettledValidate(that) {
    for (var i = 0; i < that.trace.length; i++) {
      if (cells[that.trace[i].x + 1][that.trace[i].y].hasBlock == true || that.trace[i].x + 1 == boardHeight - 1) {
        return 'settled';
      }
    }
  }

  validateLeftMovement(that) {
    for (var i = 0; i < that.trace.length; i++) {
      if (that.trace[i].y - 1 < 0 || cells[that.trace[i].x][that.trace[i].y - 1].hasBlock == true) {
        return 'blocked';
      }
    }
  }

  validateRightMovement(that) {
    for (var i = 0; i < that.trace.length; i++) {
      if (that.trace[i].y + 1 > boardWidth - 1 || cells[that.trace[i].x][that.trace[i].y + 1].hasBlock == true) {
        return 'blocked';
      }
    }
  }

  validateRowFilled() {
    var wasRowErased = 0;

    for (var i = boardHeight - 2; i >= 0; i--) {
      for (var j = 0; j < boardWidth; j++) {
        if (cells[i][j].hasBlock == false) {
          break;
        } else if (j == boardWidth - 1 && cells[i][boardWidth - 1].hasBlock == true) {
          eraseRow(i);
          wasRowErased++;
          i++;
        }
      }
    }

    updateScore();

    function updateScore(){
      score = score + Math.pow(wasRowErased, 2) * 10;
      domScore.innerHTML = score;
    }

    function eraseRow(row) {
      for (var j = 0; j < boardWidth; j++) {
        cells[row][j].hasBlock = false;
        pointColor(row, j, 'transparent');
      }

      collapseRow(row);
    }

    function collapseRow(row) {
      for (var i = row; i >= 1; i--) {
        for (var j = 0; j < boardWidth; j++) {
          cells[i][j].hasBlock = cells[i - 1][j].hasBlock;

          if (cells[i][j].hasBlock == true) {
            pointColor(i, j, document.getElementById((i - 1) + 'x' + j).style.backgroundColor);
          } else {
            pointColor(i, j, 'transparent');
          }
        }
      }
    }
  }

  rotate(that) {
    var centerX,
      centerY,
      tmpX,
      tmpY;

    if (that.blockType == 'line' || that.blockType == 'stair' || that.blockType == 't') {
      centerX = that.trace[1].x;
      centerY = that.trace[1].y;
    } else if (that.blockType == 'el') {
      centerX = that.trace[0].x;
      centerY = that.trace[0].y;
    }

    if (that.isSettled == 'false' && isRotationAllowed() != false) {
      for (var i = 0; i < that.trace.length; i++) {
        pointColor(that.trace[i].x, that.trace[i].y, 'transparent');

        that.trace[i].x -= centerX;
        that.trace[i].y -= centerY;
        tmpX = that.trace[i].x;
        tmpY = that.trace[i].y;

        tmpX = -1 * (that.trace[i].y);
        tmpY = that.trace[i].x;

        that.trace[i].x = tmpX + centerX;
        that.trace[i].y = tmpY + centerY;

        pointColor(that.trace[i].x, that.trace[i].y, that.color);

      }
    }

    function isRotationAllowed() {
      var centerX,
        centerY,
        checkX,
        checkY,
        tmpX,
        tmpY;

      if (that.blockType == 'line' || that.blockType == 'stair' || that.blockType == 't') {
        centerX = that.trace[1].x;
        centerY = that.trace[1].y;
      } else if (that.blockType == 'el') {
        centerX = that.trace[0].x;
        centerY = that.trace[0].y;
      }

      for (var i = 0; i < that.trace.length; i++) {
        checkX = that.trace[i].x;
        checkY = that.trace[i].y;

        checkX -= centerX;
        checkY -= centerY;

        tmpX = checkX;
        tmpY = checkY;

        tmpX = -1 * checkY;
        tmpY = checkX;

        checkX = tmpX + centerX;
        checkY = tmpY + centerY;

        if (checkX < 0 || checkX > boardHeight - 3 || checkY < 0 || checkY > boardWidth - 1 || cells[checkX][checkY].hasBlock == true) {
          return false;
        }
      }
    }
  }

  move(that) {

    for(var i=0; i<that.trace.length; i++){
      if(cells[that.trace[i].x][that.trace[i].y].hasBlock == true){
        domReset.removeAttribute('disabled');
        return;
      }      
    }

    for (var i = 0; i < that.trace.length; i++) {
      pointColor(that.trace[i].x, that.trace[i].y, 'transparent');
    }
    for (var i = 0; i < that.trace.length; i++) {
      that.trace[i].x++;
    }
    for (var i = 0; i < that.trace.length; i++) {
      pointColor(that.trace[i].x, that.trace[i].y, that.color);
    }

    if (that.isSettledValidate(that) != 'settled') {
      setTimeout(function() {
        that.move(that)
      }, speed);
    } else {
      setTimeout(function() {
        that.isSettled = true;
        for (var i = 0; i < that.trace.length; i++) {
          cells[that.trace[i].x][that.trace[i].y].hasBlock = true;
        }

        that.validateRowFilled();
        generateRandomBlock();
      }, 50);
    }
  }
}
\$\endgroup\$
  • \$\begingroup\$ I love your questions, I am still working on an answer for your minesweeper. \$\endgroup\$ – konijn Aug 1 '16 at 12:44
  • \$\begingroup\$ @konijn lmao ^.^- I sure do too \$\endgroup\$ – Siobhan Aug 1 '16 at 13:00
  • \$\begingroup\$ Was my minesweeper that bad? :) But anyway, all answers are welcome! \$\endgroup\$ – Furman Aug 1 '16 at 15:29
  • \$\begingroup\$ I can't even change the change the orientation of the shape neither can the game be stopped once it has started :-( how sad. \$\endgroup\$ – Siobhan Aug 1 '16 at 22:58
  • \$\begingroup\$ Oh sorry forgot to mention, you can rotate block with space key. And indeed so far game can't be stopped once it has started, I'll try to update it soon. \$\endgroup\$ – Furman Aug 2 '16 at 4:53
1
\$\begingroup\$
  • Naming: I get that that.isSettled looks cute, but that means nothing. If you really want to have some resemblance to the english language, you could try to name the variable thePiece and the function hasSettled. Also, validateLeftMovement could become thePiece.canGoLeft(), which then could also return a simple boolean instead of a string.

37, 32 and 32 are not exactly readable, consider using constants.

  • Functions: choosing a random element should not require that much code,

and actually it doesn't:

var items = ['square', 'el', 'stair', 'line', 't']

function generateRandomBlock() {
  var blockType = items[Math.floor(Math.random()*items.length)]

  new Block(blockType);
}

Or you could shuffle the array and get the first element. This should be slightly faster, but with only 5 elements it shouldn't really matter.

  • I think the switch blocks should be as short as possible, so you should probably move all of the code in separate functions.

Something like:

document.addEventListener('keydown', function(ev) {
  ev.preventDefault();
  switch (ev.which) {
    case LEFT_KEY:
        moveLeft();
        break;
    case RIGHT_KEY:
        moveRight();
        break;
    case SPACE_KEY:
        rotate();
        break;
  • There is actually no reason for you to give an argument to validateLeftMovement (or validateRightMovement). No other parts of your code are calling that function, so you can just remove it and use this.

  • Various: You are setting the properties of the blocks based on a big switch, while you could use a hash for that, for example having var colors = {'line': 'yellow', 'square': 'red'} and so on, so that you don't even need to assign this.color, you can just reference colors[this.blockType].

  • Loops and "building the blocks".

This

for (var i = 0; i < 3; i++) {
  if (i > 1) {
    continue;
  } else {
    for (var j = 0; j < 3; j++) {
      if (i == 1 && j != 0) {
        continue;
      } else {
        this.trace.push({
          x: i,
          y: start + j
        });
        pointColor(i, start + j, this.color);
      }
    }
  }
}

Well, it works. But if you think about it, every piece has a maximum 4x4 matrix. Wouldn't it be better to have a hash with a two dimensional 4x4 array telling you the shape of the piece? And you could combine this with the other properties, like color.

Something like:

var pieces = {
    'line': {
        'blocks': [[1, 0, 0, 0],[1, 0, 0, 0],[1, 0, 0, 0],[1, 0, 0, 0]],
        'color': 'yellow'
    },
    'square': {
        'blocks': [[1, 1, 0, 0],[1, 1, 0, 0],[1, 1, 0, 0],[1, 1, 0, 0]],
        'color': 'blue'
    } 
}

Then you can simply iterate over pieces[blockType].blocks and use pieces[blockType].color. This would get rid of the big switch and all those for loops.

| improve this answer | |
\$\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.