7
\$\begingroup\$

I have a method which checks all of its surrounding squares and returns the number of bombs around it. But it's really long code and ugly, so can it be shorten?

final int MINE =10

for (int x = 0; x < counts.length; x++) {
    for (int y = 0; y < counts[0].length; y++) {
    if (counts[x][y] != MINE) {
    int Minesearch = 0;
    if (x > 0 && y > 0 && counts[x-1][y-1] == MINE) {//up left
        Minesearch++;
    }
    if (y > 0 && counts[x][y-1] == MINE) {//up
        Minesearch++;
    }
    if (x < counts.length - 1 && y > 0 && counts[x+1][y-1] == MINE) {//up right
        Minesearch++;
    }
    if (x > 0 && counts[x-1][y] == MINE) {//left
        Minesearch++;
    }
    if (x < counts.length - 1 && counts[x+1][y] == MINE) {//right
        Minesearch++;
    }
    if (x > 0 && y < counts[0].length - 1 && counts[x-1][y+1] == MINE) {//down left
        Minesearch++;
    }
    if (y < counts[0].length - 1 && counts[x][y+1] == MINE) {//down
        Minesearch++;
    }
    if (x < counts.length - 1 && y < counts[0].length - 1 && counts[x+1][y+1] == MINE) {//down right
        Minesearch++;
    }
    counts[x][y] = Minesearch;
    }
    }
}
}
\$\endgroup\$
  • \$\begingroup\$ What is MINE initialized to, or what is its value if it is a constant? \$\endgroup\$ – bob Aug 28 at 16:57
  • \$\begingroup\$ @bob final int MINE = 10; This is it. \$\endgroup\$ – Program beginner Aug 29 at 14:34
  • \$\begingroup\$ It might be helpful to include that info in the question. \$\endgroup\$ – bob Aug 31 at 13:05
  • 1
    \$\begingroup\$ @bob Thanks I'll add MINE initialized \$\endgroup\$ – Program beginner Sep 1 at 16:49
12
\$\begingroup\$

It can be shortened and "beautified" by refactoring the bounds cyhecking and mine checking into internal method:

private boolean isWithinBounds(int x, int y) {
    return x >= 0 && y >= 0 && x < width && y < height;
}

private boolean isMine(int x, int y) {
    return field[x][y] == MINE;
}

Then the mine count becomes trivial (we can assume that the center of the 3x3 square does not have a mine, otherwise the player would have exploded and ended the game):

for (int x1 = x - 1; x1 <= x + 1; x1++) {
    for (int y1 = y - 1; y1 <= y + 1; y1++) {
        if (isWithinBounds(x1, y1) && isMine(x1, y1) {
            mineCount++;
        }
    }
}

What we have done is breaking code into methods, each of which implement a small and well defined function. Because each method does exactly one thing, they become easier to understand, maintain and test.

You should check Java naming conventions. Variable names should be in camelCase, startingWithSmallLetter.

Variable and method names should describe the reason why the code exists. E.g. mineSearch is confusing as the variable does not search mines, it just keeps count of them. Thus mineCount is a better alternative.

Counts is also confusing as it contains a value named MINE which obviously is a marker for a cell containing mine but it also contains the surrounding mine counts. I did a minesweeper clone (or actually a minesweeper solver) once and I used an array containing Cell-objects. The Cell object provided methods for querying the status of the cell (stepped on, flagged, unknown) and the number of surrounding mines if it had been stepped on.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Why isWithinBounds(-1, -1) should return true? It will make isMine fail. The condition should be x >= 0 && y >= 0 && x < mines.length && y < mines[0].length. \$\endgroup\$ – Marc Aug 28 at 6:15
  • \$\begingroup\$ Good point @Marc. I had the condition exactly backwards! \$\endgroup\$ – TorbenPutkonen Aug 28 at 6:26
  • \$\begingroup\$ Also, I would normally write x >= 0 && x < width && y >= 0 && y < height. The same variable together. However, that's a really minor point. \$\endgroup\$ – Sulthan Aug 28 at 17:18
  • \$\begingroup\$ Why x1/y1? Also, that does check the current field, too, if I'm not mistaken. \$\endgroup\$ – Bobby Aug 28 at 20:05
  • \$\begingroup\$ @Bobby The assumption is that mine count is performed only after the user has stepped on the current field. If there is a mine, the game ends and a mine count is not needed. :) \$\endgroup\$ – TorbenPutkonen Aug 31 at 5:10
1
\$\begingroup\$

While the Answer of @TorbenPutkonen is correct it is a procedural approach to the problem.

There is nothing wrong with procedural approaches as such, but since Java is a object oriented language we might look out for OO-approaches instead...

I would extract the neighbor check into an enum like this:

enum Direction {
  NORTH{
     boolean isBomb(inx x, int y, boolean[] field){
       if(0 < x)
         return BOMB == field(x-1, y);
       else
         return false;
     }
  },
  NORTH_WEST{
     boolean isBomb(inx x, int y, boolean[] field){
       if(0 < x && 0 < y)
         return BOMB == field(x-1, y-1);
       else
         return false;
     }
  },
  SOUTH{
     boolean isBomb(inx x, int y, boolean[] field){
       if(field.length-1 > x)
         return BOMB == field(x+1, y);
       else
         return false;
     }
  },
  SOUTH_EAST{
     boolean isBomb(inx x, int y, boolean[] field){
       if(field.length-1 > x && field[0].length-1>y)
         return BOMB == field(x+1, y+1);
       else
         return false;
     }
  }
  // other directions following same pattern

  abstract boolean isBomb(inx x, int y, boolean[] field);
}

The benefit is that this enum could live in its own file and has a very limited responsibility. That means it is easy to understand what is does, isn't it?

In your calculation method you can simply iterate over the enum constants like this:

for (int x = 0; x < counts.length; x++) {
  for (int y = 0; y < counts[0].length; y++) {
    int mineCount =0;
    for(Direction direction : Direction.values()) {
       if (direction.isBomb(x, y, counts) ) {
            mineCount++;
       }
    }
  }
}

As a next step I'd apply the "tell, don't ask" principle by changing the method signature:

abstract int getBombValueOf(inx x, int y, boolean[] field);

the implementation in the enum would change like this:

     int getBombValueOf(inx x, int y, boolean[] field){
       if(0 < x && BOMB == field(x-1, y))
         return 1;
       else
         return 0;
     },

That could be simplified to the "elvis operator":

     int getBombValueOf(inx x, int y, boolean[] field){
       return (0 < x && BOMB == field(x-1, y))
         ? 1 
         : 0;
     },

and the usage would change to this:

for (int x = 0; x < counts.length; x++) {
  for (int y = 0; y < counts[0].length; y++) {
    int mineCount =0;
    for(Direction direction : Direction.values()) {
       mineCount += 
           direction.getBombValueOf(x, y, counts) );
    }
  }
}

We could achief the same (exept moving the neighbor calculation to another file) by using a FunctionalInterface and a simple collection:

@FunctionalInterface
interface Direction{
  int getBombValueOf(inx x, int y, boolean[] field);
}

private final Collection<Direction> directions = new HashSet<>();

// in constructor
   directions.add(new Direction() { // old style anonymous inner class
         int getBombValueOf(inx x, int y, boolean[] field){
           return (0 < x && BOMB == field(x-1, y))
             ? 1 
             : 0;           
         }
    };
   directions.add((x, y, field)-> { // Java8 Lambda 
           return (0 < x && 0 < y &&BOMB == field(x-1, y-1))
             ? 1 
             : 0;
    };
    // more following same pattern

// in your method
    for (int x = 0; x < counts.length; x++) {
      for (int y = 0; y < counts[0].length; y++) {
        int mineCount =0;
        for(Direction direction : directions) {
           mineCount += 
               direction.getBombValueOf(x, y, counts) );
        }
      }
    }

Of cause we could make much more benefit from OO principles if the game field would not be an array of primitives but a Collection of Objects. But that might be stuff for another answer... ;o)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Instead of copy-pasting the code in the enums, you could define an xy-offset for each direction and reuse the same code in each value. Personally I don't advocate for adding any functionality to enums. They're intended to be a tool for enumerating value sets and making them a container for code violates that intention and as such, always surprises the user. There are always better ways to implement the functionality. \$\endgroup\$ – TorbenPutkonen Aug 31 at 5:16
  • \$\begingroup\$ @TorbenPutkonen enums are full featured classes. Classes are used to provide different behavior. So I don't see any reason to use enums as dump constants only, especially if in turn they are used to introduce branching by use of a switch block. But I agree that the code in the enums could be cleaned a little more... And just vor the records: my second approach does what you suggest. \$\endgroup\$ – Timothy Truckle Sep 2 at 13:02

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.