Android应用程序访问(扫雷游戏)

Question

我最近做了一个Android Minesweeper应用程序，在我和任何一个工程师交谈之前，我做了一个面试前的编码挑战，但被拒绝了。这是给出的反馈，但我不确定其中的一些。

• ☺︎游戏成功
• 非常小的实现(嗯，我做了所需的.)
• 没有逻辑分离，整个游戏都塞进了GameActivity (不确定他们想要什么)
• ☺︎简明码
• 游戏逻辑与视图类不分离(不确定它们想要什么)
• 当你点击一个空的广场时，充满洪水并不是很正常。
• 没有充分利用Android的功能(例如GridView) (这对我来说是最好的，因为我用TableLayout代替GridView)

我的代码在这里。

最相关的守则如下：

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();
}
}

招聘人员说，他们目前没有Android工程师，但他们有一些人“知道”谁审查了我的代码。看上去有点粗略。公司也不会考虑我简历上的任何其他样本，除了他们的编码挑战(是的，其中一家公司)。

有人能检查一下我的代码并给出指点吗？

Answer 1

我不知道Android，但由于还没有人发表评论，我可以看到一些问题，我将尝试一下。

有时你会使用这种卷曲支撑的风格：

stuff {
    inner stuff;
}

有时这种风格：

stuff
{
    inner stuff;
}

保持一致。

在宽度和高度上都有大量的编码8s。如果他们突然告诉你让你的代码工作在一个9x17板，而不是一个8x8，你应该能够通过改变确切的两个数字(除了可能的图形东西，如安装在屏幕上)。

noSurroundingMines是个坏名字。这听起来像布尔值，但它是一个int。我建议nearbyMines。您使用mData[0].length和mData.length来表示宽度和高度。由于游戏中棋盘的大小不会改变，我建议width和height。mData也是一个不好的名字，我建议board或grid。

有些注释在阅读代码时是显而易见的，或者通过更好的命名是显而易见的，而在其他地方，代码不清楚，也没有任何注释可以帮助。换个角度比较好。

这是丑陋的：

  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;
  }

定义适当的数组，如下所示：

this.setBackgroundResource(arr[nearbyMines]);

尽管你想想出一个比arr更好的名字。

这似乎令人费解：

    // 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();
                        }
                    }
                }
            }
        }

这是棋盘初始化的一部分，你在计算一个空间附近的地雷数量。内部2循环每个循环只有3次迭代，因此它的效率并不像乍一看上去那么低，但是内部循环和外部循环在逻辑上没有关系，所以您可以将内部循环放在一个单独的方法中，将内部部分转换为countNearbyMines(row, column);，并使整个代码块更具可读性。

此外，硬编码的8s应该是width和height，似乎没有任何理由使用ii和jj代替i和j作为计数器。

稍后，我提出了一个边界检查函数的建议，这也将简化这段代码。

简化代码块：

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

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();
        }
    }
}

在此代码和至少一个其他点中，您已经将ifs与外部if嵌套在一起，测试坐标是否在边界内。IIRC，您可以使用类似inBounds(i, j) && grid[i][j].isMine()的东西，并依赖于&&的短路，将其转化为单一的if。

在同一方法的前面，您有以下内容：

        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);

        }

由于如果while循环的条件为false，则不会执行它，因此这里实际上不需要if语句。我认为randomRow和randomCol太冗长了，因为他们是本地人，我们就在这里给rnd.nextInt()打电话。进行建议的修改后，我们得到了以下内容：

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);

我不知道setTag()是什么，但您可能希望在setMine()中也这样做，将最后3行转换为grid[row][col].setMine(true);。

因为所有这些都是在public void initMineField(int mines)中，有人可以用一个大的数字调用它，将程序放入一个无限循环中。它可以通过私密解决，也可以通过检查输入是否正常来解决，但它本身就是一个bug。

这是在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);
            }
        }
    }
}

rowClicked + row - 1和columnClicked + column - 1的表达式很难读懂，使原本复杂的表达式变得非常复杂，并且严重违反了DRY。我建议r和c作为局部变量来简化这一点，并且通过调整for循环限制，我们可以更清楚地了解我们要做的事情。

因为您正在检查坐标是否在板上很多，所以创建一个助手函数(称为inBounds(r, c) )是有意义的，它的主体是return ((r >= 0) && (r < 8) && (c >= 0) && (c < 8));。

// check all the above checked conditions的评论目前尚不清楚。

这使我们：

// 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);
            }
        }
    }
}

看看嵌套的ifs，很难知道您想要做什么。< 8也是< 9。它似乎以不同的方式对待董事会的顶部和底部。他们提到洪水填充物不太好用，所以如果虫子在这里的某个地方，我就不会感到惊讶了。

Answer 2

没有逻辑分离，整个游戏都塞进了GameActivity (不确定他们想要什么)

他们可能希望您将事物分离成不同的类，并使用MVC 模型视图控制器的一些变体。