3

Thanks for reading my question. I am currently taking a Java class on Coursera, and was asked to write a program on minesweeper for the assignment. My code creates the correct result, but my grade was deducted greatly because my code is ”excessively complex, with a cyclomatic complexity of 60“ according to the auto-grader. I understand that there are too many conditionals and loops, but I had a hard time trying to make it more simple.

Here is my code. It takes 3 integer command-line arguments m, n, and k to create an m-by-n grid with k mines in random locations. I use "5" to mark the mines instead of "" because the highest a number in a tile can get is 4 (since a tile has 4 sides). If two mines are located side by side, extra values might be added to its marker of "5". So I make all the values >= 5 become "" when I print them out. Each value is separated by two spaces.

public class Minesweeper {
  public static void main(String[] args) {
    int m = Integer.parseInt(args[0]);
    int n = Integer.parseInt(args[1]);
    int k = Integer.parseInt(args[2]);
    int[][] mine = new int[m][n];
    //put the mines
    for(int z = 0; z < k; z++) {
      int randomX = (int) (Math.random() * m);
      int randomY = (int) (Math.random() * n);
      mine[randomX][randomY] = 5; 
    }

    for(int y = 0; y < n; y++) {
      for(int x = 0; x < m; x++) {
        //first row of the grid
        if(y == 0) {
          //upper left corner
          if(x == 0) {
            if(mine[x + 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y + 1] >= 5) {
              mine[x][y] += 1;
            }
          }
          //upper right corner
          else if(x == m - 1) {
            if(mine[x - 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y + 1] >= 5) {
              mine[x][y] += 1;
            } 
          }
          //mid of first row
          else {
            if(mine[x - 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x + 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y + 1] >= 5) {
              mine[x][y] += 1;
            } 
          }
        }
        //mid rows
        else if(y > 0 && y < n - 1) {
          //left side
          if(x == 0) {
            if(mine[x][y - 1] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y + 1] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x + 1][y] >= 5) {
              mine[x][y] += 1;
            }
          }
          //right side
          else if(x == m - 1) {
            if(mine[x][y - 1] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y + 1] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x - 1][y] >= 5) {
              mine[x][y] += 1;
            }
          }
          //mid
          else {
            if(mine[x][y - 1] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y + 1] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x - 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x + 1][y] >= 5) {
              mine[x][y] += 1;
            }
          } 
        }
        //bottom row
        else if(y == n - 1) {
          //bottom left corner
          if(x == 0) {
            if(mine[x + 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y - 1] >= 5) {
              mine[x][y] += 1;
            }
          }
          //bottom right corner
          else if(x == m - 1) {
            if(mine[x - 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y - 1] >= 5) {
              mine[x][y] += 1;
            }
          }
          //middle of the bottom row
          else {
            if(mine[x + 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x - 1][y] >= 5) {
              mine[x][y] += 1;
            }
            if(mine[x][y - 1] >= 5) {
              mine[x][y] += 1;
            }
          }
        }
      }
    }
  //print out the grid
    for(int y = 0; y < n; y++) {
      for(int x = 0; x < m; x++) {
        //println at the right edge of the grid
        if(x == m - 1) {
          if(mine[x][y] >= 5) {
            System.out.println("*");
          }
          else {
            System.out.println(mine[x][y]);
          }
        }
        //other tiles, no need to switch lines
        else {
          if(mine[x][y] >= 5) {
            System.out.print("*  ");
          }
          else {
            System.out.print(mine[x][y] + "  ");
          }
        }
      } 
    }
  }
}

Thank you for your time, and I'd really appreciate any suggestions.

s.fuhrm
  • 438
  • 4
  • 9
NonMiPiaceJava
  • 31
  • 1
  • This is a well asked question, and I wish I had a better answer for you, but basically you need to think how you can make your code more generic and avoid hard coded numbers like 5. – Ian Newson Mar 01 '20 at 09:37
  • This is something for [code review](https://codereview.stackexchange.com/) not stackoverflow. Move the question there please. – akuzminykh Mar 01 '20 at 09:57

2 Answers2

0

You can reduce the complexity if you do the left/right/up/down testing in a loop to save a lot of lines of code:

for (int tryX = -1; tryX <= 1; tryX++) {
  for (int tryY = -1; tryY <= 1; tryY++) {
    if(mine[x + tryX][y + tryY] >= 5) {
      mine[x][y] += 1;
    }
  }
}

Since this is taking a lot of code lines, it will reduce the complexity. You should extract code to methods with your IDE (see here for IntelliJ). Good extraction points are loops.

I see two good extraction points:

  1. initArrayWithRandomMines()
  2. calculateNeighborMines()
s.fuhrm
  • 438
  • 4
  • 9
0

I won't share a refactored code, but give you some ideas and examples that you should do. I think it will be more useful for your learning.

You should find the same condition and extract them from several branches to one. I recommend you start from inner conditions and after got to outside.

Let's take an example the branch with y == 0 condition:

if(x == 0) {
  if(mine[x + 1][y] >= 5) {
     mine[x][y] += 1;
  }
  if(mine[x][y + 1] >= 5) {
    mine[x][y] += 1;
  }
}
//upper right corner
else if(x == m - 1) {
  if(mine[x - 1][y] >= 5) {
     mine[x][y] += 1;
  }
  if(mine[x][y + 1] >= 5) {
     mine[x][y] += 1;
  } 
//mid of first row
else {
  if(mine[x - 1][y] >= 5) {
    mine[x][y] += 1;
  }
  if(mine[x + 1][y] >= 5) {
    mine[x][y] += 1;
  }
  if(mine[x][y + 1] >= 5) {
    mine[x][y] += 1;
  } 
}

You can see that you check mine[x + 1][y] >= 5 when x == 0 or in else branch. You can merg two conditions in one and it will looks like x < m-1 and now code will look like:

if(x < m-1 && mine[x + 1][y] >= 5) {
  mine[x][y] += 1;
}

if(x == 0) {
  if(mine[x][y + 1] >= 5) {
    mine[x][y] += 1;
  }
}
//upper right corner
else if(x == m - 1) {
  if(mine[x - 1][y] >= 5) {
     mine[x][y] += 1;
  }
  if(mine[x][y + 1] >= 5) {
     mine[x][y] += 1;
  } 
//mid of first row
else {
  if(mine[x - 1][y] >= 5) {
    mine[x][y] += 1;
  }
  if(mine[x][y + 1] >= 5) {
    mine[x][y] += 1;
  } 
}

Continue with each if statement where do you use x in condition. when you finish with x, do the same actions with conditions with y. Extract the common parts from several branches.

And about part where you print the result. Please think about what you should do. You should print * or mine[x][y] depends on mine[x][y]==5 and print spaces or new_line (System.out.println()) depends on x == m - 1. Now think about how to implement it.

Maxim Popov
  • 1,167
  • 5
  • 9