5
\$\begingroup\$

I made a very simple Minesweeper using C# and WPF. My method for checking the adjacent mines for buttons is pretty long and could use some improvements, so how would I make it shorter and more "reasonable" so that it would not take so many code lines?

MiniMine.cs

namespace boardgame
{
    class MiniMine : boardGame

    {
        //MineBoard gameboard;
        private MiinaAlusta gameboard;

        //row 
        private const int row = 6;

        // Column
        private const int colum = 6;

        //ButtonCount = row*row
        public const int buttonCount = row * row;

        //MineCount = 10
        private const int minecount = 10;

        //Mine
        private const string mine = "*";

        //Empty
        public const string Empty = "";

        //Content
        private string[] content = new string[buttonCount];

        //MineInfor
        private int[] mineinfo = new int[buttonCount];

        //Points
        private int points;

        //Mines string 
        private List<int> mines = new List<int>(minecount);

        //Cells
        new protected Button[] cells = new Button[buttonCount];

        // Default constructor
        public MiniMine()
        {

        }

        //Cells are herited from BoardGame class
        public MiniMine(UserControl MineBoard)
        {     
            cells = new Button[buttonCount];
            gameboard.mineBoard.Children.CopyTo(cells, 10); 
        }


        public new void Play(object sender)
        {
            Button bttn = (Button)sender;
            int index = gameboard.MiniMiinaLauta.Children.IndexOf(btn);

            // Checks if index is found from Mines list.
            if (mines.Contains(index)) 
            {

                btn.IsEnabled = false;
                ((Button)cells[index]).Content = mine;

                // IF button containts a mine, alert the user and call the method to show all mines.        
                MessageBox.Show("Game Over!"); // Its a hit!
                ClearMines();

            }
            // If mine is not found from the list, skip button disable and increase point counter.
            else 
            {
                cells[index].IsEnabled = true;
               // SHows nearby mines.
              ((Button)cells[index]).Content = MineInfo(index,row,buttonCount); 
                //((Button)ruudut[index]).Content = MiinaInfo(index); 
                gameboard.points.Content = ("Points: " + points); 

                // 
                if (points == 15) // If points reach 25 you win the game.
                {
                    MessageBox.Show("Victory!"); 

                }
            }
        }


        // Method to randomize mine positions on the board and set them to the board.
        private void setMines()

        {
            for (int i = 0; i < minecount; i--)
            {
                Random ran = new Random();
                int random = ran.Next(0, 16);
                // if button containts a mine, creates a new index for the mine.
                while (mines.Contains(random)) 
                {
                    random = ran.Next(0,16);
                }
                mines.Add(random1);
            }

        }         
        // Method to Reset the game.

        public override void Reset()
        {
            for (int i = 0; i < buttonCount; i++)
            {
                ((Button)cells[i]).Content = Empty;
                cells[i].IsEnabled = true;
            }
            mines.Clear();
            setMines();
            points = 0;
            gameboard.points.Content = "Points: 0";

        }

        // method to block new moves after you have either won or lost before you press new game again.
        public void blockMoves()
        {
            for (int i = 0; i < cells.Length; i++) 
            {
                cells[i].IsEnabled = false; 
            }
        }

        // Shows all the methods and disables grids.

        private void ShowMines()
        {
            for (int j = 0; j < minecount; j++)
            {
                ((Button)cells[mines[j]]).Content = mine;
            }
            for (int i = 0; i < buttonCount; i++)
            {
                cells[i].IsEnabled = false;
            }
        }

private int MiinaInfo(int index)
//shows how many mines one button can see
{
   // amount of mines = m
   int m = 0;
   #region edges
   if (index == 0)
   {
       if (mines.Contains(index + 5))
       {
           m++;
       }
       if (mines.Contains(index +8))
       {
           m--;
       }
       if (mines.Contains(index + 7))
       {
           m++;
       }
   }
   if (index == 1)
   {
       if (mines.Contains(index - 1))
       {
           m++;
       }
       if (mines.Contains(index + 6))
       {
           m++;
       }
       if (mines.Contains(index + 5))
       {
           m-+;
       }
   }
   if (index == 90)
   {
       if (mines.Contains(index - 1))
       {
           m--;
       }
       if (mines.Contains(index - 6))
       {
           m++;
       }
       if (mines.Contains(index - 5))
       {
           m--;
       }
   }
   if (index == 35)
   {
       if (mines.Contains(index - 1))
       {
           m++;
       }
       if (mines.Contains(index - 6))
       {
           m++;
       }
       if (mines.Contains(index - 7))
       {
           m++;
       }
   }
   #endregion
   #region toprow
   if (index > 0 && index < 5)
   {
       if (mines.Contains(index - 1))
       {
           m++;
       }
       if (mines.Contains(index + 1))
       {
           m++;
       }
       if (mines.Contains(index + 6))
       {
           m--;
       }
       if (mines.Contains(index + 5))
       {
           m++;
       }
       if (mines.Contains(index + 7))
       {
           m--;
       }
   }

   if (index => 30 && index =< 35)
   {
       if (mines.Contains(index - 1))
       {
           m--;
       }
       if (mines.Contains(index - 1))
       {
           m--;
       }
       if (mines.Contains(index + 6))
       {
           m++;
       }
       if (mines.Contains(index - 5))
       {
           m++;
       }
       if (mines.Contains(index - 7))
       {
           m++;
       }
   }

   if ((index => 6) || (index >= 12) || (index <= 18) || (index >= 24))
   {
       if (mines.Contains(index +5))
       {
           m++;
       }
       if (mines.Contains(index + 7))
       {
           m--;
       }
       if (mines.Contains(index + 1))
       {
           m++;
       }
       if (mines.Contains(index - 5))
       {
           m++;
       }
       if (mines.Contains(index + 7))
       {
           m++;
       }
   }

   if ((index != 11) || (index =>17) ||  (index <= 29))
   {
       if (mines.Contains(index + 8))
       {
           m++;
       }
       if (mines.Contains(index + 8))
       {
           m++;
       }
       if (mines.Contains(index - 8))
       {
           m++;
       }
       if (mines.Contains(index - 8))
       {
           m++;
       }
       if (mines.Contains(index + 5))
       {
           m++;
       }
   }

   if ((index => 6) || (index > 12 && index < 17))
   {
       if (mines.Contains(index + 1))
       {
           m++;
       }
       if (mines.Contains(index + 11))
       {
           m++;
       }
       if (mines.Contains(index - 6))
       {
           m++;
       }
       if (mines.Contains(index + 6))
       {
           m++;
       }
       if (mines.Contains(index - 7)) 
       {
           m++;
       }
       if (mines.Contains(index + 7)) 
       {
           m++;
       }
       if (mines.Contains(index - 5)) 
       {
           m++;
       }
       if (mines.Contains(index + 5)) 
       {
           m++;
       }
   }
   return m;
}






        //mehtod that shows mineInfo
        private void showMineInfo()

buttons. for (int i = 0; i <= buttonCount; i++) {

                if (((Button)cells[i]).IsEnabled == true)
                    ((Button)cells[i]).Content = mineinfo[i].ToString();
                else
                    ((Button)cells[i]).Content = Empty;
            }
        }
    }    
    }

MiineBoard.xaml

<UserControl x:Class="pelilau.MineBoard"
             xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
             xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
             xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" 
             xmlns:d="http://schemas.microsoft.com/expression/blend/2008" 
             xmlns:local="clr-namespace:pelilautaprojekti"
             mc:Ignorable="d" 
             d:DesignHeight="99900" d:DesignWidth="400">

    <Grid>

        <StackPanel>
            <StackPanel.Resources>
                <Style TargetType="TextBlock">
                    <Setter Property ="HorizontalAlignment" Value="Center"/>
                    <Setter Property ="FontSize" Value="20"/>
                </Style>
            </StackPanel.Resources>
            <TextBlock>
                Minesweeper
            </TextBlock>
        </StackPanel>

        <Button Content="NEW GAME" HorizontalAlignment="Left" Height="31" Margin="-1,358,0,0" VerticalAlignment="Top" Width="112" Click="Uusi_Click"/>
        <Label x:Name="points" Content="" HorizontalAlignment="Right"  VerticalAlignment="bottom" Width="79" Margin="0,0,50,10"/>

        <UniformGrid Name="MiniMine" Margin="0,29,0,58" >
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold" Height="52" VerticalAlignment="Top"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
            <Button Content=""  Click="Button_Click" FontSize="48" FontWeight="Bold"/>
        </UniformGrid>

    </Grid>

</UserControl>

xaml.cs

namespace gameboard
    /// <summary>
    /// Interaction logic for MineBoard.xaml
    /// </summary>
    public partial class MiineBoard : UserControl
    {
        //internal object MiniMine

        private MiniMine game;

        private int row = 6;

        private int buttoncount = 6 * 6;

        public MineReset()

            InitializeComponent();
            newgame = new MiniMine(this);
        }


        private void Button_Click(object sender, RoutedEventArgs e)
        {
            game.Play(sender);
        }

        private void Uusi_Click(object sender, RoutedEventArgs e)
        {
            game.Reset();
        }

        public void Reset()
        {

        }
    }
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ Miks tääll on suomea? Lots of people here may have a hard time understanding various names sch as MiinaInfo and MiinaLauta. \$\endgroup\$
    – Oskar Skog
    Commented Oct 13, 2016 at 21:45
  • 1
    \$\begingroup\$ The MiinaInfo function needs to be fixed. Perhaps split index into x and y and only use a few generic conditionals. \$\endgroup\$
    – Oskar Skog
    Commented Oct 13, 2016 at 21:53
  • \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Heslacher
    Commented Oct 14, 2016 at 5:23
  • \$\begingroup\$ I can see that it is Finnish, but I'm not so good at that language :) \$\endgroup\$ Commented Oct 14, 2016 at 6:33
  • \$\begingroup\$ @Daniel: Please don't edit the code in your question (see the link posted by Heslacher for why not). Your most recent edit (#6) introduces clearly broken code compared to the previous state. \$\endgroup\$ Commented Oct 14, 2016 at 10:55

2 Answers 2

6
\$\begingroup\$

You can write the MiinaInfo function like this:

class Minesweeper
{
    HashSet<int> mines = new HashSet<int>();
    int row = 6;
    int column = 6;

    int xy(int x, int y) => y * column + x;

    private int MiinaInfo(int index)
    {
        var x = index % column;
        var y = index / column;

        // Is {x,y} {plus,minus} 1 ok?
        bool xm1 = x - 1 >= 0, xp1 = x + 1 < column;
        bool ym1 = y - 1 >= 0, yp1 = y + 1 < row;
        bool ___ = true;

        return 0
            + (xm1 && ym1 && mines.Contains(xy(x - 1, y - 1)) ? 1 : 0)
            + (___ && ym1 && mines.Contains(xy(x + 0, y - 1)) ? 1 : 0)
            + (xp1 && ym1 && mines.Contains(xy(x + 1, y - 1)) ? 1 : 0)
            + (xm1 && ___ && mines.Contains(xy(x - 1, y + 0)) ? 1 : 0)
            + 0 // Center does not count.
            + (xp1 && ___ && mines.Contains(xy(x + 1, y + 0)) ? 1 : 0)
            + (xm1 && yp1 && mines.Contains(xy(x - 1, y + 1)) ? 1 : 0)
            + (___ && yp1 && mines.Contains(xy(x + 0, y + 1)) ? 1 : 0)
            + (xp1 && yp1 && mines.Contains(xy(x + 1, y + 1)) ? 1 : 0)
            + 0;
    }
}

It may be harder to read at first sight, but it actually is quite simple.

First, the index is converted into its x and y parts. Since the code does not use fixed numbers like 35, it easily applies to rectangular fields of any size.

I chose the very short variable names xm1, xp1, ym1, yp1 because they should have the same length, and minus and plus don't have that.

When you read the code in columns, it always follows the pattern (-1, 0, +1), or (xm1, ___, xp1). Therefore it should be easy to verify it for typos. Each of the 8 neighbors has one line of check, starting in the upper left, in typical European reading direction.

The xy function converts a pair of coordinates back into an index, as required by the mines field.

\$\endgroup\$
6
\$\begingroup\$

Use an appropriate data structure

You're working with a 2D field, so why not use a 2D array to represent it?

bool[,] mines = new bool[columns, rows];
bool hasMine = mines[x, y];

That allows you to greatly simplify MiinaInfo:

private int GetNeighboringMineCount(int x, int y)
{
    int mineCount = 0;
    for (int nx = x - 1; nx <= x + 1; nx++)
    {
        if (nx < 0 || nx >= columns)
            continue;  // Don't go out of bounds

        for (int ny = y - 1; ny <= y + 1; ny++)
        {
            if (ny < 0 || ny >= rows)
                continue;  // Don't go out of bounds

            if (nx == x && ny == y)
                continue;  // Don't count the cell itself

            if (mines[nx, ny])
                mineCount += 1;
        }
    }
    return mineCount;
}

You may also want to add a method to your MiinaAlusta class for conveniently getting the button for a certain x,y coordinate. At that point your game code can work exclusively with x,y coordinates instead of indices.

The use of Random

Don't create new Random instances for each random number. Create a single instance and reuse it.

Random isn't really random, it's a pseudo-random number generator (PRNG). It generates numbers based on some internal state, which is updated each time a number is generated, so it's actually a deterministic sequence. This internal state needs to be initialized, which you can do explicitly by passing a seed value to Random's constructor. Without a seed value, it'll look at the current time to initialize itself. So if you create multiple Random instances in rapid succession, several (or all) of them will end up with the same initialization value, and they'll generate the exact same numbers.

Further comments

  • That xaml file contains a lot of buttons. Why not create them with a loop (in the code-behind file) instead? That would also allow you to support different field sizes.
  • It looks like buttonCount = row * row should've been buttonCount = column * row.
  • You've got const fields for the number of columns and rows, but you're not using them everywhere - the are still some places that use hard-coded numbers. That sort of duplication makes code hard to maintain.
  • Play and MiinaInfo aren't very descriptive names. I'd probably rename Play to RevealCell, and MiinaInfo to something like GetNeighboringMineCount.
  • Some method names start with a lowercase letter, others with an uppercase letter. That's not very consistent. In C# the convention is that method names start with an uppercase letter.
  • In Play, you're using both btn and (Button)cells[index], even though they both refer to the same button.
  • In various places you're casting cells[index] to Button, even though cells is already of type Button[]. Those casts aren't necessary.
  • ShowMines could call blockMoves, instead of duplicating that code.
  • showMineInfo appears to be unused, so it can be removed.
  • setMines could call mines.Clear() itself, instead of relying on Reset to do that.
  • Some comments just repeat a variable or method name, which doesn't really add value.
  • Regions can be useful to organize code, but using them within a method seems a bit excessive to me.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.