9
\$\begingroup\$

I'm writing Minesweeper implementation in Java. I have little experience yet, so I need some advices:

  • about improving code, making it more readable and flexible
  • about coding style
  • about common mistakes that newbies usually do (I suspect I have some in my code)
  • about code structure (for example, I'm not sure about where and how it would be better to store information about neighbour cells)
  • about choosing best data structure for storing the minefield (currently I use simple 2D array, but I think it won't be suitable if I want non-square cells, for example)
  • about optimization and rewriting some functions if needed

Cell.java

// represents single cell
public class Cell {
    public enum Type {
        MINED,EMPTY
    }
    
    public enum State {
        COVERED,UNCOVERED,FLAGGED,FLAGGED_DEFUSED
    }
    
    public int nColIndex,nRowIndex; // cell position inside the grid
    
    public int nMinesAround; // total number of mines in nearby cells
    
    protected Type type;
    protected State state;
    
    public Cell(int row, int col) { // by default cell is clear and covered
        this.nColIndex=col;
        this.nRowIndex=row;
        this.type = Type.EMPTY;
        this.state = State.COVERED;
    }
    
    public boolean isCovered()
    {
        return (this.state==State.COVERED || this.state==State.FLAGGED);
    }
}

Game.java

import java.util.ArrayList;
import java.util.List;
import java.util.Stack;
import java.util.concurrent.ThreadLocalRandom;
import javax.swing.JPanel;

//responsible for game logic
public class Game extends JPanel {
    public enum GameStatus {
        LOST,VICTORY,RUNNING;
    }
    
    protected Cell[][] minefield; // 2d array representing a minefield
    protected int nCols, nRows, nMines, nUncoveredClearCells=0;
    protected GameStatus status; // state of the current game (running, lost or won)
    
    public Game(int cols, int rows, int mines)
    {
        // start a new game
        this.nCols=cols;
        this.nRows=rows;
        this.nMines=mines;
        this.status=GameStatus.RUNNING;
        field_init();
    }
    
    protected void field_init()
    {
        // initialize an array by filling it with cell objects
        this.minefield = new Cell[nRows][nCols];
        for (int i = 0; i < nRows; i++)
        {
            for (int j = 0; j < nCols; j++)
            {
                minefield[i][j] = new Cell(i, j);
            }  
        }
    }   
    
    public void plantMines(Cell firstClickedCell)
    {
        // randomly plant mines by generating random coords in 2d array
        // (like if it was one-dimensional)
        int nMinesPlanted=0;
        int nPos=0,nColIndex=0,nRowIndex=0;
        Cell currentCell;
        ThreadLocalRandom rnd = ThreadLocalRandom.current();
        List<Cell> nearbyCells;
        while(nMinesPlanted<nMines)
        {
            nPos = rnd.nextInt(0, nCols*nRows);
            nRowIndex = nPos/nCols; // get Y cell coord (row index)
            nColIndex = nPos%nCols; // get X cell coord (column index)
            currentCell=minefield[nRowIndex][nColIndex];
            if(currentCell.type!=Cell.Type.MINED && currentCell!=firstClickedCell)
            {
                currentCell.type=Cell.Type.MINED;
                nearbyCells = getCellNeighbours(currentCell); 
                for(Cell nearbyCell : nearbyCells) nearbyCell.nMinesAround++; // notify cell neighbours about new mine
                nMinesPlanted++;
            }
            
        }
    }
    
    protected List<Cell> getCellNeighbours(Cell myCell)
    {
        // find neighbours of current cell
        // and add them to the list
        List<Cell> cells = new ArrayList<Cell>();
        if(myCell.nColIndex-1>=0) cells.add(minefield[myCell.nRowIndex][myCell.nColIndex-1]); // left neighbour
        if(myCell.nColIndex+1<nCols) cells.add(minefield[myCell.nRowIndex][myCell.nColIndex+1]); //right neighbour
        if(myCell.nRowIndex-1>=0) cells.add(minefield[myCell.nRowIndex-1][myCell.nColIndex]); // top neighbour
        if(myCell.nRowIndex+1<nRows) cells.add(minefield[myCell.nRowIndex+1][myCell.nColIndex]); // bottom neighbour
        if(myCell.nRowIndex+1<nRows && myCell.nColIndex+1<nCols) // bottom-right neighbour
            cells.add(minefield[myCell.nRowIndex+1][myCell.nColIndex+1]);
        if(myCell.nRowIndex-1>=0 && myCell.nColIndex-1>=0) // top-left neighbour
            cells.add(minefield[myCell.nRowIndex-1][myCell.nColIndex-1]);
        if(myCell.nRowIndex+1<nRows && myCell.nColIndex-1>=0) // bottom-left neighbour
            cells.add(minefield[myCell.nRowIndex+1][myCell.nColIndex-1]);
        if(myCell.nRowIndex-1>=0 && myCell.nColIndex+1<nCols) // top-right neighbour
            cells.add(minefield[myCell.nRowIndex-1][myCell.nColIndex+1]);
        return cells;
    }
    
    
    public void clickCell(Cell selectedCell)
    {
        // Trying to open a mined cell - the game is lost
        if(selectedCell.type==Cell.Type.MINED) {
            status=GameStatus.LOST;
            uncoverAllMines();
        }
        else 
        {
            // Clicked cell is clear
            uncoverCells(selectedCell);
            if(isVictory()) { // Check if there are any covered cells without mines
                status=GameStatus.VICTORY; // there aren't))
                flagAllMines(); 
            }
        }
    }
    
    protected void uncoverCells(Cell selectedCell)
    {
        // Uncover a clear cell. If it has no mines around
        // we also open all its nearby cells
        // Then do the same for their neighbours and so on...
        Stack<Cell> stack = new Stack<Cell>();
        List<Cell> nearbyCells;
        stack.push(selectedCell);
        while(!stack.isEmpty()) // expand area around clicked cell while possible
        {
            Cell currentCell = stack.pop();
            if(currentCell.state!=Cell.State.UNCOVERED) // skip already uncovered cells
            {
                currentCell.state=Cell.State.UNCOVERED;
                nUncoveredClearCells++;
                if(currentCell.nMinesAround==0) // if ALL neighbours are clear, we open them too
                {
                    nearbyCells = getCellNeighbours(currentCell);
                    for(Cell nearbyCell : nearbyCells)
                    {
                        if(nearbyCell.isCovered()) { // skip already uncovered cells (x2)
                            stack.push(nearbyCell); 
                        }
                    }
                }
            }
        }
    }
    
    
    protected void uncoverAllMines()
    {
        // If we open a mined cell, the game is lost
        // In such situation other mined cells are opened automatically
        // If some of them were previously flagged then they are shown as defused
        Cell currentCell;
        for (int i = 0; i < nRows; i++)
            for (int j = 0; j < nCols; j++)
            {
                currentCell=minefield[i][j];
                if(currentCell.type==Cell.Type.MINED) {
                    if(currentCell.state==Cell.State.FLAGGED) currentCell.state=Cell.State.FLAGGED_DEFUSED;
                    else currentCell.state=Cell.State.UNCOVERED;
                }
            }
    }
    
    protected boolean isVictory()
    {
        // Check if we haven't opened all the clear cells yet
        // If it's already done - it's a victory. Otherwise we continue the game
        return (nCols*nRows-nMines==nUncoveredClearCells);
    }
    
    protected void flagAllMines()
    {
        // If there are no clear covered cells left then the game is won
        // Mined cells are automatically marked with flags
        Cell currentCell;
        for (int i = 0; i < nRows; i++)
            for (int j = 0; j < nCols; j++)
            {
                currentCell=minefield[i][j];
                if(currentCell.type==Cell.Type.MINED) {
                    currentCell.state=Cell.State.FLAGGED;
                }
            }
    }
    
    public void restartGame()
    {
        // If user wants to restart existing game
        // we just cover any open cells
        Cell currentCell;
        nUncoveredClearCells=0;
        status=GameStatus.RUNNING;
        for (int i = 0; i < nRows; i++)
            for (int j = 0; j < nCols; j++)
            {
                currentCell=minefield[i][j];
                currentCell.state=Cell.State.COVERED;
            }
    }
    
    public void setFlag(Cell selectedCell)
    {
        // Sets a flag on covered cell (or removes it if it's already present)
        if(selectedCell.state==Cell.State.COVERED) selectedCell.state=Cell.State.FLAGGED;
        else if(selectedCell.state==Cell.State.FLAGGED) selectedCell.state=Cell.State.COVERED;
    }
}       

GameGrid.java

import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.Image;
import java.io.IOException;
import java.util.HashMap;

import javax.imageio.ImageIO;

// Responsible for painting the minefield
public class GameGrid extends Game {
    final static int CELL_WIDTH=20; // cell width in pixels
    final static int CELL_HEIGHT=20; // cell height in pixels
    HashMap<String,Image> icons;
    
    public void paint(Graphics graphics) {
        for (int i = 0; i < nRows; i++)
            for (int j = 0; j < nCols; j++)
            {
                Image currentCellImage=null;
                switch(minefield[i][j].state)
                {      
                    case COVERED: { // cell is covered
                        currentCellImage = icons.get("covered");
                        break;
                    }
                    case UNCOVERED: {
                        if(minefield[i][j].type==Cell.Type.MINED)
                        { 
                            currentCellImage = icons.get("mined"); // cell is open and contains a mine
                        }
                        else currentCellImage = icons.get(Integer.toString(minefield[i][j].nMinesAround)); // cell is open and clear
                        break;
                    }
                    case FLAGGED: { // cell is covered and flagged
                        currentCellImage = icons.get("covered_flagged");
                        break;
                    }
                    case FLAGGED_DEFUSED: { // cell is open, mined and was previously marked with a flag
                        currentCellImage = icons.get("mined_flagged");
                        break;
                    }   
                    default: break;
                }
                if(currentCellImage!=null)
                    graphics.drawImage(currentCellImage, j*CELL_WIDTH, i*CELL_HEIGHT, CELL_WIDTH, CELL_HEIGHT,null); // draw current cell
            }
    }
        
    @Override
    public Dimension getPreferredSize()
    {
        // report grid dimensions in order to adjust parent's size
        return new Dimension(CELL_WIDTH*nCols, CELL_HEIGHT*nRows);
    };
    
    public GameGrid(int cols, int rows, int mines) {
        super(cols, rows, mines);
        icons_init();
    }
    
    private void icons_init()
    {
        // load icons that represent different cell types and states
        icons = new HashMap<String,Image>();
        try {
        for(int i=0; i<=8; i++)
            {
                // Current cell is open and has N (0-8) mines around (horiz., vert. and diag.)
                icons.put(Integer.toString(i), ImageIO.read(GameGrid.class.getResource("images/"+Integer.toString(i)+".png")));
            }
            icons.put("covered", ImageIO.read(GameGrid.class.getResource("images/coveredCell.png"))); // cell is covered (and has no flag)
            icons.put("covered_flagged", ImageIO.read(MainWindow.class.getResource("images/coveredFlaggedCell.png"))); // cell is covered and has a flag
            icons.put("mined", ImageIO.read(GameGrid.class.getResource("images/minedCell.png"))); // cell is open and mined
            icons.put("mined_flagged", ImageIO.read(GameGrid.class.getResource("images/minedFlaggedCell.png"))); // cell is open, mined and was previously marked with a flag
        }
        catch(IOException ex)
        {
            ex.printStackTrace(); // could not load icon(s) for some reason
        }
    }
    
    public Cell getCellFromCoords(int x, int y) {
        return minefield[y/CELL_HEIGHT][x/CELL_WIDTH]; // get cell from it's coords
    }
}

GamePanel.java

import java.awt.Graphics;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;
import javax.swing.Timer;
import javax.swing.Box;
import javax.swing.BoxLayout;
import javax.swing.JButton;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.SwingUtilities;

public class GamePanel extends JPanel
{
    final int ONE_SECOND=1000; // 1 s = 1000 ms
    JPanel pnlProgress;
    JLabel lblTime,lblMines; // elapsed time in seconds and mines count
    JButton btnRestart; // "Restart" button
    GameGrid myGame;
    ActionListener updateCurrentTime = new ActionListener() {
          public void actionPerformed(ActionEvent evt) {
              int nCurrentTime=Integer.parseInt(lblTime.getText());
              if(nCurrentTime>=999) { tmGame.stop(); return; }
              lblTime.setText(String.format("%03d", nCurrentTime+1)); // increase time counter by one
          }
      };
    Timer tmGame = new Timer(ONE_SECOND, updateCurrentTime); // game timer
    
    boolean bFirstClick = true; // mines are planted after first click
    
    MouseAdapter userClickHandler = new MouseAdapter() {
        public void mousePressed(MouseEvent me) { 
            int x = me.getX();
            int y = me.getY();
            Cell requestedCell = myGame.getCellFromCoords(x,y); // get cell under mouse cursor
            if(SwingUtilities.isLeftMouseButton(me) && requestedCell.state==Cell.State.COVERED) // open cell on left-click
            {   
                if(bFirstClick) { // first click is always safe (impossible to lose)
                    myGame.plantMines(requestedCell);
                    tmGame.start();
                    bFirstClick=false;
                }
                myGame.clickCell(requestedCell);
                if(myGame.status!=Game.GameStatus.RUNNING) endGame(myGame.status); // check game result
            }
            else if(SwingUtilities.isRightMouseButton(me)) myGame.setFlag(requestedCell); // set or remove flag on right-click
            invalidate();
            repaint();
        }
    };
    
    public void startGame(Level lvl, boolean bRestartExisting)
    {
        tmGame.stop();
        if(bRestartExisting) {
            myGame.restartGame();
            tmGame.start();
        }
        else {
            if(myGame!=null) remove(myGame);
            myGame=new GameGrid(lvl.nCols, lvl.nRows, lvl.nTotalMines);
            add(myGame);
            bFirstClick=true;
        }
        btnRestart.setText("Restart");
        if(myGame.getMouseListeners().length==0) myGame.addMouseListener(userClickHandler); // enable mouse input if it's disabled
        lblMines.setText(String.format("%03d", myGame.nMines));
        lblTime.setText("000");
        myGame.invalidate();
        myGame.repaint();
    }
    
    public void endGame(Game.GameStatus state)
    {
        tmGame.stop();
        if(state==Game.GameStatus.VICTORY) btnRestart.setText("You Win!");
        else btnRestart.setText("You Lose!");
        myGame.removeMouseListener(userClickHandler); // disable mouse input
    }
    
    public GamePanel()
    {   
        this.setLayout(new BoxLayout(this,BoxLayout.Y_AXIS));
        lblTime=new JLabel("000");
        lblMines=new JLabel("000");
        btnRestart=new JButton("Restart");
        pnlProgress=new JPanel();
        pnlProgress.setLayout(new BoxLayout(pnlProgress, BoxLayout.X_AXIS));
        pnlProgress.add(lblTime);
        pnlProgress.add(Box.createGlue());
        pnlProgress.add(btnRestart);
        btnRestart.addActionListener(new ActionListener() { 
              public void actionPerformed(ActionEvent e) { 
                   startGame(null, true); // restart existing game
                  } 
                });
        pnlProgress.add(Box.createGlue());
        pnlProgress.add(lblMines);
        add(pnlProgress);
        startGame(Level.arrPredefinedLevels[0],false); // "Beginner" level is default
    }
    
    public void paintComponent(Graphics g) {
        super.paintComponent(g);  
    }  
}

GameParamsWindow.java

import java.awt.Dimension;
import java.awt.FlowLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.FocusAdapter;
import java.awt.event.FocusEvent;
import java.awt.event.WindowEvent;
import java.awt.event.WindowAdapter;
import javax.swing.BoxLayout;
import javax.swing.JButton;
import javax.swing.JComboBox;
import javax.swing.JDialog;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.SwingUtilities;
import javax.swing.text.DocumentFilter;
import javax.swing.text.AbstractDocument;
import javax.swing.text.AttributeSet;
import javax.swing.text.BadLocationException;

public class GameParamsWindow extends JDialog {
    JComboBox<Level> cbDifficulty;
    JLabel lblDifficulty,lblCols,lblRows,lblMines;
    JNumericTextField txtCols,txtRows,txtMines;
    JButton btnStart;
    JPanel pnlCommon,pnlBasic,pnlCustomize;
    JFrame frmParent;
    boolean bCancelled=false;
    
    public class JNumericTextField extends JTextField {
        
        JNumericTextField(int nMaxLength)
        {
            setPreferredSize(new Dimension(40,20));
            AbstractDocument doc=(AbstractDocument) getDocument();
            addFocusListener(new FocusAdapter() {
                    public void focusGained(FocusEvent e) {
                        SwingUtilities.invokeLater(new Runnable() {
                            public void run() {
                                ((JNumericTextField)e.getSource()).selectAll();
                            }
                        });
                    }
                });
            doc.setDocumentFilter(new DocumentFilter() {
                public void replace(FilterBypass fb, int offset, int len,
                        String str, AttributeSet a) throws BadLocationException {
                        // JNumericTextField allows only digits
                        String sNumericOnly=str.replaceAll("[^0-9]", "");
                        super.replace(fb, offset,len, sNumericOnly ,a);
                        // restrict text length
                        if(doc.getLength()>nMaxLength) doc.replace(0, doc.getLength(), doc.getText(0, doc.getLength()).substring(0,nMaxLength),a);
                }
            });
        }
        
        public int getNumber()
        {
            // get integer from JNumericTextField instance
            if(getText().isEmpty()) return 0;
            return Integer.parseInt(getText());
        }
        
        public void setNumber(int num)
        {
            setText(Integer.toString(num));
        }
    }
    
    public void setCustomValuesAllowed(boolean bFlag)
    {
        //(dis)allow setting custom values
        txtCols.setEditable(bFlag);
        txtRows.setEditable(bFlag);
        txtMines.setEditable(bFlag);
    }
    
    public GameParamsWindow()
    {
        initSettingsUI();
        setModal(true); // this is a dialog window
    }
    
    Level showDialog(JFrame frmParent)
    {
        Level selectedLevel;
        bCancelled=false;
        this.frmParent=frmParent;
        setLocationRelativeTo(this.frmParent);
        cbDifficulty.setSelectedItem(cbDifficulty.getSelectedItem()); // refresh text fields
        setVisible(true);
        if(bCancelled) { return null; } // dialog was closed with "X" button
        selectedLevel=((Level)cbDifficulty.getSelectedItem()); // get selected level
        selectedLevel.setParameters(txtCols.getNumber(), txtRows.getNumber(), txtMines.getNumber()); // update level info
        return selectedLevel;
    }
    
    private void initSettingsUI()
    {
        setTitle("New Game");
        setResizable(false);
        pnlCommon=new JPanel();
        pnlCommon.setLayout(new BoxLayout(pnlCommon, BoxLayout.Y_AXIS));
        pnlBasic = new JPanel(new FlowLayout());
        pnlCustomize = new JPanel(new FlowLayout());
        lblDifficulty= new JLabel("Difficulty level");
        cbDifficulty = new JComboBox<Level>();
        txtCols = new JNumericTextField(2); // number of cols
        txtRows = new JNumericTextField(2); // number of rows
        txtMines = new JNumericTextField(3); // number of mines
        cbDifficulty.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e) {
                Level selectedLevel=(Level)((JComboBox<Level>)e.getSource()).getSelectedItem();
                txtCols.setNumber(selectedLevel.nCols);
                txtRows.setNumber(selectedLevel.nRows);
                txtMines.setNumber(selectedLevel.nTotalMines);
                setCustomValuesAllowed(selectedLevel.sName=="Custom");
            }
        });
        for(Level lvl: Level.arrPredefinedLevels) cbDifficulty.addItem(lvl); // push predefined levels to combobox
        lblCols = new JLabel("Columns:");
        lblRows = new JLabel("Rows:");
        lblMines = new JLabel("Mines:");
        btnStart = new JButton("Start");
        btnStart.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                setVisible(false); // "Start" button is pressed - dialog should be closed
                // Now parent window will get the selected level and we are ready to start the game
            }
        }); 
        pnlBasic.add(lblDifficulty);
        pnlBasic.add(cbDifficulty);
        pnlBasic.add(btnStart);
        pnlCustomize.add(lblCols);
        pnlCustomize.add(txtCols);
        pnlCustomize.add(lblRows);
        pnlCustomize.add(txtRows);
        pnlCustomize.add(lblMines);
        pnlCustomize.add(txtMines);
        pnlCommon.add(pnlBasic);
        pnlCommon.add(pnlCustomize);
        add(pnlCommon);
        pack();
        getRootPane().setDefaultButton(btnStart);
        addWindowListener( new WindowAdapter() {
              @Override
              public void windowClosing(WindowEvent we) {
                bCancelled=true; // "X" button was clicked
                // Dialog is cancelled and game won't be started
              }
              
              @Override
              public void windowActivated(WindowEvent we)
              {
                // if dialog is activated - make it's parent visible too
                  frmParent.setVisible(true); 
              }
            } );
    }
}

Level.java

public class Level {
    static Level[] arrPredefinedLevels = {
        new Level("Beginner",9,9,10),
        new Level("Medium", 16,16,40),
        new Level("Expert", 30,16,99),
        new Level("Custom", 9,9,10)
    };
    
    String sName;
    public int nCols,nRows,nTotalMines;
    final private int MIN_ROWS=9,MAX_ROWS=30,MIN_COLS=9,MAX_COLS=24,MIN_MINES=10; // limits
        
    Level(String name, int cols,int rows, int mines)
    {
        sName=name;
        setParameters(cols,rows,mines);
    }
    
    public void setParameters(int cols,int rows, int mines)
    {   
        // Assign values and make sure they aren't out of range (otherwise correct them)
        nCols=cols;
        nRows=rows;
        nTotalMines=mines;
        if(nCols<MIN_COLS) nCols=MIN_COLS;
        else if(nCols>MAX_COLS) nCols=MAX_COLS;
        if(nRows<MIN_ROWS) nRows=MIN_ROWS;
        else if(nRows>MAX_ROWS) nRows=MAX_ROWS;
        if(nTotalMines<MIN_MINES) nTotalMines=MIN_MINES;
        else if(nTotalMines>(nRows-1)*(nCols-1)) nTotalMines=(nRows-1)*(nCols-1);
    }
        
    public String toString() { return sName; }
}

MainWindow.java

import java.awt.EventQueue;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;
import javax.swing.JFrame;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;
import javax.swing.JOptionPane;
import java.awt.FlowLayout;
import java.awt.Image;
import javax.imageio.ImageIO;
import javax.swing.BoxLayout;
import javax.swing.ImageIcon;

public class MainWindow {

    private JFrame frame;
    GamePanel pnlGameUI=new GamePanel();
    GameParamsWindow dlgNewGame = new GameParamsWindow();
    static Image appIcon;
    /**
     * Launch the application.
     */
    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                try {
                    appIcon=ImageIO.read(getClass().getResource("images/appIcon.png"));
                    MainWindow window = new MainWindow();
                    window.frame.setVisible(true);
                } catch (Exception e) {
                    e.printStackTrace();
                }
            }
        });
    }

    /**
     * Create the application.
     */
    public MainWindow() {
        initialize();
    }

    /**
     * Initialize the contents of the frame.
     */
    private void initialize() {
        frame = new JFrame("Minesweeper");
        frame.setIconImage(appIcon);
        frame.setBounds(100, 100, 450, 300);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().setLayout(new BoxLayout(frame.getContentPane(), BoxLayout.Y_AXIS));
        frame.setResizable(false);
        JMenuBar tbMenu = new JMenuBar();
        tbMenu.setLayout(new FlowLayout(FlowLayout.LEFT,0,0));
        JMenu mnuGame = new JMenu ("Game");
        JMenu mnuAbout = new JMenu ("About");
        JMenuItem itemNewGame = new JMenuItem("New Game");
        JMenuItem itemExit = new JMenuItem("Exit");
        itemNewGame.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent ev) {
                Level lvlChosen = dlgNewGame.showDialog(frame);
                if(lvlChosen!=null) // if dialog was just closed with "X" button - do nothing
                {
                    pnlGameUI.startGame(lvlChosen, false); // start a new game (level was set inside the dialog)
                    frame.pack();
                }
            }
        });
        itemExit.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent ev) {
                System.exit(0);
            }
        });
        mnuAbout.addMouseListener(new MouseAdapter() {
            public void mousePressed(MouseEvent me) {   
                JOptionPane.showMessageDialog(frame,"Minesweeper 1.0, 2021",
                        "About",JOptionPane.INFORMATION_MESSAGE,new ImageIcon(appIcon));
            }
        });
        mnuGame.add(itemNewGame);
        mnuGame.add(itemExit);
        tbMenu.add(mnuGame);
        tbMenu.add(mnuAbout);
        frame.getContentPane().add(tbMenu);
        frame.getContentPane().add(pnlGameUI);
        frame.pack();
    }

}
\$\endgroup\$

1 Answer 1

4
+100
\$\begingroup\$

Very nice piece of code, thank you very musch for sharing your work with us!

i tried to list the issued order by severity.

segregation of concerns / composition over inheritance / MVC

Your displaying class GameGrid extends from Game. That violates all principles in different ways. It would be easy to increase loose coupling by defining an interface in the game that provides all content, that needs to be drawn.

interface ContentProvider{
    Set<Cell> getMinefield(); 
}

assuming Cell provides a method getLocation().

The purpose of GameGrid is to paint a Minefiled (a Set<Cell>), so you should give it a proper name.

And since paint is very specific on java.awt... and you treat it as a component (see GamePanel: GameGrid.invalidate(); and GameGrid.repaint();) it should be a component. (i think your legacy annotations (@Override getPrefferedSize()) - even though there is no proper super class) show, that you already had this idea, btw, remove them or change hierarchy towards component).

complexity / object orientated design / testability

these three issues go hand in hand. Lots of your code is complex to read and it would help to create methods to simplify the code - and increase testability. its easier to test small methods rather than the whole class.

unluckily the complexity is distributed, so i can only pick some examples in detail.

example: GameGrid.paint(Graphics graphics)

paint(Graphics graphics){
    for (int i = 0; i < nRows; i++){
        for (int j = 0; j < nCols; j++){
        ...
            //ok, here we should break complexity:
            Cell cell = minefield[i][j];
            paintCell(cell, graphics);
        }
    }
}        

stop complexity by calling a method paintCell on Cell level.

example: Game.uncoverCells(Cell selectedCell)

protected void uncoverCells(Cell selectedCell){
    // Uncover a clear cell. If it has no mines around
    // we also open all its nearby cells
    // Then do the same for their neighbours and so on...
    Stack<Cell> stack = new Stack<Cell>();
    List<Cell> nearbyCells;
    stack.push(selectedCell);
    while(!stack.isEmpty()) {// expand area around clicked cell while possible
        Cell currentCell = stack.pop();
        if(currentCell.state!=Cell.State.UNCOVERED){ // skip already uncovered cells 
            ...
            //stop here
            expandCell(currentCell);
        }
    }
}

again, stop complexity by calling a method instead of digging deeper and deeper.

example: Game.getCellNeighbours()

here is a good example, where you missed an opportunity to use object-orientated code

instead of providing a method on the board to check if a cell is inside the board, you directly add the logic into the method.

In an object orientatded design our cell would have a Location instead of primitive coordinates. and a cell would know its neighbour locations

class Cell{
    private final Location location;//injected on constructor
    List<Location> getSurroundedLocations(){//...returns eight locations}
}

and our board would know if a location is inside the board

class Board{
    boolean isInside(Location location){//you know when its inside}
    Cell getCellAt(Location location){//again: trivial};
}

that would reduce complexity of your method (and increase readability strongly)

protected List<Cell> getCellNeighbours(Cell myCell){
    List<Cell> cells = new ArrayList<Cell>();
    for (Location location: myCell.getSurroundedLocations()){
        if(isInside(location)){
            cells.add(getCell(location);
        }
    }
}

Note: that would also reduce the duplicated code: (if cell.nColIndex >= 0) that is called 3 times, as well as on row index...

example: primitives instead of behaviour (methods)
what is see all across your code is a direct check on status. that is a poor design and adds complexity and violates the object-orientated design.

  • here: Game.clickCell: if(selectedCell.type==Cell.Type.MINED) should be if(selectedCell.isMined()){...}
  • here: Game.uncoverCells: if(currentCell.state!=Cell.State.UNCOVERED) should be if(currentCell.isCovered())
  • here: Game.uncoverCells: currentCell.state=Cell.State.UNCOVERED; should be currentCell.setCovered();
  • here: GamePanel.endGame: if(state==Game.GameStatus.VICTORY) should be if(game.isVictory()){...}
  • here: ...the list is long

Note: one thing was astounding me: you did the oop approach on your method Cell.isCovered(). but here you violated the naming convention, the method does not return waht is expected: it has some hidden logic inside. that's no good idea, the method name should be adjusted to properly express the behaviour.

example:Cell iteration

for (int i = 0; i < nRows; i++){
    for (int j = 0; j < nCols; j++){
        currentCell=minefield[i][j];
    }
}

since you use this code quite often, you should consider using a more proper type of data
Set<Cell> minefield maybe (or a map), and instead of accessing the array directly(see Locaction) you could get Cells with a proper method: getCellAt(Location location).

for(Cell currentCell: mineField){...}

naming issues

some examples where you should provide better names for variables:

  • Cell.nColIndex,nRowIndex; (aside from using a Location class), the proper names would be column and row (what is that n for? and the usage of Index should have been an alarm signal for unclean design
  • a never-ending story:
    for (int i = 0; i < nRows; i++){
          for (int j = 0; j < nCols; j++){}
    }
    
    i and j should be row and column, or maybe xand y (my personal all time issue)
  • GamePanel, where are the vocal? pnlProgress, lblTime, lblMines (and many many more)?
  • ONE_SECOND is really SECOND_IN_MILLIS to avoid misunderstandings.

code convention

  1. Method names are CamelCase, in java we don't use underscore.
  • field_init(); would be fieldInit();
  • icons_init(); would be iconsInit();
  1. brackets are at the end of the statement
if(condition){
    ...
}
  1. Always use braces for control flow structure
  • GameParamsWindow.getNumber: if(getText().isEmpty()) return 0;

improved programm flow

avoid complicated if/else structure, Game:

public void clickCell(Cell selectedCell){ 
    if(selectedCell.type==Cell.Type.MINED) {
        status=GameStatus.LOST;
        uncoverAllMines();
        //remove complicated else trail here!
        return;
    }
    
    // Clicked cell is clear
    uncoverCells(selectedCell);
    if(isVictory()) { // Check if there are any covered cells without mines
        status=GameStatus.VICTORY; // there aren't))
        flagAllMines(); 
    }
}

provide tests

sadly i do not see any tests, i would apprechiate to see those too... in goodwill i think you just didn't post them ;-)

use an IDE

the way your code is formatted shows that you don't use an IDE for Developement - or at least you do not use its fully potential. An IDE would help you to handle naming conventions and code formatting just by pressing one key.

summary

the whole code is full of shadows of procedural legacy ^_ ^. whenever you do any primitive access you should stop and think: why do i access primitives directly, why do i not use a proper method? methods are the heart of object orientated programming. the less you use them the further away you are from oop.

update - details on oop cell

how could an implementation of the neighbour-hood relation look like?

class Cell{
    private final Location location;

    Cell(Location location){
        this.location = location;
    }

    Set<Location> getSurroundedLocations(){
        return location.getSourroundingLocations();
    }
}
class Location{
    final int x;
    final int y;

    Location(int x, int y){
       this.x = x;
       this.y = y;
    }

    Set<Location> getSourroundingLocations(){
        return new HashSet<>(Arrays.asList(
            new Location(x-1, y-1),
            new Location(x-1, y),
            new Location(x-1, y+1),
            new Location(x, y-1),
            new Location(x, y+1),
            new Location(x+1, y-1),
            new Location(x+1, y),
            new Location(x+1, y+1));
    }
}
class Game{

    Set<Cell> minefield;  

    protected Set<Cell> getCellNeighbours(Cell myCell){
        return minefield.stream().
            filter(c -> myCell.getSourroundingLocations.contains(c.location)).
            collect(Collectors.toSet());
    }
}

update - details on "seperation of concerns"

The Separation of Concerns principle is closely connected to the Single Responsibility principle.

TL'DR put Things where they belong to - as a car, motor stuff comes to the motor class, gear stuff to the gear class.

part Cell
A Cell has attributes and behaviour - and they should be placed within the Cell class. A Cell is responsible to hold (or not) hold a mine. Its state can be covered, uncovered, flagged (and defused?). That is greatly solved by your model (attributes & enums are very nice).

But to work with this Cell there might be methods required. E.g. for getting the state (not the State enum) of the Cell. So if you want to handle a click on the Cell you might want to know if this Cell is not yet handled. so you would provide the specific method Cell.isUnhandled(), which returns true if it is either/or, otherwise returns false;

Another Example: To reveal the board you might want to reveal each Cell, so there might be a method uncover() which does exactly that. it does the magic of uncovering, where it happens. And it does even handle the fact, that a Cell might already have been uncovered. this is called single responsibility and the method is on the right place (Separation of concerns).

Bonus: Your cell is very testable now. Invovoke Cell.unCover() on different Cells and see if the uncover behaviour is as expected.

part Board
a Boardis responsible to assemble Cells. (no - it is not responsible to assemble Cells in Rows and Columns - Rows and Columns are never used on any action).

SideNote
You decided to abuse the Board as Game, something i'm not 100% in with. It violates the Single responsibility, a board is single/only responsible to manage the cells in a form of grid - nothing more, nothing less.

When you create or re-create your board have to distribute Cells on the Board. According to the principle of least astonishment i would expect to have a method setup() on the Board that does exactly that. Again, by doing the setup Board Magic on the Board we follow the Single Responsibility Principle and the Separation of Concerns.

If you think that the Board.setup() method does more than merely distribute the covered/uncovered Cells randomly over the board... then you might have an idea of a BoardGenerator in mind? In this case: do it! create a class BoardGenerator and add the BoardGenerator to the the Board and delegate the setup call to the Generator.

Delegation example:

class Board{//Game

    BoardGenerator generator;
    Set<Cell> cells;

    void setup(SetupParameters parameters){
        cells = generator.setup(parameters);
    }
}
calls 

already done nicely
you did very well on injection the setup Parameters. That part was done good in your code!

\$\endgroup\$
16
  • \$\begingroup\$ Please, tell me, how should the direction object work? (for getting neighbour cells) \$\endgroup\$ Commented Nov 1, 2021 at 12:10
  • \$\begingroup\$ i have updated my answer. \$\endgroup\$ Commented Nov 1, 2021 at 15:12
  • \$\begingroup\$ Thank you. P.S. Should I use separate getter and setter functions for cell properties like "mined", "flagged", etc? Or it might be a good idea to store all the such properties in single data structure (and use single getter and setter function passing parameter name to them))? \$\endgroup\$ Commented Nov 1, 2021 at 17:19
  • 1
    \$\begingroup\$ return minefield.stream().filter(c -> myCell.getSourroundingLocations.contains(c.location)) will call rows*cols times myCell.getSurroundingLocations(). Each time building the same HashSet with 9 elements ? \$\endgroup\$
    – Holger
    Commented Nov 2, 2021 at 8:43
  • 1
    \$\begingroup\$ @MartinFrank, could I also show you changed source code, please? I'll update the post soon. I've tried to take your remarks into account \$\endgroup\$ Commented Nov 2, 2021 at 17:19

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.