4
\$\begingroup\$

I have written a simple Code using Array List, Scanner and String for Developing a Simple Console based 2- Player Battleship Game in Java

The Complete Question can be found here

Below is the Solution

import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Scanner;

 /**
  *Created by Mohammad Javed
  *Completed on 3 October
 */
class BattleShip {
    private static final String PLAYER_A = "A";
    private static final String PLAYER_B = "B";

    private static final String SHIP_Q = "Q";

    static  int M;
    static  int N;
    static List<String> frigateWithPlayerA=new ArrayList<String>();
    static  List<String> frigateWithPlayerB=new ArrayList<String>();

    private static void placeShipOnBoard(int row1, int col1, String locBattleShip1PlayerA, String typeBattleShip, String boardType) {
        //Adding the co-ordinates having the Ship  in  frigateWithPlayerA, frigateWithPlayerB List
        char firstA=locBattleShip1PlayerA.toUpperCase().charAt(0);
        char secondA=locBattleShip1PlayerA.charAt(1);
        for(int i=1;i<=row1;i++){
            for(int j=1;j<=col1;j++){
                int k=convertLetterToInt(firstA)-1+i;
                int l=secondA-49+j;
                if(boardType.equals(PLAYER_A)){
                    frigateWithPlayerA.add(String.valueOf(convertInttoLetter(k))+String.valueOf(l));
                    if(typeBattleShip.equalsIgnoreCase(SHIP_Q)){
                        frigateWithPlayerA.add(String.valueOf(convertInttoLetter(k))+String.valueOf(l));
                    }
                }
                else if(boardType.equals(PLAYER_B)){
                    frigateWithPlayerB.add(String.valueOf(convertInttoLetter(k))+String.valueOf(l));    
                    if(typeBattleShip.equalsIgnoreCase(SHIP_Q)){
                        frigateWithPlayerA.add(String.valueOf(convertInttoLetter(k))+String.valueOf(l));
                    }
                }
            }
        }

    }

    public static void main(String[] args) {

        Scanner sc=null;
        String s=null;
        final  int M;
        String typeBattleShip1,battleShip1Dim,typeBattleShip2,battleShip2Dim,locBattleShip1PlayerA,locBattleShip1PlayerB,locBattleShip2PlayerA,locBattleShip2PlayerB;
        int row1,col1,row2,col2;
        String[] MissileTargetPlayerA;
        String[] MissileTargetPlayerB;
        try{

            sc=new Scanner(System.in);//.useDelimiter("\\|");
            System.out.println("Fetching Input  : "+LocalDateTime.now());
            while(true){
                /*
                 * Sample Input used for Testing 
                 * Enter area boundaries: 5 E
                        Type for battleship 1: Q
                        Dimension for battleship 1: 1 1
                        Location of battleship 1 for player A: A1
                        Location of battleship 1 for player B: B2
                        Type for battleship 2: P
                        Dimension for battleship 2: 2 1
                        Location of battleship 2 for player A: D4
                        Location of battleship 2 for player B: C3
                        Missile targets for player A: A1 B2 B2 B3
                        Missile targets for player B: A1 B2 B3 A1 D1 E1 D4 D4 D5 D5
                 */
                System.out.println("\nEnter area boundaries :\n");
                s=sc.nextLine();
                // Enter area boundaries
                M=Integer.valueOf(s.split(" ")[0]);
                N=Integer.valueOf(convertLetterToInt(s.split(" ")[1]));
                if (M > 9)
                {
                    throw new IllegalArgumentException("ERROR! M  CANNOT BE > 9");
                }

                //Type for battleship 1
                System.out.println("\nType for battleship 1 :\n");
                typeBattleShip1=sc.nextLine();
                System.out.println("\nDimension for battleship 1 :\n");

                battleShip1Dim=sc.nextLine();
                col1=Integer.valueOf(battleShip1Dim.split(" ")[0]);
                row1=Integer.valueOf(battleShip1Dim.split(" ")[1]);

                System.out.println("\n Location of battleship 1 for player A :\n");

                locBattleShip1PlayerA=sc.nextLine();
                System.out.println("\n Location of battleship 1 for player B:\n");
                locBattleShip1PlayerB=sc.nextLine();

                System.out.println("\nType for battleship 2 :\n");

                typeBattleShip2=sc.nextLine();
                System.out.println("\nDimension for battleship 2 :\n");
                battleShip2Dim=sc.nextLine();

                col2=Integer.valueOf(battleShip2Dim.split(" ")[0]);
                row2=Integer.valueOf(battleShip2Dim.split(" ")[1]);
                System.out.println("\n Location of battleship 2 for player A :\n");
                locBattleShip2PlayerA=sc.nextLine();
                System.out.println("\n Location of battleship 2 for player B :\n");
                locBattleShip2PlayerB=sc.nextLine();
                System.out.println("\n Missile targets for player A : \n");
                MissileTargetPlayerA=sc.nextLine().split(" ");
                System.out.println("\n Missile targets for player B : \n");
                MissileTargetPlayerB=sc.nextLine().split(" ");
                break;
            }

            placeShipOnBoard(row1,col1,locBattleShip1PlayerA,typeBattleShip1.toUpperCase(),PLAYER_A);

            placeShipOnBoard(row2,col2,locBattleShip2PlayerA,typeBattleShip2.toUpperCase(),PLAYER_A);

            placeShipOnBoard(row1,col1,locBattleShip1PlayerB,typeBattleShip1.toUpperCase(),PLAYER_B);

            placeShipOnBoard(row2,col2,locBattleShip2PlayerB,typeBattleShip2.toUpperCase(),PLAYER_B);

            fireMissileAndHit(MissileTargetPlayerA,MissileTargetPlayerB);


        }
        catch(IllegalArgumentException e){
            System.err.println("IllegalArgumentException  "+e);
        }
        catch(Exception e){
            System.err.println("Error "+e);
        }

    }


    private static void fireMissileAndHit(String[] missileTargetPlayerA,
            String[] missileTargetPlayerB) {
        // TODO Auto-generated method stub

        //Mainlogic where we play the game fire and declare winners
        List<String> playerA=new ArrayList<String>(Arrays.asList(missileTargetPlayerA));
        List<String> playerB=new ArrayList<String>(Arrays.asList(missileTargetPlayerB));



        Iterator<String> PlayerAIterator = playerA.iterator();
        Iterator<String> PlayerBIterator = playerB.iterator();

        while(true)
        {
            while (PlayerAIterator.hasNext()) {
                // Start the Hits by Player A on Player B board
                String hit=PlayerAIterator.next().toUpperCase();
                if(frigateWithPlayerB.contains(hit)){
                    System.out.println("Player-1 fires a missile with target "+hit+" which hit");
                    frigateWithPlayerB.remove(hit);
                }
                else{
                    System.out.println("Player-1 fires a missile with target "+hit+" which missed");
                    break;
                }
            }
            if(!(PlayerAIterator.hasNext())){
                System.out.println("Player-1 has no more missiles left");
            }
            if(!(PlayerBIterator.hasNext())){
                System.out.println("Player-2 has no more missiles left");
            }
            if(!(PlayerAIterator.hasNext())&&!(PlayerBIterator.hasNext())){
                // Declaring winners
                if(frigateWithPlayerA.isEmpty()){
                    System.out.println("Player-2 won the battle");
                }
                if(frigateWithPlayerB.isEmpty()){
                    System.out.println("Player-1 won the battle");

                }
                if(!frigateWithPlayerB.isEmpty() && !frigateWithPlayerA.isEmpty()){
                    System.out.println("Peace");
                }
                break;
            }
            while (PlayerBIterator.hasNext()) {
                // Start the Hits by Player B on Player A board
                String hit=PlayerBIterator.next().toUpperCase();
                if(frigateWithPlayerA.contains(hit)){
                    System.out.println("Player-2 fires a missile with target "+hit+" which hit");
                    frigateWithPlayerA.remove(hit);
                }
                else{
                    System.out.println("Player-2 fires a missile with target "+hit+" which missed");
                    break;
                }
            }

        }
        System.out.println("*******************************************");
    }
    /*HELPER METHODS*/
    private static int convertLetterToInt(String val)
    {

        return val.toUpperCase().charAt(0)-'A'+1;
    }

    private static String convertInttoLetter(int val)
    {

        if(val < 0 && val > 27 ){
            throw new IllegalArgumentException("ERROR!  CANNOT BE > 27 or < 0");
        }

        return  String.valueOf((char)(val + 'A' - 1)) ;
    }

    /*HELPER METHODS*/
    private static int convertLetterToInt(char val)
    {

        return val-'A'+1;
    }


}`

I have coded to be the best of my understanding.

But I got the below remark from my mentor (it actually broke my heart)

What was good?

  • Good variable and method names

What was not so good?

  • Very little knowledge of OOPS
  • Duplicate code which could have easily been extracted into methods
\$\endgroup\$
4
\$\begingroup\$

I'll go into the 'Little OOP' remark.

In OOP you try to model your object to real world stuff.

Objects

In this assignment, you have several objects:

  • Board
  • Player
  • Ship
  • MissileShot
  • MissileResult (hit / miss)
  • Location

In the assignment you see some attributes of there objects, for example the board that has a size, the ships that have dimensions and locations on the their board.

For example, the board class could look like this:

public class Board
{ 
   List<Ship> ships;

   //not really necessary for the assignment to keep track.
   List<MissileShot> firedMissiles;

   int width;
   int height;
   ....
}

Methods

Then, there is a behaviour. This is more tricky; but for example take 'a player shoots a missile on a location on a board'

This might become a method on a board:

public class Board
{ 
   ....
   public MissileResult fireMissile(Location location)
   {
       MissileResult result = MISS;
       for (Ship s : ships)
       { 
           if (s.fireAt(location) == HIT)
           {
                result = HIT;
           }
       }
       firedMissiles.add(new MissileShot(location));
       return result;
   }
}

Ships can keep track of the missiles that hit it (at the locations), and Ships themselves might use this in a method isSunk()

public class Ship 
{ 
     boolean isSunk()
     {
          ...
     }
}

Determining game-over become something like this:

public class Board
{
    public boolean isGameOver() 
    {
         boolean allSunk = true;
         for (Ship s : ships)
         {
              allSunk =  allSunk && s.isSunk();
         }    
         return allSunk;
    }
}

Note that all these classes and methods can be reused for players.

\$\endgroup\$
  • \$\begingroup\$ Seems great. I actually had all these thoughts before I started coding. But one thing I want to ask, if we can solve a problem using simple String and Arraylist, is it worth to use Objects. Is it not a memory burden. I actually want to fight with my mentor on this point :-) \$\endgroup\$ – Mohammad Javed Oct 7 '17 at 6:55
  • 1
    \$\begingroup\$ For such a small application don't even bother. Here is some info: stackoverflow.com/questions/258120/…. Don't fight your mentor over this, pick a better fight later :) \$\endgroup\$ – RobAu Oct 7 '17 at 9:07
  • \$\begingroup\$ I agree with you, RobAu over fight :-) \$\endgroup\$ – Mohammad Javed Oct 8 '17 at 13:46
  • 1
    \$\begingroup\$ Thanks RobAu, I will work on the points mentioned above regarding OOPS \$\endgroup\$ – Mohammad Javed Oct 8 '17 at 16:05
3
\$\begingroup\$

I'd just like to add a few more points on top of what @RobAu has already suggested in his answer.

It's considered an anti-pattern / bad practice to use exceptions as control flow. In your program, you throw / catch an IllegalArgumentException to re-start the user input process on an invalid input. I would recommend that you simply continually prompt for a valid value instead.

You should keep the main method in a separate class. At the moment your BattleShip class has multiple responsibilities. It's in charge of being the game, but also as the runner/driver of the program. I would expect the main method to look like this.

public class BattleShipRunner {
    public static void main(String[] args){
        BattleShip battleShip = new BattleShip();
        battleship.start();
    }
}

This class has the sole responsibility of starting your game.

I would avoid using Strings to represent something more complex than a String.

The String "A", is not a Player, it's a String. A Player can and should use Strings internally where needed to achieve the desired functionality.

Your BattleShip is kind of a God object it currently stands. It knows about and is in charge of too many things.

It keeps track of Ships/Players/Missles/Grids etc. I would say that the BattleShip object should tie all of these things together, but each of these responsibilities should belong in its own class. @RobAu showed some good examples in his answer.

One of the main benefits of using custom Objects instead of just Strings/Lists/Arrays for everything, is that it makes the code highly readable if done well.

At the top, we have

List<String> frigateWithPlayerA = new ArrayList<String>();

so all I know here, is that it's a list of Strings that represents the ships that player A has. But to find out how it does that, I need to go look at where and how it's used.

So I search through the code and find this line

frigateWithPlayerA.add(String.valueOf(convertInttoLetter(k))+String.valueOf(l));

It's quite a mouth full, convertInttoLetter is a good method name, it explains exactly what it does, I don't need to go read the code to understand it. k is not a great name, I have no idea what that is. Same goes for l, these tell me nothing about what they are, at first glance they're just numbers being plugged into a method to convert them to letters. So some investigation needs to be done to understand what's going on here.

I look through to see some more uses to get an idea what's going on

 if(frigateWithPlayerA.isEmpty()){
     System.out.println("Player-2 won the battle");
 }

Okay so now it looks like if the list is empty, the other player wins.

And one more example, this code

String hit=PlayerBIterator.next().toUpperCase();
if(frigateWithPlayerA.contains(hit)){
    System.out.println("Player-2 fires a missile with target "+hit+" which hit");
    frigateWithPlayerA.remove(hit);
}

So there's something to do with some sort of iterator, but PlayerBIterator doesn't tell me a whole lot about it. It's also just giving back Strings. So far everything is dealing with Strings, it becomes hard to keep track of.

Instead, to make it more readable and easier to follow, I'd like to see something like this.

if(player1.hasAnotherMissle()){
    Missle missle = player1.getNextMissle();
    missle.fireAt(player2);
    if(!player2.hasShipsLeft()){
        // player 1 wins the game
    }
}

Now this isn't meant to act as a replacement for your code examples I talked about, but just to act as a demonstration as to how utilising OOP concepts and creating your own classes and objects can hugely improve readability of your code.

You could use just strings and arrays for everything, but it makes it harder to read and follow. If done correctly, someone could read your code and not for the most part not need to look at the bodies of the methods. By using good method and variable names, as well as class names, and modelling your system in a sensible way, your code will be self documenting

Hopefully this was useful for you!

\$\endgroup\$
  • \$\begingroup\$ Greatly appreciate your comments ! Good point raised on self documentation. \$\endgroup\$ – Mohammad Javed Oct 8 '17 at 16:01

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.