OOP Battleship console game in Java

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 cha

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.

StackExchange Code Review Q#161680, answer score: 7