current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I have written Minesweeper in JavaScript. Using OO-Approach.

Cell class describes a cell in the grid.

Cell.t property tells whether the cell is a mine or not.

Cell.count property stores the number of mines in the cell's vicinity.

Cell.state property stores whether the cell is used / unused / flagged.

Grid class provides an abstraction from the game's flow.

My main concerns are Grid.init and Grid.click functions.

Grid.init assigns the number of mines in a cell's vicinity to Cell.count.

Grid.click recursively clears out the neighbor cells (without mines) when a cell is clicked.

The conditions in Grid.init and Grid.click seem quite redundant. Is there any way to fix that? Is there any other change that I should make to improve the code.

function Cell(type) {

  if (!(this instanceof Cell)) {
    throw new Error("Invalid call to constructor");
  }
  this.t = type;
  this.count = 0;
  this.state = 1;
}

function Grid(length, width, mines) {

  var i, j, rc, rr;
  if (!(this instanceof Grid)) {
    throw new Error("Invalid call to constructor");
  }
  this.l = length;
  this.w = width;
  this.m = mines;
  this.grid = [];
  for (i = 0; i < this.l; i += 1) {
    this.grid.push([]);
    for (j = 0; j < this.w; j += 1) {
      this.grid[i].push(new Cell("n"));
    }
  }

  for (i = 0; i < this.m; i += 1) {
    rr = parseInt(Math.random() * this.l);
    rc = parseInt(Math.random() * this.w);
    if (this.grid[rr][rc].t === "n") {
      this.grid[rr][rc].t = "m";
    } else {
      i -= 1;
    }
  }
}

Grid.prototype.init = function() {

  var i, j, n;

  for (i = 0; i < this.l; i += 1) {
    for (j = 0; j < this.w; j += 1) {
      n = 0;
      if (i - 1 >= 0 && j - 1 >= 0 && this.grid[i - 1][j - 1].t === "m") {
        n += 1;
      }
      if (j - 1 >= 0 && this.grid[i][j - 1].t === "m") {
        n += 1;
      }
      if (i + 1 < this.l && j - 1 >= 0 && this.grid[i + 1][j - 1].t === "m") {
        n += 1;
      }
      if (i - 1 >= 0 && this.grid[i - 1][j].t === "m") {
        n += 1;
      }
      if (i + 1 < this.l && this.grid[i + 1][j].t === "m") {
        n += 1;
      }
      if (i - 1 >= 0 && j + 1 < this.w && this.grid[i - 1][j + 1].t === "m") {
        n += 1;
      }
      if (j + 1 < this.w && this.grid[i][j + 1].t === "m") {
        n += 1;
      }
      if (i + 1 < this.l && j + 1 < this.w && this.grid[i + 1][j + 1].t === "m") {
        n += 1;
      }
      this.grid[i][j].count = n;
    }
  }
};

Grid.prototype.click = function(i, j) {


  this.grid[i][j].state = 0;

  if (this.grid[i][j].t === "m") {
    return 0;
  }

  if (this.grid[i][j].count === 0) {
    if (i - 1 >= 0 && j - 1 >= 0 && this.grid[i - 1][j - 1].state === 1) {
      this.click(i - 1, j - 1);
    }
    if (j - 1 >= 0 && this.grid[i][j - 1].state === 1) {
      this.click(i, j - 1);
    }
    if (i + 1 < this.l && j - 1 >= 0 && this.grid[i + 1][j - 1].state === 1) {
      this.click(i + 1, j - 1);
    }
    if (i - 1 >= 0 && this.grid[i - 1][j].state === 1) {
      this.click(i - 1, j);
    }
    if (i + 1 < this.l && this.grid[i + 1][j].state === 1) {
      this.click(i + 1, j);
    }
    if (i - 1 >= 0 && j + 1 < this.w && this.grid[i - 1][j + 1].state === 1) {
      this.click(i - 1, j + 1);
    }
    if (j + 1 < this.w && this.grid[i][j + 1].state === 1) {
      this.click(i, j + 1);
    }
    if (i + 1 < this.l && j + 1 < this.w && this.grid[i + 1][j + 1].state === 1) {
      this.click(i + 1, j + 1);
    }
  }
  return this.check();
};

Grid.prototype.check = function() {

  var i, j;

  for (i = 0; i < this.l; i += 1) {
    for (j = 0; j < this.w; j += 1) {
      if (this.grid[i][j].t === "n" && this.grid[i][j].state) {
        return 1;
      }
    }
  }
  return 2;
};

Note: I haven't provided the code which links these functions as event handlers to forms controls.

share|improve this question

1 Answer 1

up vote 1 down vote

I think your algorithms are OK. But there is room for improvement:

Naming

One letter variable names are seldom useful. If you have to explain what Cell.t means, it isn't clear enough, it should be Cell.type. The same goes for Grid.l, Grid.w and Grid.m.

Also, the limited values for the variables should also be meaningful:

  • I'm guessing "n" is "nothing" Or you could pass and "m" is "mine" but is a lot better to spell it out.
  • The same goes for the states
  • And I still don't understand what the 2 returned by check() means.

JavaScript doesn't have enums, you can use constants, but a full string (like "mine" instead of "m") should improve the code a lot (it still will be difficult to change in the future).

Setting the mines

The current algorithm has an infinite worst case: if the cell randomly chosen keeps hitting a mine, it wouldn't end.

I think a good alternative could be to generate an array of length * height with as many trues (or another meaningful value) as mines, shuffle that array, and then use that as the grid.

I don't know if you can shuffle a bidimensional array, but shuffling a one-dimension array and then converting to 2-dimension should be easy.

Neighbors

In order to simplify the init() and click() functions, you could make a neighbors() function in Grid and then iterate over these. Something like this:

Grid.prototype.neighbors = function(i, j) {
  neighbors = []
  if (i - 1 >= 0 && j -1 >= 0) {
    neighbors.push(this.grid[i - 1][j - 1]);
  }
  ...
  return neighbors;
}

And then, in init():

n = 0;
for (neighbor in this.neighbors(i, j)) {
  if (neighbor.type == "mine") {
    n++;
  }
}
this.grid[i][j].count = n;

And in click():

for (neighbor in this.neighbors(i, j)) {
  if (neighbor.state === 1) {
    this.click(neighbor.x, neighbor.y);
  }
}
share|improve this answer
    
Grid.click() returns 0 if the user has clicked on a mine, otherwise, it returns Grid.check() which returns 1 if user has not clicked on all the non-mine cells or 2 if user has clicked on the non-mine cells (has won the game). –  ShuklaSannidhya Jun 17 at 7:59
    
How do I implement Grid.click() using Grid.neighbors() (the one you mentioned)? Since, Grid.click() needs to be called with the index of the cell in the grid, the cell needs to be aware of its index. I have implemented it here. Note that, now the Cell class has stores the index in Cell.x and Cell.y. –  ShuklaSannidhya Jun 17 at 8:07
    
@ShuklaSannidhya I've added an example for click(). –  Pablo González Alba Jun 17 at 15:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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