3
\$\begingroup\$

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
\$\endgroup\$
4
\$\begingroup\$

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;
    } 
  }
  //...
}
\$\endgroup\$
  • \$\begingroup\$ Very useful tips, thank you. I can not tell I agree with all but I think it is ok to disagree sometimes and I guess there is no specific concrete way. \$\endgroup\$ – Koray Tugay Feb 16 at 16:02
  • \$\begingroup\$ @KorayTugay "I can not tell I agree with all but I think it is ok to disagree sometimes" - yes, there is no "right or wrong", only "more or less useful". But may you share where you disagree? Maybe I can learn something too? \$\endgroup\$ – Timothy Truckle Feb 16 at 16:09
  • \$\begingroup\$ Is a ship supposed to know it's location and orientation? If a board consists of Bombable cells and each bombable is connected to either a reference to a Ship or a NOP, depending if the cell is occupied by a vessel or not. Ship would only know it's length, as it's an inherent trait, and you'd add it to the board in either horizontal or vertical orientation and the board would be responsible for knowing which cells the ship occupies, since the cells belong to the board. \$\endgroup\$ – TorbenPutkonen Feb 18 at 13:19
  • \$\begingroup\$ @TorbenPutkonen "Is a ship supposed to know it's location and orientation?" - When doing OOP it is a good approach to look at the real world: In the real world, is it the ship or the ocean knowing position and heading? \$\endgroup\$ – Timothy Truckle Mar 26 at 10:41
  • \$\begingroup\$ @TimothyTruckle Real world is often way too complicated to be represented as a class hierarchy. In the real world the ship is in water but that water might be moved by currents and wind which affecfts the true heading. Heck, sometimes that body of water might be on a ferris wheel. :) So... there are many ways to skin this cat and whatever way fits here depends on the required fetures. \$\endgroup\$ – TorbenPutkonen Mar 27 at 6:22
4
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ A ship can not handle if it is shot or not, a board knows where the ship is. Thanks for the thoughts though, as usual +1. \$\endgroup\$ – Koray Tugay Feb 19 at 13:41
  • \$\begingroup\$ You can see @Torbens comment in the answer above as well, I actually saw it after I commented to your answer. He seems agree with me. \$\endgroup\$ – Koray Tugay Feb 19 at 13:42
  • 1
    \$\begingroup\$ But since the ship holds the shipGeographic (ship.shipGeographic) it knows where it is.. or am I wrong.. \$\endgroup\$ – Roman Feb 19 at 13:44
  • 1
    \$\begingroup\$ I would add to that that board should not contain algorithms to handle missile hits. Missile hits are part of the rules of the game so they should be in the game engine. How the game engine implements them is another issue (e.g. it can set up object relations so that a hit to board automatically triggers a hit at a certain location of a ship). \$\endgroup\$ – TorbenPutkonen Feb 19 at 13:45
  • \$\begingroup\$ I find that mind games regarding extensions sometimes help. E.g. what if I want to add a nuke that hits at a radius larger than one? Would adding that be easy? If not, the design is flawed. \$\endgroup\$ – TorbenPutkonen Feb 19 at 13:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.