Android App Interview (Minesweeper game)

I recently did an Android Minesweeper app for a pre-interview coding challenge before I could talk to any engineer and was rejected. This feedback was given but I'm not sure on some of it.

• ☺︎ Game works

• Very minimal implementation (well, I did what was required...)

• No separation of logic, entire game crammed into GameActivity (not sure what they want)

• ☺︎ Concise code

• Game logic isn't separated from view classes (not sure what they want)

• Flood-fill when you click on an empty square doesn't quite work right

• Doesn't make the best use of Android features (e.g. GridView) (seems debatable that it is the 'best' to me since I used TableLayout for the rows instead of GridView)

My code is at here.

The most relevant code is here:

```
import android.app.Activity;
import android.app.AlertDialog;
import android.content.Context;
import android.content.DialogInterface;
import android.content.Intent;
import android.graphics.Color;
import android.os.Bundle;
import android.util.AttributeSet;
import android.view.Menu;
import android.view.MenuInflater;
import android.view.MenuItem;
import android.view.View;
import android.widget.*;
import java.util.Random;
public class GameActivity extends Activity {
private TableLayout mTableLayout;
private ImageButton mValidateButton;
private static final int mineCount = 10;
private final Tile[][] mData = new Tile[8][8]; //8x8 grid
public class Tile extends Button
{
private boolean isMine;
private boolean isFlag;
private boolean isCovered;
private int noSurroundingMines;
public Tile(Context context)
{
super(context);
}
public Tile(Context context, AttributeSet attrs)
{
super(context, attrs);
}
public Tile(Context context, AttributeSet attrs, int defStyle)
{
super(context, attrs, defStyle);
}
public void setDefaults()
{
isMine = false;
isFlag = false;
isCovered = true;
noSurroundingMines = 0;
this.setBackgroundResource(R.drawable.til

I don't know Android, but since nobody who does has posted a review yet and I can see some issues, I'll take a stab at it.

Sometimes you use this curly brace style:

stuff {
    inner stuff;
}

and sometimes this style:

stuff
{
    inner stuff;
}

Be consistent.

There are lots of harcoded 8s for both the width and the height. If they suddenly told you to make your code work for a 9x17 board instead of an 8x8, you should be able to do it by changing exactly 2 numbers (with the possible exception of graphical stuff like fitting on a screen).

noSurroundingMines is a bad name. It sounds like a boolean, but it is an int. I'd suggest nearbyMines. You use mData[0].length and mData.length for width and height. Since the size of the game board won't change in the middle of a game, I'd suggest width and height. mData is also a bad name, I'd suggest board or grid.

Some of the comments are things that are obvious from reading the code, or would be obvious with better naming, while there are other places where the code isn't clear and there aren't any comments to help. It's better the other way around.

This is ugly:

switch (noSurroundingMines)
  {
    case 0:
       this.setBackgroundResource(R.drawable.sq0);
       break;
    case 1:
       this.setBackgroundResource(R.drawable.sq1);
       break;
    case 2:
       this.setBackgroundResource(R.drawable.sq2);
       break;
    case 3:
       this.setBackgroundResource(R.drawable.sq3);
       break;
    case 4:
       this.setBackgroundResource(R.drawable.sq4);
       break;
    case 5:
       this.setBackgroundResource(R.drawable.sq5);
       break;
    case 6:
       this.setBackgroundResource(R.drawable.sq6);
       break;
    case 7:
      this.setBackgroundResource(R.drawable.sq7);
      break;
    case 8:
      this.setBackgroundResource(R.drawable.sq8);
      break;
  }

Define the appropriate array, and it becomes this:

this.setBackgroundResource(arr[nearbyMines]);

although you'd want to come up with a better name than arr.

This seems rather convoluted:

// count number of mines in surrounding blocks
    for (int row = 0; row = 0 && ii = 0 && jj <8) {
                            if (mData[ii][jj].isMine()) mData[row][column].updateSurroundingNumber();
                        }
                    }
                }
            }
        }

It's part of the initialization of the board, and you're counting the number of mines near a space. The inner 2 loops only have 3 iterations each, so it's not as inefficient as it seems at first glance, but the inner loops and the outer loops aren't logically related, so you could put the inner loops in a separate method, turning the inner portion into just countNearbyMines(row, column);, and making the whole chunk of code more readable.

Also, the hardcoded 8s should be width and height, and there doesn't seem to be any reason to use ii and jj instead of i and j for counters.

Later on, I make a suggestion for a bounds checking function, which would also simplify this code.

Simplified code block:

// count number of mines in surrounding blocks
    for (int row = 0; row < height; row++) {
        for (int column = 0; column < width; column++) {
                countNearbyMines(row, column);
        }
    }

Body of countNearbyMines:

for (int i = row - 1; i <= row + 1; i++) {
    for (int j = column - 1; j <= column + 1; j++) {
        if (inBounds(i, j)) {
            if (grid[i][j].isMine()) 
                grid[row][column].updateSurroundingNumber();
        }
    }
}

In this code and at least one other spot, you have nested ifs with the outer if testing whether coordinates are in bounds. IIRC, you could use something like inBounds(i, j) && grid[i][j].isMine() and rely on the short-circuiting of &&, turning it into a single if.

Earlier in the same method, you have this:

int randomRow = rnd.nextInt(mData.length);
        int randomCol = rnd.nextInt(mData[0].length);
        //if randomRow/Col happens to already have a mine,
        //continue looking for another spot to fill
        if (!mData[randomRow][randomCol].isMine())
        {

            mData[randomRow][randomCol].setMine(true);
            mData[randomRow][randomCol].setTag(R.id.mine, true);
        } else {  //if it is already a mine, do random until you find an empty spot
            while (mData[randomRow][randomCol].isMine())
            {
                randomRow = rnd.nextInt(mData.length);
                randomCol = rnd.nextInt(mData[0].length);

            }
            //set mine
            mData[randomRow][randomCol].setMine(true);
            mData[randomRow][randomCol].setTag(R.id.mine, true);

        }

Since a while loop won't execute if its condition is false, we don't actually need the if statement here. I think randomRow and randomCol are a bit too verbose, since they're local

StackExchange Code Review Q#84849, answer score: 3