Android App Interview (Minesweeper game)

Question

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.tile);
   }
   public void setMine(boolean mine)
   {
      this.isMine = mine;
      if (isMine && !isCovered) //only show mine if isn't
      {
        this.setBackgroundResource(R.drawable.mine);
      }
   }
   public void setFlag(boolean flag)
   {
      this.isFlag = flag;
      if (flag)
      {
     this.setBackgroundResource(R.drawable.flag);
      }
   }
   public void setUncovered()
   {
      this.isCovered = false;
      if (isMine())
      {
        this.setBackgroundResource(R.drawable.mine);
      } else if (noSurroundingMines > -1) {
      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;
    }
  }
}
public void updateSurroundingNumber() {
     this.noSurroundingMines++;
}
public void setSurroundingNumber(int number)
{
      this.noSurroundingMines = number;
}
public boolean isMine()
{
      return this.isMine;
}
public boolean isFlag()
{
      return this.isFlag;
}
public int getNoSurroundingMines()
{
      return noSurroundingMines;
}
}
/** Called when the activity is first created. */
@Override
public void onCreate(Bundle bundle) {
      super.onCreate(bundle);
      setContentView(R.layout.lib_game);
      mValidateButton = (ImageButton) findViewById(R.id.validate);
      mValidateButton.setOnClickListener(new View.OnClickListener() {
              @Override
              public void onClick(View v) {
                  //check if win. if not, reset game
                 if (!checkGameFinished()) {
                  //reset game
                 mValidateButton.setBackgroundResource(R.drawable.normal_smiley);
                 initTiles();
                mTableLayout.removeAllViews();
                initTable();
                initMineField(mineCount);
             } else {
                v.setBackgroundResource(R.drawable.win_smiley);
                setUncoveredMinesToFlags();
                setWinState();
             }
        }
});
      mValidateButton.setBackgroundResource(R.drawable.normal_smiley);
      mTableLayout = (TableLayout) findViewById(R.id.game_view);
      mTableLayout.setShrinkAllColumns(true);
      initTiles();
      initTable();
      initMineField(mineCount);
}

private void initTiles()
{
      for (int i = 0; i < mData.length; i++) {
          for (int j = 0; j < mData[0].length; j++) {
             Tile tile = new Tile(this);
             tile.setDefaults();
             mData[i][j] = tile;
          }
      }
}

private void initTable()
{
       //setting up rows for tablelayout
       for (int i = 0; i < mData.length; i++)
       {
          TableRow row = new TableRow(this);
          for (int button = 0; button < mData[0].length; button++) {
               // and you have to add them to the TableRow
            mData[i][button].setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
            //check if you won before anything incl. setting adjacent elements
               if (v.getTag(R.id.mine) != null)
                {
                   Object tag = v.getTag(R.id.mine);
                   if ((Boolean)tag)
                   {
                     //you clicked on a mine, so you lost
                      v.setBackgroundColor(Color.RED);
                      v.setBackgroundResource(R.drawable.mine);
                       mValidateButton.setBackgroundResource(R.drawable.lose_smiley);
                     //display all other mines
                     uncoverAllMines();
                 }
              }
rippleUncover((Integer)v.getTag(R.id.x), (Integer)v.getTag(R.id.y));
checkGameFinished();
}
});
mData[i][button].setTag(R.id.x, i);
mData[i][button].setTag(R.id.y, button);
row.addView(mData[i][button]);
}
// a new row has been constructed -> add to table
mTableLayout.addView(row);
}
}

private void rippleUncover(int rowClicked, int columnClicked)
{
// don't open mined rows
if (mData[rowClicked][columnClicked].isMine())
{
return;
}
// open clicked block
mData[rowClicked][columnClicked].setUncovered();
// if clicked block have nearby mines then don't open further
if (mData[rowClicked][columnClicked].getNoSurroundingMines() != 0 )
{
return;
}
// open next 3 rows and 3 columns recursively
for (int row = 0; row < 3; row++)
{
for (int column = 0; column < 3; column++)
{
// check all the above checked conditions
// if met then open subsequent blocks
if ((rowClicked + row - 1 >= 0) && (rowClicked + row - 1 < 8) && (columnClicked + column - 1 >= 0) &&
columnClicked + column - 1 < 8) {
if (mData[rowClicked + row - 1][columnClicked + column - 1].isCovered
&& (rowClicked + row - 1 > 0) && (columnClicked + column - 1 > 0)
&& (rowClicked + row - 1 < 9) && (columnClicked + column - 1 < 9)) {
rippleUncover(rowClicked + row - 1, columnClicked + column - 1);
}
}
}
}
}

public void uncoverAllMines()
{
      for (int i = 0; i < mData.length; i++)
      {
         for (int j = 0; j < mData[0].length; j++)
          {
             if (mData[i][j].isMine())
             {
               mData[i][j].setBackgroundResource(R.drawable.mine);
               mData[i][j].setUncovered();
             }
          }
      }
}
public void initMineField(int mines)
{
Random rnd = new Random();
for (int i = 0; i < mines; i++)
{
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);
}
}
// count number of mines in surrounding blocks
for (int row = 0; row < 8; row++) {
for (int column = 0; column < 8; column++) {
// check in all nearby blocks
for (int ii = row - 1; ii <= row + 1; ii++)
{
      for (int jj = column - 1; jj <= column + 1; jj++)
       {
          if (ii >= 0 && ii < 8 && jj >= 0 && jj <8) {
            if (mData[ii][jj].isMine()) mData[row][column].updateSurroundingNumber();
            }
          }
       }
    }
  }
}

public void setUncoveredMinesToFlags()
{
       for (int i = 0; i < mData.length; i++)
       {
           for (int j = 0; j < mData[0].length; j++) {
                if (mData[i][j].isMine()) {
                   mData[i][j].setFlag(true);
                   mData[i][j].setBackgroundResource(R.drawable.flag);
                }
            }
       }
}

@Override
public boolean onCreateOptionsMenu(Menu menu) {
         MenuInflater inflater = getMenuInflater();
         inflater.inflate(R.menu.game_activity_options, menu);
         return true;
}

@Override
protected void onResume() {
     super.onResume();
}

/*
Returns void and reveals/uncovers all mines, while not ending the game.
*/
public void revealMines()
{
       for (int i = 0; i < mData.length; i++)
        {
            for (int j = 0; j < mData[0].length; j++)
            {
                if (mData[i][j].isMine())
                {
                   mData[i][j].setBackgroundResource(R.drawable.mine);
                   mData[i][j].setUncovered();
                }
            }
      }
}

@Override
public boolean onOptionsItemSelected(MenuItem item) {
       // Handle item selection
       switch (item.getItemId()) {
           case R.id.action_cheat:
               revealMines();
               return true;
            default:
               return super.onOptionsItemSelected(item);
       }
}

public boolean checkGameFinished() {
      // check grid and see if all squares are uncovered
      for (int i = 0; i < mData.length; i++) {
          for (int j = 0; j < mData[0].length; j++ ) {
             if (mData[i][j].isCovered)
             {
                return false;
             }
          }
       }
setWinState();
return true;
}

private void setWinState() {
         AlertDialog.Builder builder = new AlertDialog.Builder(this);
         builder.setMessage(R.string.player1_win)
         .setPositiveButton(R.string.ok, new DialogInterface.OnClickListener() {
public void onClick(DialogInterface dialog, int id) {
//go back to main activity
Intent intent = new Intent(getApplicationContext(), MainActivity.class);
startActivity(intent);
}
})
.setNegativeButton(R.string.cancel, new DialogInterface.OnClickListener() {
public void onClick(DialogInterface dialog, int id) {
// User cancelled the dialog, so do nothing
}
});
// Create the AlertDialog object and show it
builder.create().show();
}
}

The recruiter said that they had no Android engineers currently but they had some people that "knew it" who reviewed my code. It seems kind of sketchy. The company would not consider any other samples I had on my resume either except their coding challenge (yes, one of those companies).

Can someone please review my code and give pointers?

Answer 1

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 < 8; row++) {
        for (int column = 0; column < 8; column++) {

                // check in all nearby blocks
                for (int ii = row - 1; ii <= row + 1; ii++)
                {
                    for (int jj = column - 1; jj <= column + 1; jj++)
                    {
                        if (ii >= 0 && ii < 8 && jj >= 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 locals and we're calling rnd.nextInt() right here. Making the suggested changes, we get this:

int row = rnd.nextInt(height);
int col = rnd.nextInt(width);

while (grid[row][col].isMine())
{
    row = rnd.nextInt(height);
    col = rnd.nextInt(width);
}

//set mine
grid[row][col].setMine(true);
grid[row][col].setTag(R.id.mine, true);

I don't know what setTag() is, but probably what you want here is to do that in setMine() too, turning the last 3 lines into just grid[row][col].setMine(true);.

Since all this is in public void initMineField(int mines), someone could call it with a large number, putting the program into an infinite loop. It could be solved by taking it private, or by checking the input for sanity, but it's a bug as it is.

---

This is in rippleUncover():

// open next 3 rows and 3 columns recursively
for (int row = 0; row < 3; row++)
{
    for (int column = 0; column < 3; column++)
    {
    // check all the above checked conditions
    // if met then open subsequent blocks
    if ((rowClicked + row - 1 >= 0) && (rowClicked + row - 1 < 8) && (columnClicked + column - 1 >= 0) &&
        columnClicked + column - 1 < 8) {
        if (mData[rowClicked + row - 1][columnClicked + column - 1].isCovered
                    && (rowClicked + row - 1 > 0) && (columnClicked + column - 1 > 0)
                    && (rowClicked + row - 1 < 9) && (columnClicked + column - 1 < 9)) {
            rippleUncover(rowClicked + row - 1, columnClicked + column - 1);
            }
        }
    }
}

The expressions rowClicked + row - 1 and columnClicked + column - 1 are hard to read, make otherwise complicated expressions extremely complicated, and badly violate DRY. I'd suggest r and c as local variables to simplify this, and by adjusting the for loop limits, we can make it clearer what we're trying to do.

Since you're checking whether coordinates are on the board a lot, it would make sense to create a helper function, call it inBounds(r, c) that has return ((r >= 0) && (r < 8) && (c >= 0) && (c < 8)); as its body.

The comment // check all the above checked conditions is unclear.

This gives us:

// open next 3 rows and 3 columns recursively
for (int row = -1; row <= 1; row++)
{
    int r = rowClicked + row;

    for (int column = -1; column <= 1; column++)
    {
        int c = columnClicked + column;

        if (inBounds(r, c)) {
            if (grid[r][c].isCovered
                    && (r > 0) && (c > 0)
                    && (r < 9) && (c < 9)) {
                rippleUncover(r, c);
            }
        }
    }
}

Looking at the nested ifs, it's hard to tell what you were trying to do. Something that's < 8 is also < 9. It appears to be treating the top and bottom of the board differently. They mentioned that flood fill wasn't working quite right, so I wouldn't be surprised if the bug was in here somewhere.

Answer 2

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

They probably wanted you to separate things into separate classes, and to use some variation of MVC Model View Controller.