Java Battleship game

Question

I have been given a problem statement to create a Battleship game in java.

My working code (Spring Boot+Web) is placed here along with the problem statement. https://github.com/ankidaemon/BattleShip

This question is majorly focused on design, please help me figure out, how can I make it decoupled and apply suitable design patterns.

StartGame.java - getting called from controller

@Component
public class StartGame {

    private static final Logger logger = LoggerFactory.getLogger(StartGame.class);

    public String init(File inputFile) throws FileNotFoundException, InputException {
        // TODO Auto-generated method stub
        ArrayList<BattleShips> p1s = new ArrayList<BattleShips>();
        ArrayList<BattleShips> p2s = new ArrayList<BattleShips>();
        int areaWidth = 0;
        int areahight = 0;
        ArrayList<Coordinate> player1missiles = null;
        ArrayList<Coordinate> player2missiles = null;
        try{
            Scanner sc = new Scanner(inputFile);

            areaWidth = sc.nextInt();
            if(areaWidth>9 || areaWidth<1){
                raiseException("Supplied area width is invalid.",sc);
            }
            areahight = sc.next().toUpperCase().charAt(0) - 64;
            if(areahight>25 || areahight<0){
                raiseException("Supplied area height is invalid.",sc);
            }
            sc.nextLine();
            int noOfships = sc.nextInt();
            if(noOfships>areahight*areaWidth || noOfships<1){
                raiseException("Supplied no of ships is invalid.",sc);
            }
            sc.nextLine();
            for (int j = 0; j < noOfships; j++) {
                char typeOfShip = sc.next().toUpperCase().charAt(0);
                if(typeOfShip!='P' && typeOfShip!='Q'){
                    raiseException("Supplied type of ship is invalid.",sc);
                }
                int shipWidth = sc.nextInt();
                if(shipWidth>areaWidth || shipWidth<0){
                    raiseException("Supplied ship width is invalid.",sc);
                }
                int shiphight = sc.nextInt();
                if(shiphight>areahight || shiphight<0){
                    raiseException("Supplied ship height is invalid.",sc);
                }
                BattleShips ship;
                for (int i = 0; i <= 1; i++) {
                    char[] locCharArr = sc.next().toUpperCase().toCharArray();
                    int[] loc = new int[2];
                    loc[0] = locCharArr[0] - 65;
                    loc[1] = locCharArr[1] - 49;
                    if(loc[0]>areahight || loc[0]<0 || loc[1]>areaWidth || loc[1]<0){
                        raiseException("Supplied ship location is invalid.",sc);
                    }
                    ship = new BattleShips(shipWidth, shiphight, typeOfShip, loc);
                    if (i % 2 == 0)
                        p1s.add(ship);
                    else
                        p2s.add(ship);
                }
                sc.nextLine();
            }

            player1missiles = returnMissileCoordinates(sc.nextLine());
            player2missiles = returnMissileCoordinates(sc.nextLine());
            sc.close();
        }catch(InputMismatchException e){
            throw new InputException("Invalid Input supplied.",ErrorCode.INVALIDINPUT);
        }
        BattleArea player1 = new BattleArea("player1", areaWidth, areahight, p1s);
        BattleArea player2 = new BattleArea("player2", areaWidth, areahight, p2s);

        player1.placeShips();
        player2.placeShips();

        while (!player1.isLost() && !player2.isLost()) {
            for (int i = 0; i < player1missiles.size();) {
                Coordinate c = player1missiles.get(i);
                while (player1.fireMissile(c, player2)) {
                    player1missiles.remove(i);
                    if (i < player1missiles.size()) {
                        c = player1missiles.get(i);
                    } else
                        break;
                }
                if (player1missiles.size() > 0) {
                    player1missiles.remove(i);
                }
                break;
            }
            for (int j = 0; j < player2missiles.size();) {
                Coordinate c = player2missiles.get(j);
                while (player2.fireMissile(c, player1)) {
                    player2missiles.remove(j);
                    if (j < player2missiles.size()) {
                        c = player2missiles.get(j);
                    } else
                        break;
                }
                if (player2missiles.size() > 0) {
                    player2missiles.remove(j);
                }
                break;
            }
        }

        if (player1.isLost()) {
            logger.info("-------------------------");
            logger.info("Player 2 has Won the Game");
            logger.info("-------------------------");
            return "Player 2 has Won the Game";
        } else {
            logger.info("-------------------------");
            logger.info("Player 1 has Won the Game");
            logger.info("-------------------------");
            return "Player 1 has Won the Game";
        }
    }

    private static ArrayList<Coordinate> returnMissileCoordinates(String nextLine) {
        // TODO Auto-generated method stub
        ArrayList<Coordinate> tmp = new ArrayList<Coordinate>();
        String[] arr = nextLine.split("\\ ");
        Coordinate tmpC;
        for (String s : arr) {
            char[] charArr = s.toCharArray();
            tmpC = new Coordinate(charArr[1] - 49, charArr[0] - 65);
            tmp.add(tmpC);
        }
        return tmp;
    }

    private void raiseException(String message, Scanner sc) throws InputException {
        sc.close();
        throw new InputException(message, ErrorCode.INVALIDINPUT);
    }
}

BattleArea.java

public class BattleArea {

private static final Logger logger = LoggerFactory.getLogger(BattleArea.class);

private String belongsTo;
private int width,height;
private ArrayList<BattleShips> battleShips;
private Set<Coordinate> occupied=new TreeSet<Coordinate>();
private int[][] board=null;
private boolean lost=false;

public BattleArea(String belongsTo, int width, int height, ArrayList<BattleShips> battleShips) {
    super();
    this.belongsTo = belongsTo;
    this.width = width;
    this.height = height;
    this.battleShips = battleShips;
    this.board=new int[this.width][this.height];
}

public void placeShips(){
    for(BattleShips ship:this.battleShips){
        int x=ship.getLocation()[1];
        int y=ship.getLocation()[0];
        if(ship.getWidth()+x>this.width || ship.getHeight()+y>this.height){
            logger.error("Coordinate x-"+x+" y-"+y+" for "+this.belongsTo+" is not avilable.");
            throw new ProhibitedException("Ship cannot be placed in this location.",ErrorCode.OUTOFBATTLEAREA);
        }else{
            Coordinate c=new Coordinate(x, y);
            if(occupied.contains(c)){
                logger.error("Coordinate x-"+c.getX()+" y-"+c.getY()+" for "+this.belongsTo+" is already occupied.");
                throw new ProhibitedException("Ship cann't be placed in this location.",ErrorCode.ALREADYOCCUPIED);
            }else{
                Coordinate tempC;
                for(int i=x;i<ship.getWidth()+x;i++){
                    for(int j=y;j<ship.getHeight()+y;j++){
                        logger.debug("Placing at x-"+i+" y-"+j+" for "+this.belongsTo);
                        tempC=new Coordinate(i, j);
                        occupied.add(tempC);
                        if(ship.getTypeOfShip()=='P'){
                            board[i][j]=1; 
                        }else if(ship.getTypeOfShip()=='Q'){
                            board[i][j]=2;
                        }
                    }
                }
            }
        }
    }
}

public boolean fireMissile(Coordinate c, BattleArea enemyBattleArea){
    int x=c.getX();
    int y=c.getY();
    logger.info("Firing at "+enemyBattleArea.belongsTo+" x-"+x+" y-"+y+" :");
    if(enemyBattleArea.board[x][y]!=0){
        if(enemyBattleArea.board[x][y]==-1){ 
            logger.debug("Already blasted!");
            return false;
        }
        else if(enemyBattleArea.board[x][y]==1){
            Coordinate temp=new Coordinate(x,y);
            enemyBattleArea.occupied.remove(temp);
            enemyBattleArea.board[x][y]=-1;
            if(enemyBattleArea.occupied.size()==0){
                enemyBattleArea.setLost(true);
            }
            logger.debug("Suucessfully blasted!!");
            return true;
        }else{
            enemyBattleArea.board[x][y]=enemyBattleArea.board[x][y]-1;
            logger.debug("Half life left!!");
            return true;
        }
    }else{
        logger.debug("Missed");
        return false;
    }
}

public boolean isLost() {
    return lost;
}

public void setLost(boolean lost) {
    this.lost = lost;
}
}

BattleShips.java

public class BattleShips {

private int width,height;
private char typeOfShip;
private int[] location;

public BattleShips(int width, int height, char typeOfShip, int[] loc) {
    super();
    this.width = width;
    this.height = height;
    this.typeOfShip = typeOfShip;
    this.location = loc;
}

public int getWidth() {
    return width;
}

public int getHeight() {
    return height;
}

public char getTypeOfShip() {
    return typeOfShip;
}

public int[] getLocation() {
    return location;
}

}

Coordinate.java

public class Coordinate implements Comparable<Coordinate> {

private int x,y;

public Coordinate(int x, int y) {
    super();
    this.x = x;
    this.y = y;
}

@Override
public String toString() {
    return "Coordinate [x=" + x + ", y=" + y + "]";
}

@Override
public int compareTo(Coordinate o) {
    // TODO Auto-generated method stub
    if(this.x==o.x && this.y==o.y)
        return 0;
    else if(this.x<o.x && this.y<o.y)
        return -1;
    else
        return 1;
}

public int getX() {
    return x;
}

public int getY() {
    return y;
}

}

Sample Input

5 E

2

Q 1 1 A1 B2

P 2 1 D4 C3

A1 B2 B2 B3

A1 B2 B3 A1 D1 E1 D4 D4 D5 D5

Rules
1. Player1 will fire first. Each player will get another chance till ( hit == successful ).
2. Battleships will be placed horizontally.
3. Type-Q ship requires 2 missiles hit to get destroyed.
4. Type-P ship requires 1 missile hit to get destroyed.

Input
First line of the input contains dimensions of battle area having width and height separated by space.
Second line will have number (B) of battleships each player has.
Then in the next line battleship type, dimensions (width and height) & positions (Y coordinate and X coordinate) for Player-1 and then for Player-2 will be given separated by space.
And then in the next line Player-1’s sequence (separated by space) of missiles target location coordinates (Y and X) will be given and then for sequence for Player-2.

Constraints:
1 <= Width of Battle area (M) <= 9
A <= Height of Battle area (N) <= Z
1 <= Number of battleships <= M * N
Type of ship = {‘P’, ‘Q’}
1 <= Width of battleship <= M
A <= Height of battleship <= N
1 <= X coordinate of ship <= M
A <= Y coordinate of ship <= N

Answer 1

ok lets put hands on:

Class name for your StartGame is not helpful, rename it into a more matching name, i think like BattleShipGame and start the game instead from your controller

BattleShipGame game = new BattleShipGame();
game.start();

the init - method is far to big and it does no init but does even more things... so let's break that down a bit:

• init should return a boolean (or a Result) that indicates that init was successful.
• init looks like it's a delegate methode wich means ther should be very little logic insde - instead it is useful to put most work into methods
• just init things and don't do any other things
• use Player objects...
• move the game logic out of the method

it could look like this then

private Player playerOne;
private Player playerTwo;

public boolean init(){ 

    playerOne = new Player("player1");
    playerTwo = new Player("player2");
    GameSetup setup = readFile(inputFile);

    ArrayList<BattleShips> p1bs = setup.getFirstBattleShips();                
    ArrayList<BattleShips> p2bs = setup.getSecondBattleShips();        
    playerOne.setBattleShips(p1bs);
    playerTwo.setBattleShips(p2bs);

    playerOne.setMissiles(setup.getFirstMissileCoordinates());
    playerTwo.setMissiles(setup.getSecondMissileCoordinates());

    playerOne.setBoard(new BattleShipBoard(setup.getDimension());
    playerTwo.setBoard(new BattleShipBoard(setup.getDimension());

    playerOne.placeShips();
    playerTwo.placeShips();

    return true; 
}

NOTE: the init method could be shortenend far more, but i think i point out in a good way what init should really do...

as mentioned above you have moved the game logic out of your init method and put it in the playGame() method.

public Result playGame(){
    Result result = new Result();
    Scores score = new Score();
    while (!player1.isLost() && !player2.isLost()) {
        for (int i = 0; i < player1missiles.size();) {
            ... 
        }            
    }
    result.setWinner(playerOne);
    result.setScore(scores);
    return result;
}

the BattleShipGame would start now this manner:

public void start(){
    init();
    Result result = playGame();        
    ... //do whatever you want with your result - for example print it into the console
}

for your BattleShip there are some more issuses wich can be talked about. I think it was a very good idea to use a class Coordinate which looks good at first glance. But you don't use it consequencically. think about how it would be if you used Coordinate for you ship instead of int[] that would make your code as well easier to read and the math would be much easier. And don't use a char for your shiptype, use a enum instead. But let's be honest, you don't have a 'position and width and height' what you really have is an rectangle - so use an rectangle!

public class BattleShips {

    private ShipType shipType;
    private Rectangle bounds;
    private int lifePoints;

    public BattleShips(ShipType typeOfShip, Coordinate pos) {
        super();
        shipType = typeOfShip;
        bounds = new Rectangle(shipType.getDimension, pos);
        lifePoints = shipType.getLifePoints();
    }

    public Rectangle getBounds() {
        return bounds();
    }

    ...
}

the dimension of the Rectangle (width/height) and the amount of lifepoints can be determind by the ShipType

public Enum Shiptype {
    DESTROYER(2,4,2), SUBMARINE(1,3,1), ...; //don't use shiptype P or shiptype Q

    private final Dimension dimension;
    final int lifePoints;

    public ShipType(int w, int h, int life){
        dimension = new Dimension(w,h);
        lifePoints = life;
    }

    public Dimension getDimension(){
        return dimension;
    }

    public int getLifePoints(){
        return lifePoints();
    }
}

The BattleArea is now far more easy to use, think about how simple you can placeShips now:

public class BattleArea {

    private Player owner;
    private Rectangle boardBounds;
    private List<BattleShips> battleShips;
    private List<Coordinates> board;

    public BattleArea(Player owner, Rectangle bounds, List<BattleShips> battleShips) {
        super();
        this.owner = owner;
        this.dimension = dimension;
        this.battleShips = battleShips;
        board = createBoard();
    }

    public void placeShips(){
        List<BattleShip> placedShips = new ArrayList<>();
        for(BattleShips ship:this.battleShips){
            Bound shipBounds = ship.getBounds();
            if(!boardBounds.contains(shipBounds)){
                throw new ProhibitedException(
                    "Ship cannot be placed in this location.",ErrorCode.OUTOFBATTLEAREA);
            }

            for (BattleShip placedShip: placedShips){
                if (bounds.intersects(placedShip.getBounds()){
                    throw new ProhibitedException(
                       "Ship cann't be placed in this location.",ErrorCode.ALREADYOCCUPIED);   
                }
            }
            placedShips.add(battleShip);
        }        
    }

    public boolean fireMissile(Coordinate c, BattleArea enemyBattleArea){
        BattleShip shipAt = enemyBattleArea.getShipAt(c);
        if(shipAt == null){
            return false;
        }else{
            handleDamge(shipAt, enemyBattleArea);
            return true;
        }
    }

    private void handleDamage(BattleShip opponent, BattleArea area){
        int lifePointsLeft = opponent.getLifePoints() - 1; //hardcoded damage (that's bad)
        if(lifPoints > 0){
            //Log damage done
        }else{ 
            //log destroyed
            area.removeBattleShip(opponent);
        }    
    }
}

all code above has not been compiled so there may be some spelling errors and a lot of methods are not even implemented yet (like Rectangle.contains() or others).

summary

but let's look at what we have now:

• you can change the ship type quite easily without modifing any code !!! (you simply have to add another shiptype in ShipType )
• you have reduced the complexity of your code very far, you don't have dangerous calculations.
• you have seperated concerns, the objects now do what they are supposed to do
• you could easily change your code for another player (three-player game)
• you could test your code now

Answer 2

To decouple you have to make sure that your functions don't need to call each other to do their work. If possible, the only functions that should call others are the driver functions of each subsystem that are intended to be called like the API interface.

Think about how you would need to port a lot of code just for adding an useful function if that function calls other functions or uses variables from other big subsystems. If you make additional effort to implement that function in a way that it doesn't depend on anything else, even if it looks like duplicated code, you will be able to individually port it to another program, and even more if you don't make it depend on language features or library features that are not present on every single compiler and programming language you use to make it possible to port any code you write to any environment you need, that's what's called decoupling.

As you can see, decoupling can be appplied at the compiler, language, system environment, functions and subsystem levels. It might involve duplicating code and rewriting to have stand-alone, dependency-less routines. It might also imply using more widely-standardized features to make the code more portable, and it could also need that you work on implementing or porting missing functionality to all your programming/system environments yourself so that no matter the system/language/compiler you use, you will always have the same functionality available.

---

---

---

---

About design patterns.

If you want to make your code highly reusable and if you want it to last for decades, you could use the low level approach of CPU assembly programming.

Think about a task or microtask you want to perform in a way that will always take the same type of parameters and that will always return a result in the exact same way.

Then give it a VERY specific name to this routine. This will be an opcode, an instruction, implemented as a function/subroutine, that you can reuse just like any other native CPU instruction. This way of designing the code is highly reusable and stable. If you want a variation in what to do, you just add a new opcode function instead of destroying the previous valid functionality.

Applying this throughout the program as the main design approach can make the code more strictly structured yet easier to follow.