Battleship Game in Java

Question

I wanted to make a very simply Battleship game in Java, any feedback is welcome.

Nevermind the getters / setters. Just assume they are there.

class Game {
    Board board = new Board();

    void playGame() {
        Scanner scanner = new Scanner(System.in);
        while (!board.allShipsSank()) {
            System.out.println("Enter missile locations, x and y.");
            int x = scanner.nextInt();
            int y = scanner.nextInt();
            board.shoot(x, y);
        }
    }
}

class Board {
    Set<Ship> ships = new HashSet<>();

    boolean allShipsSank() {
        for (Ship ship : ships)
            if (!ship.isSank())
                return false;
        return true;
    }

    void shoot(int x, int y) {
        for (Ship ship : ships) {
            if (ship.shipGeographic.orientation == ShipGeographic.Orientation.HORIZONTAL) {
                if (y == ship.shipGeographic.y && ship.shipGeographic.x <= x && x <= ship.shipGeographic.x + ship.len) {
                    ship.missileAt(x - ship.shipGeographic.x);
                    return;
                }
            }
            if (ship.shipGeographic.orientation == ShipGeographic.Orientation.VERTICAL) {
                if (x == ship.shipGeographic.x && ship.shipGeographic.y <= y && y <= ship.shipGeographic.y + ship.len) {
                    ship.missileAt(ship.shipGeographic.y + y);
                    return;
                }
            }
        }
    }
}


class ShipGeographic {
    enum Orientation {
        HORIZONTAL, VERTICAL;
    }

    int x;
    int y;
    ShipGeographic.Orientation orientation;
}

class Ship {
    // length of the ship
    int len;

    // Nodes where this ship has been hit
    Set<Integer> hit = new HashSet<>();

    // Where this Ship is
    ShipGeographic shipGeographic;

    Ship(int len) {
        this.len = len;
    }

    boolean isSank() {
        return hit.size() == len;
    }

    // For example, if ship is hit at head, parameter is 0
    // Missile at same node of ship has no effect
    void missileAt(int offsetFromLen) {
        if (hit.add(offsetFromLen)) {
            System.out.println("boom!");
            System.out.println(hit);
        }
    }
}

here is how I initialize and run the game:

class App {
    public static void main(String[] args) {
        Game game = new Game();

        // Just some sample data!
        {
            Ship ship = new Ship(3);
            ShipGeographic shipGeographic = new ShipGeographic();
            shipGeographic.orientation = ShipGeographic.Orientation.HORIZONTAL;
            shipGeographic.x = 1;
            shipGeographic.y = 1;

            ship.shipGeographic = shipGeographic;

            game.board.ships.add(ship);
        }

        game.playGame();
    }
}

A sample run:

Enter missile locations, x and y.
1
1
boom!
[0]
Enter missile locations, x and y.
5
5
Enter missile locations, x and y.
2
1
boom!
[0, 1]
Enter missile locations, x and y.
3
1
boom!
[0, 1, 2]

Process finished with exit code 0

Answer 1

Thanks for sharing your code.

what I like

• you follow the Java Naming conventions
• you resist to solve the problem based on an array

what I dislike

violation of encapsulation / information hiding

You access directly properties of an object and (even worse) their properties too like this:

x == ship.shipGeographic.x && ship.shipGeographic.y <= y

But the code using an object should not know anything about the internal implementation pf this object. The using code should only call (public) methods on on an object. This enables both: polymorphism and improving the internal implementation without affecting the calling code. The getters and setters you mention are not a solution to this problem.

The names for this "code smells" is feature envy: the calling code does something what should better be done inside the object by applying the "tell, don't ask" principle.

separation of concerns - intermix of user IO and business logic

In your code the Board class is responsible for the user IO and also does game logic. This reduces the re-usability of the game logic. I.e.: you have to do a complete rewrite if you want to change the User interface to graphics instead of a command line. You should learn about the Model-View-Controller concept (MVC) and its relatives MCP, MVVC and MVVM.

unnecessary mutability

Your ship objects are mutable, this means their properties could be changed at any time during the game. But in practice the properties never change after initialization. Therefore your should pass all the properties as constructor parameters and store them in final declared properties.

incomplete OO approach.

IMHO you stopped doing an OO approach half way.

Holding a list of ships which know their positions is a good first step. But you could walked that path further.

E.g. The list of ships could only hold unsunk ones. Then the check for the end of the game would change from a loop to a simple statement:

boolean allShipsSank() {
    return listOfShips.isEmpty();    
}

Next possibility could be the "ship hit" logik.

For once the ship should know itself if being hit:

class Ship {
  //...
  boolean isHitBy(int x, int y) {
    // logic here
  }
  //...
}

but instead of doing a calculation here I'd go the "collection" approach again.

First we need to introduce another class:

class BordPosition{ // this is a DTO and has no business logik
   final int x;
   final int y;
   BordPosition(int x, int y){
     this.x = x;
     this.y = y;
   }
   // implement equals() and hascode()
}

and we need to enhance the Orientation enum:

enum Orientation {
    HORIZONTAL{
        public BordPosition translate(BordPosition startPoint, int stepsToMove){
           return new BordPosition(startPoint.x+stepsToMove, startPoint.y);
        }
    }, 
    VERTICAL{
        public BordPosition translate(BordPosition startPoint, int stepsToMove){
           return new BordPosition(startPoint.x, startPoint.y+stepsToMove);
        }
    };
    abstract public BordPosition translate(BordPosition startPoint, int stepsToMove);
}

then class Ship would change to this

class Ship {
  private final Collection<BordPosition> occupiedFields = new HashSet<>();
  private final Collection<BordPosition> hitFields = new HashSet<>();
  Ship(BordPosition startPoint, Orientation orientation, int size){
    for (int stepsToMove = 0; stepsToMove<size; stepsToMove++) {
        occupiedFields.add(orientation.translate(startPoint,stepsToMove));
    }
  }

  boolean isHitBy(BordPosition missleTarget) {
    if(occupiedFields.contains(missleTarget)){
      hitFields.add(missleTarget);
      if (hitFields.size()==occupiedFields.size()){
       // deregister from ship list via Listener pattern
      }
      return true;
    } else {
      return false;
    } 
  }
  //...
}

Answer 2

Procedural Programming

In general it uses data-structures to solve a problem. Data-structures are like lookup tables, in which it is possible to save, modify and read values.

Working with data-structures could look in java like

class Stroke { /* ... */}
class Color { /* ... */}

class Rect {
   double width;
   double hight;
   Stroke stroke;
}

Rect rect = new Rect();
rect.width = 5;
rect.hight = 14;
rect.stroke.type = "dotted";
rect.color.value = "#000";
rect.color.opacity= "0.3";

In the code you provided I found this statemant

game.board.ships.add(ship);

Object Oriented Programming

In general it is about sending messages (interact with methods) from one object to an other.

The following two lines of code tries to express "Hey Game! Please add a ship to the board".

// in a procedural way
game.board.ships.add(ship);

// in an object oriented way
game.addToBoard(ship)

What I can find in your code base is a hybrid (half object and half data).

Readability

Variables

Variables are a good way to express what code does.

void missileAt(int offsetFromLen)

What is a len? To answer these question I will look into your class and find the instance variable len.

// length of the ship
int len;

Now I know it is the length and not a acronym. I thing it is much easier to write the 3 more letters, than to write a comment to explain it.

Methods

if (ship.shipGeographic.orientation == ShipGeographic.Orientation.HORIZONTAL)

This code be readen easier as

if (ship.isInHorizontalPosition())

Feature Envy

A method accesses the data of another object more than its own data.

The method shoot in Board do heavy operations on Ship. This is possible because your "objects" don't hide their implementation.

void shoot(int x, int y) {
    for (Ship ship : ships) {
        if (ship.shipGeographic.orientation == ShipGeographic.Orientation.HORIZONTAL) {
            if (y == ship.shipGeographic.y && ship.shipGeographic.x <= x && x <= ship.shipGeographic.x + ship.len) {
                ship.missileAt(x - ship.shipGeographic.x);
                return;
            }
        }
        if (ship.shipGeographic.orientation == ShipGeographic.Orientation.VERTICAL) {
            if (x == ship.shipGeographic.x && ship.shipGeographic.y <= y && y <= ship.shipGeographic.y + ship.len) {
                ship.missileAt(ship.shipGeographic.y + y);
                return;
            }
        }
    }
}

Instead of Board the Ship itself should lookup if it gets hit.

void shoot(int x, int y) {
    for (Ship ship : ships)
        ship.handleShootTo(x, y);
}