OOP Battleship console game in Java

Question

My Second take on this can be found here

I wanted to make a simple console game in order to practice OOP. I would really appreciate a review that looks at readability, maintenance, and best practices.

What annoys me a little bit with this code is I don't use interfaces, abstract classes, or inheritance, but I couldn't find a good use case for them here.

Board.java

package com.tn.board;

import com.tn.constants.Constants;
import com.tn.ship.Ship;
import com.tn.utils.Position;
import com.tn.utils.Utils;

import java.awt.Point;
import java.util.Scanner;


public class Board {
    private static final Ship[] ships;
    private char[][] board;

    /**
     * Initialize ships (once).
     *
     */

    static {
        ships = new Ship[]{
                new Ship("Carrier", Constants.CARRIER_SIZE),
                new Ship("Battleship", Constants.BATTLESHIP_SIZE),
                new Ship("Cruiser", Constants.CRUISER_SIZE),
                new Ship("Submarine", Constants.SUBMARINE_SIZE),
                new Ship("Destroyer", Constants.DESTROYER_SIZE)
        };
    }


    /**
     * Constructor
     */
    public Board() {
        board = new char[Constants.BOARD_SIZE][Constants.BOARD_SIZE];

        for(int i = 0; i < Constants.BOARD_SIZE; i++) {
            for(int j = 0; j < Constants.BOARD_SIZE; j++) {
                board[i][j] = Constants.BOARD_ICON;
            }
        }

        placeShipsOnBoard();
    }


    /**
     * Target ship ship.
     *
     * @param point the point
     * @return ship
     */
    public Ship targetShip(Point point) {
        boolean isHit = false;
        Ship hitShip = null;
        for(int i = 0; i < ships.length; i++) {
            Ship ship = ships[i];
            if(ship.getPosition() != null) {
                if(Utils.isPointBetween(point, ship.getPosition())) {
                    isHit = true;
                    hitShip = ship;
                    break;
                }

            }
        }
        final char result = isHit ? Constants.SHIP_IS_HIT_ICON : Constants.SHOT_MISSED_ICON;
        updateShipOnBoard(point, result);
        printBoard();

        return (isHit) ? hitShip : null;
    }

    /**
     * Place ships on board.
     */
    private void placeShipsOnBoard() {
        System.out.printf("%nAlright - Time to place out your ships%n%n");
        Scanner s = new Scanner(System.in);

        for(int i = 0; i < ships.length; i++) {
            Ship ship = ships[i];
            boolean isShipPlacementLegal = false;

            System.out.printf("%nEnter position of %s (length  %d): ", ship.getName(), ship.getSize());

            while(!isShipPlacementLegal) {
                try {
                    Point from = new Point(s.nextInt(), s.nextInt());
                    Point to = new Point(s.nextInt(), s.nextInt());

                    while(ship.getSize() != Utils.distanceBetweenPoints(from, to)) {
                        System.out.printf("The ship currently being placed on the board is of length: %d. Change your coordinates and try again",
                                ship.getSize());

                        from = new Point(s.nextInt(), s.nextInt());
                        to = new Point(s.nextInt(), s.nextInt());
                    }
                    Position position = new Position(from, to);

                    if(!isPositionOccupied(position)) {
                        drawShipOnBoard(position);
                        ship.setPosition(position);
                        isShipPlacementLegal = true;
                    } else {
                        System.out.println("A ship in that position already exists - try again");
                    }

                } catch(IndexOutOfBoundsException e) {
                    System.out.println("Invalid coordinates - Outside board");
                }
            }
        }

    }


    private void updateShipOnBoard(Point point, final char result) {
        int x = (int) point.getX() - 1;
        int y = (int) point.getY() - 1;
        board[y][x] = result;
    }

    /**
     *
     * @param position
     * @return
     */
    private boolean isPositionOccupied(Position position) {
        boolean isOccupied = false;
        Point from = position.getFrom();
        Point to = position.getTo();

        outer:
        for(int i = (int) from.getY() - 1; i < to.getY(); i++) {
            for(int j = (int) from.getX() - 1; j < to.getX(); j++) {
                if(board[i][j] == Constants.SHIP_ICON) {
                    isOccupied = true;
                    break outer;
                }
            }
        }


        return isOccupied;
    }

    /**
     *
     * @param position
     */
    private void drawShipOnBoard(Position position) {
        Point from = position.getFrom();
        Point to = position.getTo();
        for(int i = (int) from.getY() - 1; i < to.getY(); i++) {
            for(int j = (int) from.getX() - 1; j < to.getX(); j++) {
                board[i][j] = Constants.SHIP_ICON;
            }
        }
        printBoard();
    }


    /**
     * Print board.
     */
    private void printBoard() {
        System.out.print("\t");

        for(int i = 0; i < Constants.BOARD_SIZE; i++) {
            System.out.print(Constants.BOARD_LETTERS[i] + "\t");
        }

        System.out.println();

        for(int i = 0; i < Constants.BOARD_SIZE; i++) {
            System.out.print((i+1) + "\t");
            for(int j = 0; j < Constants.BOARD_SIZE; j++) {
                System.out.print(board[i][j] + "\t");
            }
            System.out.println();
        }
    }

}

Constants.java

package com.tn.constants;


public class Constants {

    private Constants() {}

    public static final int PLAYER_LIVES = 17; //sum of all the ships

    public static final int CARRIER_SIZE = 5;
    public static final int BATTLESHIP_SIZE = 4;
    public static final int CRUISER_SIZE = 3;
    public static final int SUBMARINE_SIZE = 3;
    public static final int DESTROYER_SIZE = 2;

    public static final char SHIP_ICON = 'X';
    public static final char BOARD_ICON = '-';
    public static final char SHIP_IS_HIT_ICON = 'O';
    public static final char SHOT_MISSED_ICON = 'M';

    public static final char[] BOARD_LETTERS = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J'};
    public static final int BOARD_SIZE = 10;
}

Player.java

package com.tn.player;

import com.tn.board.Board;
import com.tn.constants.Constants;
import com.tn.ship.Ship;

import java.awt.Point;
import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;


public class Player {
    private int id;
    private int lives;
    private Board board;
    private Map<Point, Boolean> targetHistory;
    private Scanner scanner;

    /**
     * Instantiates a new Player.
     *
     * @param id the id
     */
    public Player(int id) {
        System.out.printf("%n=== Setting up everything for Player %s ====", id);
        this.id = id;
        this.lives = Constants.PLAYER_LIVES;
        this.board = new Board();
        this.targetHistory = new HashMap<>();
        this.scanner = new Scanner(System.in);
    }

    /**
     * Gets id.
     *
     * @return the id
     */
    public int getId() {
        return id;
    }

    /**
     * Gets lives.
     *
     * @return the lives
     */
    public int getLives() {
        return lives;
    }

    /**
     * Decrement live by one.
     */
    public void decrementLiveByOne() {
        lives--;
    }

    /**
     * Turn to play.
     *
     * @param opponent the opponent
     */
    public void turnToPlay(Player opponent) {
        System.out.printf("%n%nPlayer %d, Choose coordinates you want to hit (x y) ", id);
        Point point = new Point(scanner.nextInt(), scanner.nextInt());

        while(targetHistory.get(point) != null) {
            System.out.print("This position has already been tried");
            point = new Point(scanner.nextInt(), scanner.nextInt());
        }

        attack(point, opponent);
    }

    /**
     * Attack
     *
     * @param point
     * @param opponent
     */
    private void attack(Point point, Player opponent) {
        Ship ship = opponent.board.targetShip(point);
        boolean isShipHit = (ship != null) ? true : false;

        if(isShipHit) {
            ship.shipWasHit();
            opponent.decrementLiveByOne();
        }
        targetHistory.put(point, isShipHit);
        System.out.printf("Player %d, targets (%d, %d)",
                id,
                (int)point.getX(),
                (int)point.getY());
        System.out.println("...and " + ((isShipHit) ? "HITS!" : "misses..."));
    }
}

Ship.java

package com.tn.ship;

import com.tn.utils.Position;


public class Ship {
    private String name;
    private int size;
    private int livesLeft;
    private boolean isSunk;
    private Position position;

    public Ship(String name, int size) {
        this.name = name;
        this.size = size;
        this.livesLeft = size;
        this.isSunk = false;
    }

    public String getName() {
        return name;
    }

    public int getSize() {
        return size;
    }

    public int getLivesLeft() {
        return livesLeft;
    }

    public boolean isSunk() {
        return isSunk;
    }

    public void setSunk(boolean sunk) {
        isSunk = sunk;
    }

    public Position getPosition() {
        return position;
    }

    public void setPosition(Position position) {
        this.position = position;
    }

    public void shipWasHit() {
        if(livesLeft == 0) {
            isSunk = true;
            System.out.println("You sunk the " + name);
            return;
        }
        livesLeft--;
    }
}

Position.java

package com.tn.utils;

import com.tn.constants.Constants;

import java.awt.Point;

public class Position {
    private Point from;
    private Point to;

    /**
     * Instantiates a new Position.
     *
     * @param from the from
     * @param to   the to
     */
    public Position(Point from, Point to) {
        if(from.getX() > Constants.BOARD_SIZE || from.getX() < 0
                || from.getY() > Constants.BOARD_SIZE || from.getY() < 0
                || to.getX() > Constants.BOARD_SIZE || to.getX() < 0
                || to.getY() > Constants.BOARD_SIZE || to.getY() < 0) {
            throw new ArrayIndexOutOfBoundsException();
        }

        this.from = from;
        this.to = to;
    }

    /**
     * Gets from.
     *
     * @return the from
     */
    public Point getFrom() {
        return from;
    }

    /**
     * Gets to.
     *
     * @return the to
     */
    public Point getTo() {
        return to;
    }
}

Utils.java

package com.tn.utils;

import java.awt.Point;


public class Utils {

    private Utils() {

    }

    /**
     * Distance between points double.
     *
     * @param from the from
     * @param to   the to
     * @return the double
     */
    public static double distanceBetweenPoints(Point from, Point to) {
        double x1 = from.getX();
        double y1 = from.getY();
        double x2 = to.getX();
        double y2 = to.getY();

        return Math.sqrt(Math.pow(x1-x2, 2) + Math.pow(y1-y2, 2)) + 1;
    }


    /**
     * Is point between boolean.
     *
     * @param point    the point
     * @param position the position
     * @return the boolean
     */
    public static boolean isPointBetween(Point point, Position position) {
        Point from = position.getFrom();
        Point to = position.getTo();

        return from.getY() <= point.getY()
                && to.getY() >= point.getY()
                && from.getX() <= point.getX()
                && to.getX() >= point.getX();
    }

}

Game.java

package com.tn.game;

import com.tn.player.Player;


public class Game {
    private Player[] players;

    /**
     * Instantiates a new Game.
     */
    public Game() {
        players = new Player[]{
                new Player(1),
                new Player(2)
        };
    }

    /**
     * Start.
     */
    public void start() {
        int i = 0;
        int j = 1;
        int size = players.length;
        Player player = null;

        while(players[0].getLives() > 0 && players[1].getLives() > 0) {
            players[i++ % size].turnToPlay(players[j++ % size]);
            player = (players[0].getLives() < players[1].getLives()) ?
                    players[1] :
                    players[0];
        }

        System.out.printf("Congrats Player %d, you won!",player.getId());
    }
}

Main.java

package com.tn;

import com.tn.game.Game;

public class Main {

    public static void main(String[] args) {
        Game game = new Game();
        game.start();
    }
}

Answer 1

Thanks for sharing your code.

What annoys me a little bit with this code is I don't use interfaces, abstract classes, or inheritance,

Doing OOP means that you follow certain principles which are (amongst others):

• information hiding / encapsulation
• single responsibility
• separation of concerns
• KISS (Keep it simple (and) stupid.)
• DRY (Don't repeat yourself.)
• "Tell! Don't ask."
• Law of demeter ("Don't talk to strangers!")

Interfaces, abstract classes, or inheritance support hat principles and should be used as needed. They do not "define" OOP.

IMHO the main reason why your approach fails OOP is that your "Model" is an array of an primitive type char. This ultimately leads to a procedural approach for the game logic.

I would think of an interface like this:

interface GameField{
  char getIcon();
  Result shootAt();
}

where Result would be an enum:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED }

And I would have different implementations of the interface:

public class BorderField implements GameField{
  private final char borderName;
  public BorderField(char borderName){
    this.borderName = borderName;
  }
  @Override
  public char getIcon(){
    return borderName;
  }
  @Override
  public Result shootAt(){
    return Result.NO_HIT;
  }
}

---

public class WaterField implements GameField{
  private boolean isThisFieldHit = false;
  @Override
  public char getIcon(){
    return isThisFieldHit?'M': ' ';
  }
  @Override
  public Result shootAt(){
    return Result.NO_HIT;
  }
}

---

public class ShipField implements GameField{
  private final Ship ship;
  private boolean isThisFieldHit = false;
  public ShipField(Ship ship){
    this.ship = ship;
  }
  @Override
  public char getIcon(){
    Result shipState = ship.getState();
    switch(shipState){
      case NO_HIT:
        return ' ';
      case PARTIAL_HIT:
        return isThisFieldHit?'O':' ';
      case DESTROYED:
        return '#';
  }
  @Override
  public Result shootAt(){
    ship.hit();
    return  ship.getState();
  }
}

This should be enough, hope you get the idea...

---

Formal issues

Naming

Finding good names is the hardest part in programming. So always take your time to think about your identifier names.

On the bright side you follow the Java naming conventions.

But you should have your method names start with a verb in its present tense.E.g.: shipWasHit() should be named hit().
Or distanceBetweenPoints() should be calculateDistanceBetween(). Here the parameters reveal that the distance is between points, so no need to put that in the method name.

Be verbose in your variable names. instead of

    double x1 = from.getX();
    double y1 = from.getY();
    double x2 = to.getX();
    double y2 = to.getY();

this variables should rather be named like this:

    double startPointX = from.getX();
    double startPointY = from.getY();
    double endPointX = to.getX();
    double endPointY = to.getY();

Take your names from the problem domain, not from the technical solution. eg.: SHIP_ICON should be SHIP only unless you have another constant within the Ship class.

Comments

Comments should explain why the code is like it is. Remove all other comments.

comments should only be used on interface or abstract methods where they contain the contract that the implementer must fulfill.

Constants class

Put things together that belong together. Define constants in the class that uses them.

Answer 2

There are already some good answers but I thought I'd add some of the things that stood out to me when I looked through the code.

At the moment, the only source of input is user input from a Scanner. This would make it quite difficult if you wanted to add some sort of computer opponents to play against.

It looks like there is some code in the Board class that might be better suited in the Player class.

Specifically the contents of the placeShipsOnBoard() method.

This method takes input from the user and creates a Position. Let's try and restructure this so that a Player (Human or computer) can create a Position.

Let's make an interface

public interface Player {
    Position placeNextShip();
    void fireAt(Position target);
}

We already have implementations from a Human,

public class HumanPlayer implements Player {

    // variables

    @Override
    public Position placeNextShip(){
        // uses Scanner instance to create a Position
    }

    @Override
    public void fireAt(Position target){
        // code from your attack method
    }
}

And what about a basic computer player

public class DumbComputer implements Player {

        @Override
        public Position placeNextShip(){
            // keep choosing random locations
        }

        @Override
        public void fireAt(Position target){
            // keep firing at random positions
        }
}

Then, in the main Board class, we program to the Player interface

while(!isShipPlacementLegal){
   for(Player player : players){ // some list of players in the game
       // either scanner input or randomly generated Position
       Position shipPlacement = player.placeNextShip(); 
       boolean validPosition = validatePos(shipPlacement);
       if(validPostion){
          // good to go! Place ship and continue to next player
       } else {
          // prompt again, whatever else you need to do here.
       }
   }
}

If all of the code refers to a Player (now an interface) rather than a concrete implementation, we can easily add new types of computer players. e.g. new CheatingComputer(), new HardComputer(), new MediumComputer(). Each one would just determine where to fire next and where to place the next ship in a different way.

And, if we had this, we could make a game with 2 computer players and let it play itself! Exciting right :D

Another related thing that we could change is that in your constructor for Game, you will always have 2 Human players. We could make this take a List of players, so you could have any number of players in your game. Just because the real game Battleship is limited to 2 players, doesn't mean yours has to be.

We could allow for adjustable grid size and an arbitrary player number. Suddenly we have a 100x100 grid with an 8 player free for all!

(any amount of which could be computer controlled).

Your ships are also initialized in a static block in your board class. You have all the real ships from Battleship. But again, why not allow for more flexibility here. What if your ship consisted of a list of points? You could have S-shaped ships, they wouldn't need to limited to horizontal of vertical alignment. (This may be over the top, but I think it's a cool thing to think about!)

I'll finish up with a few small things that looked funny to me

throw new ArrayIndexOutOfBoundsException();

from the Position class. This seems like an inappropriate Exception to throw here. There's no array in the Position class, so this must be referring to the array in the Board class. I think a more suitable Exception type would be an IllegalArgumentException, ArrayIndexOutofBoundsException is an implementation detail (of a different class!). You should also be sure to provide an appropriate error message along with throwing the Exception. e.g "value must be within range x and y"

The line

boolean isShipHit = (ship != null) ? true : false;

could simply be replaced by

boolean isShipHit = ship != null;

There's no need for the ternary operator here.

The use of targetHistory in the Player class while(targetHistory.get(point) != null)

Here you're using a map for the sole purpose of seeing if an element is in it. This is exactly what a Set is for!

targetHistory = new HashSet<>();
while(targetHistory.contains(point)){
     // re-prompt
}

Answer 3

What annoys me a little bit with this code is I don't use interfaces, abstract classes, or inheritance, but I couldn't find a good use case for them here.

There is nothing wrong with this in your game. The game concept is so simple that you don't need any of those. The problem with little toy programs is that you don't usually need to apply big design solutions. You just need to make sure you folow the usual SOLID design principles.

---

Now that we got that cleared up let's look at some details of your code that should be improved.

The first one is pretty obvious. Don't write comments for the sake of writing comments. Some teachers like to force you to write a javadoc comment on everything. I'm actually completely against this, except when writing some sort of utility package that everyone else has to use. Normally speaking code should be self documenting. And your code does a really good job at that. So just remove the comment that's basically repeating the well chosen name of the variable/function/...

For example:

/**
 * Is point between boolean.
 *
 * @param point    the point
 * @param position the position
 * @return the boolean
 */
public static boolean isPointBetween(Point point, Position position) {

What value does that comment add to the function? I think this is also the only method that I would change to make it more readable. Because it's not obvious that the position is made up of a from and to point for which we check if our point lies between them.

What if the method had this signature:

public static boolean containsPoint(Position position, Point point) {

Wouldn't that make a little more sense?

I should add here that I'm not against comments in general though. But a comment should explain WHY something is done that way. Not how it is implemented. If I wanted to know how it's implemented I would've just looked at the code.

---

The next point is having that Util class ... Unlike some purists, I have nothing against the concept of Util classes. Util classes can be useful to put similar functions together. For example java.lang.Math that groups all the usual arithmetic operations together in one class.

The thing that troubles me with your Util class is that there isn't really a good reason for it to exist. The 2 functions that you have in there are only ever used inside the Board class. So they could've just as well been private static helper functions inside that class.

In fact, we can even do a little better after changing the signature to what I suggested earlier. What if we put containsPoint(Position position, Point point) { inside the Position class instead of having the Position as a parameter for the method? Then we can use it like this:

Position shipPosition = //some ship's position
if(shipPosition.contains(targetPoint)) {
    //handle ship hit
}

It fits really well there, doesn't it?

---

Speaking of the Positions class. I had a weird feeling about this while looking through your code. At first I thought you didn't have a something[][] to represent the board. I thought you had represented everything as Points throughout the entire codebase. This could work but makes printing the board awkward. And then I noticed do have a char[][] inside your Board class. But then wouldn't it make more sense to immdediatly put the ships inside that grid without having an intermediate Position class?

I also noticed another dangerous thing about the placeShipsOnBoard(). Should you really just trust your user to enter 2 valid coordinates? What if the user tries to be funny and inputs from=(1,1) to=(2,2)? Should we allow this? Or what if you want him to input a ship of length 5 and he inputs from=(1,1) to=(1,1) essentially shrinking the ship to a single square (that you have to hit 5 times! since the ship has 5 lives). Shouldn't we prevent him from cheating like this?

Let's look at an alternative way to handle placing the ships. First, let the user choose if he wants to place the ship horizontally or vertically. Then have him input the top/left coordinate of the ship. We will calculate the remaining points ourselves.

Here's what the actual method implementation could look like:

private Scanner scanner = new Scanner(System.in);

private void placeShipsOnBoard() {
    System.out.printf("%nAlright - Time to place out your ships%n%n");

    for(Ship ship : ships) { //awesome for-each loop is better here
        boolean horizontal = askValidShipDirection();
        Point startingPoint = askValidStartingPoint(ship, horizontal);
        placeValidShip(ship, startingPoint, boolean horizontal);
    }
}

private boolean askValidShipDirection() {
    do {
        System.out.println("Do you want to place the ship horizontally (H) or vertically (V)?");
        String direction = Scanner.nextLine().trim();
    while ( !"H".equals(direction) && !"V".equals(direction);
    return "H".equals(direction);
    //note here: use "constant".equals(variable) so nullpointer is impossible.
    //probably not needed, but it's best practice in general.
}

private Point askValidStartingPoint(Ship ship, boolean horizontal) {
    do { //note: do-while more useful here
        System.out.printf("%nEnter position of %s (length  %d): ", ship.getName(), ship.getSize());          
        Point from = new Point(scanner.nextInt(), scanner.nextInt());
    while(!isValidStartingPoint(from, ship.getLength(), horizontal));
    return from;
}

private boolean isValidStartingPoint(Point from, int length, boolean horizontal) {
    int xDiff = 0;
    int yDiff = 0;
    if(horizontal) {
        xDiff = 1;
    } else {
        yDiff = 1;
    }
    for(int i = 0; i < lenth; i++) {
        if(!isInsideBoard(i,from.getY()) {
            return false;
        }
        if(!Constants.BOARD_ICON.equals(board[from.getY()+i*yDiff][from.getX()+i*xDiff])){
            return false;
        }
     }
     return true;
}

private boolean isInsideBoard(int x, int y){
     return x <= Constants.BOARD_SIZE && x >= 0
            && y <= Constants.BOARD_SIZE && y >= 0
            && x <= Constants.BOARD_SIZE && x >= 0
            && y <= Constants.BOARD_SIZE && y >= 0;
}

private void placeValidShip(Ship ship, Point startingPoint, boolean horizontal) {
    int xDiff = 0;
    int yDiff = 0;
    if(horizontal) {
        xDiff = 1;
    } else {
        yDiff = 1;
    }
    for(int i = 0; i < ship.getLenth() ; i++) {
        board[ship.getY() + i*yDiff][ship.getX()+i*xDiff] = Constants.SHIP_ICON;
    }           
}

Now that we just place the ships on the board directly we can remove the Position class and all it's references. This does mean we no longer know whether a ship has sunk or not.

While yping this I noticed that @Timothy Truckle already posted an answer as well. I really love his solution of using dedicated Fields instead of char's to represent the board.

so our place ship method changes to:

private void placeValidShip(Ship ship, Point startingPoint, boolean horizontal) {
    int xDiff = 0;
    int yDiff = 0;
    if(horizontal) {
        xDiff = 1;
    } else {
        yDiff = 1;
    }
    for(int i = 0; i < ship.getLenth() ; i++) {
        board[ship.getY() + i*yDiff][ship.getX()+i*xDiff] = new ShipField(ship);
    }           
}

That way we can check if a ship is destroyed completely or just hit partially when attacking a Field.

Now instead of continuing on this answer, I suggest you read @Timothy's as well, and look at the good points in both our anwsers. At first glance we either agree on or simply complement each other's answer. So you should have some decent pointers on how to improve your code :)

Answer 4

Other answers have pointed out almost everything, so I'll just add one thing. It looks to me that you're using exceptions to somehow perform flow control. Exceptions are not control flow mechanisms.

public Position(Point from, Point to) {
    if (from.getX() > Constants.BOARD_SIZE || from.getX() < 0
            || from.getY() > Constants.BOARD_SIZE || from.getY() < 0
            || to.getX() > Constants.BOARD_SIZE || to.getX() < 0
            || to.getY() > Constants.BOARD_SIZE || to.getY() < 0) {
        throw new ArrayIndexOutOfBoundsException();
    }

    this.from = from;
    this.to = to;
}

And then

} catch (IndexOutOfBoundsException e) {
    System.out.println("Invalid coordinates - Outside board");
}

I think you should validate that the given coordinates are inside the board before trying to create the Position. This is user input and is perfectly reasonable to do it. You're already validating that ship.getSize() != Utils.distanceBetweenPoints(from, to). You would even be doing that in the Board object itself, instead of then having Position checking for Constants.BOARD_SIZE.