Battleship game - preliminary version

Question

I created preliminary version of battleship game. For now it works that: The computer generates a new a board and our task is to find all ships. I emphasize that I didn't focus on looks.

If anyone has a moment please look and give me feedback. I tried to create according to MVC pattern, but I don't know if I understood this well.

GitHub

package battleship;

import java.awt.EventQueue;

import javax.swing.JFrame;

public class GameCreator {

    public static void main (String[] args)
    {
        EventQueue.invokeLater(new Runnable()
        {
           public void run()
           {

               BoardView bv = new BoardView();
               JFrame frame = new JFrame();
               BoardModel s = new BoardModel();
               BoardController c = new BoardController(bv,s);
               frame.add(bv.getGui());   
               frame.pack();
               frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
               frame.setVisible(true);
           }
        });
    }

}

---

package battleship;

import java.awt.BorderLayout;

import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JToolBar;

public class Frame extends JFrame{

    public Frame()
    {
        add(new JLabel("GRACZ"), BorderLayout.NORTH);
        JToolBar tools = new JToolBar();
        tools.setFloatable(false);
        add(tools, BorderLayout.EAST);
        BoardView b = new BoardView();
        pack();
    }
}

---

package battleship;

import java.awt.Color;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.Insets;
import java.awt.image.BufferedImage;

import javax.swing.ImageIcon;
import javax.swing.JButton;
import javax.swing.border.LineBorder;

public class MyButton extends JButton{

    private int row;
    private int col;
    private int z;

    public MyButton(int row, int col, int z){
        super();
        this.row=row;
        this.col=col;
        this.z=z;

        setBorder(new LineBorder(Color.DARK_GRAY));
        ImageIcon icon = new ImageIcon(
                new BufferedImage(16, 16, BufferedImage.TYPE_INT_ARGB));
        setIcon(icon);
        Insets buttonMargin = new Insets(0,0,0,0);
        setMargin(buttonMargin);

        setBackground(Color.WHITE);
    }


    public int getRow(){
        return row;

    }

    public int getColumn(){
        return col;
    }

    public int getZ(){
        return z;
    }

    public void setZ(int z){
        this.z=z;
    }

}

---

package battleship;

import java.awt.*;
import java.awt.event.MouseListener;
import java.util.ArrayList;

import javax.swing.*;
import javax.swing.border.Border;
import javax.swing.border.EmptyBorder;
import javax.swing.border.LineBorder;

public class BoardView {

    private JPanel gui = new JPanel(new BorderLayout(5,5));
    private MyButton[][] myButtons = new MyButton[10][10];
    private final String COLS = "ABCDEFGHIJ";
    private final int size=10;


    public BoardView(){

        drawBoard();
    }

    public void drawBoard(){

        gui.setSize(1000,400);
        gui.setBorder(new EmptyBorder(5,5,5,5));
        gui.setBorder(new LineBorder(Color.BLACK));

        JToolBar tools = new JToolBar();
        tools.setFloatable(false);
        gui.add(tools, BorderLayout.PAGE_START);
        tools.add(new JButton("New")); // TODO - add functionality!

        JPanel board = new JPanel();
        board.setLayout(new GridLayout(11,11));
        board.setBorder(new LineBorder(Color.BLUE, 1));

        gui.add(board);

        for (int i=0; i<myButtons.length; i++){
            for (int j=0; j<myButtons[i].length; j++){
                myButtons[i][j]=new MyButton(i,j,0);

            }   
        }

        board.add(new JLabel());
        for (int ii = 0; ii < 10; ii++) {
            board.add(new JLabel(COLS.substring(ii, ii + 1),
                    SwingConstants.CENTER));
        }

        for (int ii = 0; ii < size; ii++) {
            for (int jj = 0; jj < size; jj++) {
                switch (jj) {
                    case 0:
                        board.add(new JLabel("" + (ii + 1),
                                SwingConstants.CENTER));
                    default:
                        board.add(myButtons[ii][jj]);
                }
            }
        }       


    }

    public JPanel getGui(){
        return gui;
    }

    public MyButton getButton(int x, int y){
        return myButtons[x][y];

    }

    public void addListener(MouseListener mouseListener){

        for (int i=0; i<myButtons.length; i++)
            for (int j=0; j<myButtons[i].length;j++)
                myButtons[i][j].addMouseListener(mouseListener);
    }

    public void paintShip(ArrayList<MyButton> mybuttons){
        for(int i =0; i<mybuttons.size(); i++)
            mybuttons.get(i).setBorder(new LineBorder(new Color(50, 220, 30)));
    }

    public void paintEmpty(MyButton mybutton){
        mybutton.setIcon(new ImageIcon("yellow-ball.gif"));
        mybutton.setDisabledIcon(new ImageIcon("yellow-ball.gif"));
    }

    public void paintMast(MyButton mybutton){
        mybutton.setBorder(new LineBorder(Color.RED));
    }

    public void paintX(MyButton mybutton){
        mybutton.setIcon(new ImageIcon("krzyżyk.jpg"));
        mybutton.setDisabledIcon(new ImageIcon("krzyżyk.jpg"));
    }

    public void paintStart(MyButton mybutton){
        mybutton.setIcon(new ImageIcon());

    }
    public void endGame(){
        JOptionPane.showMessageDialog(gui, "All Ships are sunken!");
    }



}

---

package battleship;

import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;
import java.awt.event.MouseListener;
import java.util.ArrayList;

import javax.swing.AbstractButton;

public class BoardController {
    private BoardView view;
    private BoardModel model;
    private ArrayList<ArrayList<MyButton>> buttonsList;

    public void createButtonsList(){
        buttonsList=new ArrayList<ArrayList<MyButton>>();
        for(int i=0; i<=9; i++){
            buttonsList.add(new ArrayList<MyButton>());
        }
    }

    public BoardController(BoardView bv, BoardModel s ){

        this.view = bv;
        this.model= s;
        view.addListener(new MyListener());
        createButtonsList();

    }

    public class MyListener extends MouseAdapter{

        public void mouseClicked(MouseEvent e){

            if(e.getButton() == 1){

                if (!((MyButton)e.getSource()).isEnabled()) {
                    return;
                }

                showField((MyButton)e.getSource());    

            }
            else if(e.getButton()==3)
            {
                if(!((MyButton)e.getSource()).isEnabled() )
                    return;
                flagEmpty((MyButton)e.getSource());
            }
        }
    }

    public void showField(MyButton mybutton){


        boolean t =model.checkMove(mybutton.getRow(),mybutton.getColumn());
        if(t){
            view.paintX(mybutton);
            int iter = model.getShipNumber(mybutton.getRow(), mybutton.getColumn());
            addButtonToShip(mybutton, iter);
            if(model.isSunk(iter)){
                view.paintShip(buttonsList.get(iter));
                if(model.isWinner())
                    view.endGame();
            }

            else view.paintMast(mybutton);
        }
        else view.paintEmpty(mybutton);
        mybutton.setEnabled(false);
    }

    public void flagEmpty(MyButton mybutton){
        if(mybutton.getZ()==0){
            view.paintEmpty(mybutton);
            mybutton.setZ(1);
        }
        else if(mybutton.getZ()==1){
            view.paintStart(mybutton);
            mybutton.setZ(0);
        }
    }


    public void addButtonToShip(MyButton mybutton, int i){

        buttonsList.get(i).add(mybutton);
    }



}

---

package battleship;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.Random;

import battleship.Field.State;

public class BoardModel {

    private static int currentAmount;
    private static Field[][] fields = new Field[10][10];
    private Random generator = new Random();
    private ArrayList<ArrayList<Field>> shapesList;
    private static ArrayList<Field> list1;
    private static ArrayList<Field> list2;

    {
    int i, j;
    for(i=0; i<10; i++){
        for(j=0; j<10; j++){
            fields[i][j]=new Field(i,j);
        }
    }
    }

    public BoardModel (){
        shapesList = new ArrayList<ArrayList<Field>>();
        shapesList.add(createShip(4));
        shapesList.add(createShip(3));
        shapesList.add(createShip(3));
        shapesList.add(createShip(2));
        shapesList.add(createShip(2));
        shapesList.add(createShip(2));
        shapesList.add(createShip(1));
        shapesList.add(createShip(1));
        shapesList.add(createShip(1));
        shapesList.add(createShip(1));

    }
    public ArrayList<Field> createShip(int mastsAmount)
    {
        list1 = new ArrayList<>();

        do {
            currentAmount = 0;
            int x = generator.nextInt(10);
            int y = generator.nextInt(10);

            if (fields[x][y].equalsState(State.Available)){

                    currentAmount=1;
                    fields[x][y].setState(State.Ship);
                    list1.add(fields[x][y]);
                    if(mastsAmount>1){
                        list2 = new ArrayList<>();
                        addMast(x,y, mastsAmount);
                    }
            }
        }while(!(list1.size()==mastsAmount));

        for(int i = 0; i<list1.size(); i++)
        {
            for(int j =list1.get(i).getX()-1; j<=list1.get(i).getX()+1; j++)
                if (j>=0 && j<=9)
                    for( int k = list1.get(i).getY()-1; k<=list1.get(i).getY()+1; k++)
                        if(k>=0 && k<=9)
                            if(!fields[j][k].equalsState(State.Ship)){

                                fields[j][k].setState(State.Unavailable);
                            }
        }

        return list1;
    }

    public ArrayList<Field> addMast(int x, int y, int mastsAmount){

        //add all potential masts to list2
        for (int i = x-1; i<=x+1 ; i++){
            if (i>=0 && i<=9)
                for(int j= y-1; j<=y+1; j++)
                    if(j>=0 && j<=9)
                        if(((i!=x && j==y) || (i==x && j!=y)) && fields[i][j].equalsState(State.Available)){

                            list2.add(fields[i][j]);
                        }
        }   

        if (list2.size()>0){

            int r = generator.nextInt(list2.size());
            list2.get(r).setState(State.Ship);
            list1.add(list2.get(r));
            currentAmount++;
            if(currentAmount<mastsAmount){
                list2.remove(r);
                addMast(list1.get(currentAmount-1).getX(),list1.get(currentAmount-1).getY(), mastsAmount);
            }
        }
        else{

            for(int i = 0; i<list1.size(); i++)
            {
                list1.get(i).setState(State.Available);
            }
        }
        return list1;
    }

    public boolean checkMove(int x, int y){

        if( fields[x][y].equalsState(State.Ship))
            return true;
        else return false;
    }

    public int getShipNumber(int x, int y){
        int i=0;
        int iter=0;
        for(Iterator<ArrayList<Field>> iterShip =shapesList.iterator(); iterShip.hasNext();){
            i++;
            ArrayList list = iterShip.next();
            for(Iterator<Field> iterField = list.iterator(); iterField.hasNext();){
                Field field = iterField.next();
                if(field.getX()==x && field.getY()==y){
                    iterField.remove();
                    iter = i-1;
                }
            }
        }
        return iter;
    }

    public boolean isSunk(int i){
        if( shapesList.get(i).isEmpty()){
            shapesList.remove(i);
            return true;
        }else return false;
    }

    public boolean isWinner(){
        return shapesList.isEmpty();
    }

}

---

package battleship;

public class Field {

    private int x;
    private int y;
    public enum State{
        Ship ("Ship"),
        Available("Available"),
        Unavailable("Unavailable"),
        PShip("PShip");

        private String text;
        private State(String text){
            this.text=text;
        }

        public String toString(){
            return text;
        }
    }

    private State state;

    public Field(int x, int y){
        this.x = x;
        this.y = y;
        this.state=State.Available;


    }

    public boolean equalsState(State s){
        if(this.getState()==s) return true;
        else return false;
    }
    public void setState(State s){
        this.state = s;
    }

    public State getState(){

        return this.state;
    }

    public int getX(){
        return this.x;
    }

    public int getY(){
        return this.y;
    }



}

Answer 1

1. The name MyButton doesn't really say anything. What is that class made for? Make the name more descriptive.
2. Using col, row and z isn't the greatest idea. I'd instead do x, y, z.
3. Why do you initialize the game in EventQueue.invokeLater?

4. You could change this code

   for (int jj = 0; jj < size; jj++) {
       switch (jj) {
           case 0:
               board.add(new JLabel("" + (ii + 1),
                   SwingConstants.CENTER));
           default:
               board.add(myButtons[ii][jj]);
       }
   }
   

   to this

   board.add(new JLabel("" + (ii + 1), SwingConstants.CENTER));
   for (int jj = 0; jj < size; jj++) {
       board.add(myButtons[ii][jj]);
   }
   

   Improving performance and readability

5. Not really too important but I'd move JFrame frame = new JFrame() under BoardController c = new BoardController(bv,s). That way you'll have only the relevant to BoardController code above it and only JFrame related under it. This should improve readability.

6. In BoardView#drawBoard why do you have ii and jj? You should name it i and j respectively unless you have a reason to do so, but it doesn't look like it.
7. paintShip should accept a List. Accepting only ArrayLists unnecessarly couples your code. And you could use a for-each loop instead
8. BoardController shouldn't use ArrayLists either as it can accept just Lists.
9. I stopped checking the code until Field. Why doesn't your State enum use the default toString?
10. I don't really think your equalsState method does any good. Is it really too hard to write x.getState() == someState?

Answer 2

There is some remarks abour the naming and formats of your classes. But in my opnion the major remark is about the structure of your code.

Regarding your class names, I assume that you have tried to use the MVC pattern. However, to be perfect, the controller should not know anything about the view. The controller changes the model, the model update the view, and the view use (via the user) the controller.

So when your user click on a cell, the view should invoke a method on the controller. Inside this method, the controller query and update the model (if there is a ship, marke the ship as hit). Once updated the model notify that view that he has been changed, and the view redraw itself.

This will simplify your code because the view only display cells, translate the click to a cell coordinate and call the controller. Your view can be a simple JComponent that override paintComponent to draw cells by querying the model.

The controller contains the logic, when he receive a click from the view, it verifies if there is a boat at the given location from the model. And, if there is one, he mark it as hit via the model.

The model maintains a set of boat and a boat is reponsible to maintains his location, hits and decide if he is sunk or not.

I would represent Boat as one array of Point and one array of Boolean where the boolean at x is set to true when the user click on the point at x. When all cells are true, the boat is sunk. It encapsulate everything behind some clear methods like setHitAt, isHitAt and isSunk.

With that, you remove many of the classes related to the UI and concentrate only on the logic and the model that can be tested easily.

public class GameController {
    private final GameModel model;

    public GameController(final GameModel model) {
        this.model = model;
    }

    public void onTest(int col, int row) {
        if ( model.hasShipAt(col, row) ) {
            model.setHitAt(col, row);
        }
    }
}



public class GameModel {
    private final Set<Consumer<GameModel>> observers = new HashSet<>();
    private final Set<Boat> boats = new HashSet<>();

    public GameModel() {
        // Create and set boats
    }

    public boolean hasShipAt(int col, int row) {
        return boatAt(col, row) != null;
    }

    public boolean isSunkAt(int col, int row) {
        return boatAt(col, row).isSunk();
    }

    public boolean isHitAt(int col, int row) {
        return boatAt(col, row).isHitAt(col, row);
    }

    public void setHitAt(int col, int row) {
        boatAt(col, row).setHitAt(col, row);
        notifyChange();
    }

    private void notifyChange() {
        observers.forEach(c->c.accept(this));
    }

    public void register(Consumer<GameModel> observer) {
        this.observers.add(observer);
    }       

    private final Boat boatAt(int col, int row) {
        for (Boat boat : boats) {
            if (boat.isAt(col, row)) {
                return boat;
            }
        }
        return null; // because this is private method, otherwise Optional
    }

    private class Boat {
        private final Point[] points;
        private final Boolean[] hits;

        public Boat(Point[] points) {
            this.points = points;
            this.hits = new Boolean[points.length];
            Arrays.fill(hits, false);
        }

        private boolean isAt(int col, int row) {
            for (Point point : points) {
                if (point.x == col && point.y == row)
                    return true;
            }
            return false;
        }

        public void setHitAt(int col, int row) {
            for (int i = 0;i < points.length;i++) {
                if (points[i].x == col && points[i].y == row) {
                    hits[i] = true;
                }
            }
        }

        public boolean isHitAt(int col, int row) {
            for (int i = 0;i < points.length;i++) {
                if (points[i].x == col && points[i].y == row) {
                    return hits[i];
                }
            }
            return false;
        }

        public boolean isSunk() {
            boolean sunk = true;
            for (Boolean hit : hits) {
                sunk &= hit;
            }
            return sunk;
        }
    }
}



public class GameView extends JPanel {
    private static final Rectangle CELL = new Rectangle(16, 16);
    private static final int SIZE = 10;

    private final GameController ctrl;
    private final GameModel model;

    public GameView(final GameController ctrl, final GameModel model) {
        // ..
        addMouseListener(new MouseAdapter() {
            @Override
            public void mouseClicked(MouseEvent event) {
                int col = event.getX()/CELL.width;
                int row = event.getY()/CELL.height;
                ctrl.onTest(col, row);
            }
        });
        this.model = model;
        this.ctrl = ctrl;

        model.register(m -> repaint());
    }

    // EDITED

    /* Invoked by Swing when the component must be `repaint`ed */
    protected void paintComponent(Graphics graphics) {
        Graphics2D g2d = (Graphics2D) graphics;
        g2d.setColor(Color.BLACK);
        for (int row = 0;row <= SIZE;row++) {
            for (int col = 0;col <= SIZE;col++) {
                paintCellAt(col, row, g2d);
            }
        }
    }

    private void paintCellAt(int col, int row, Graphics2D g2d) {
        Rectangle cell = new Rectangle(col * CELL.width, row * CELL.height,
            CELL.width, CELL.height);
        if ( !model.hasShipAt(col, row) ) {
            g2d.setColor(Color.BLACK);
            g2d.draw(cell);
        } else if ( model.isSunkAt(col, row) ) {
            g2d.setColor(Color.BLACK);
            g2d.fill(cell);
        } else if ( model.isHitAt(col, row) ) {
            g2d.setColor(Color.GRAY);
            g2d.fill(cell);
        } else {
            g2d.setColor(Color.BLACK);
            g2d.draw(cell);
        }
    }
}