8
\$\begingroup\$

I recently whipped up a Battleship program with Java as part of my Basic OOP class in university. The class is done but all we get are grades. I would really love, though, to know where and what I can improve upon. Is there a better way to implement my code? (It's still my first year in software engineering, just in case you see glaring misunderstandings of programming concepts.)

Grid Class

public class Grid {
    private boolean isGrenade;
    private boolean isPShip;
    private boolean isCShip;
    private boolean shotAt;
    private char state;
    
    public Grid() {
        isGrenade = false;
        isPShip = false;
        isCShip = false;
        shotAt = false;
        state = '-';
    }
    
    //sets a player ship
    public void setPShip(Grid[][] a, int i, int j) {
        a[i][j].isPShip = true;
        a[i][j].state = 'S';
    }
    
    //sets a computer ship
    public void setCShip(Grid[][] a, int i, int j) {
        a[i][j].isCShip = true;
        a[i][j].state = 's';
    }
    
    //sets a player grenade
    public void setPGrenade(Grid[][] a, int i, int j) {
        a[i][j].isGrenade = true;
        a[i][j].state = 'G';
    }
    
    //sets a computer grenade
    public void setCGrenade(Grid[][] a, int i, int j) {
        a[i][j].isGrenade = true;
        a[i][j].state = 'g';
    }
    
    //returns whether a cell has been shot already
    public boolean alreadyShot(Grid[][] a, int i, int j) {
        return a[i][i].shotAt;
    }
    
    //changes a cell's state to one that has been shot
    public void changeState(Grid[][] a, int i, int j) {
        a[i][j].state = '*';
    }
    
    //sets a cell's state to one that has been shot
    public void setState(Grid[][] a, int i, int j) {
        a[i][j].shotAt = true;
    }
    
    //returns whether a cell is occupied by a ship/grenade
    public boolean getCellState() {
        return (this.state != '-');
    }
    
    //prints out the cell's state
    public char printState(Grid[][] a, int i, int j) {
        if (a[i][j].shotAt == true) {
            return a[i][j].state;
        }
        else {
            return '-';
        }
    }
    
    //prints out the cell's state - cheat mode
    public char revealedState(Grid[][] a, int i, int j) {
        return a[i][j].state;
    }   
    
}

BattleshipDriver Class

/**
 * This program simulates the game, Battleship.
 * The player and the computer each have six ships. 
 * They also have four grenades each.
 * Shooting a grenade makes one lose a turn.
 * The first to sink all the opponent's ships wins.
 */

import java.util.Scanner;

public class BattleshipDriver {
    //Global constant declarations
    public static final int GRID_ROWS = 8;
    public static final int GRID_COLS = 8;
    public static final int PLAYER_SHIPS = 6;
    public static final int PLAYER_GRENADES = 4;
    public static final int COMP_SHIPS = 6;
    public static final int COMP_GRENADES = 4;
    public static final int PROMPT_SIZE = 7;
    
    //Global variable declarations
    public static boolean cheats = false;
    public static int playerLives = PLAYER_SHIPS;
    public static int compLives = COMP_SHIPS;
    public static Scanner in = new Scanner(System.in);
        
    //Main battleship driver
    public static void main(String[] args) {    
        String menuChoice = "";
        
        //Main menu screen
        while (true) {
        printTitle();
        System.out.println("To have the best possible experience, please set your console window to the maximum size.");
        System.out.println();
        System.out.println("Press 1 to play against the computer.");
        System.out.println("Press 2 to play WITH CHEATS against the computer (reveals the grid).");
        System.out.println("Press 3 to see the game's rules.");
        System.out.println("Press 4 to exit.");
        System.out.print("Your choice: ");
        menuChoice = in.next(); 
        switch (menuChoice) {
            case "1":
                startGame();
                break;
            case "2":
                cheats = true;
                startGame();
                break;
            case "3": 
                clearScreen();
                printRules();   
                break;
            case "4":
                clearScreen();
                System.out.println("Thank you for playing!");
                in.close();
                System.exit(0);
                break;
            default:
                System.out.println("That is not a valid choice.");
                pressEnter();
                clearScreen();
            }
        } 
    }
    
    //Game proper
    public static void startGame () {
        Grid[][] currentGame = new Grid[GRID_ROWS][GRID_COLS];
        String coords = "";
        int x, y;
        boolean playerLostTurn = false;
        boolean compLostTurn = false;
        
        //Constructor loop
        for (int i = 0; i < GRID_ROWS; i++) {
            for (int j = 0; j < GRID_COLS; j++) {
                currentGame[i][j] = new Grid();
            }
        }

        playerInit(currentGame);
        compInit(currentGame);
        
        System.out.println("Let the games begin!");
        System.out.println();
        
        while (checkVictory()) {
            //Checks for cheat mode
            if (cheats) {
                revealedGrid(currentGame);
            }
            else {
                printGrid(currentGame);
            }
            
            //Player's turn
            if (playerLostTurn) {
                System.out.println("Sorry Captain, you lost a turn from that grenade.");
                playerLostTurn = false;
                pressEnter();
            }
            else {
                do {
                System.out.print("It's your turn! Please input coordinates for your missile: ");
                coords = in.next();
                x = coords.charAt(1) - 49;                                          //converts x-coord to corresponding int
                y = ((int) Character.toUpperCase(coords.charAt(0))) - 65;           //converts y-coord to corresponding int
                
                //out of bounds clause
                    if ((x > 7) || (y > 7) || (x < 0)) {
                        System.out.println("Sorry, those coordinates are beyond grid size.");
                        System.out.println("Make sure to hit within the map, oh Captain.");
                        pressEnter();
                    } 
                } while ((x > 7) || (y > 7) || (x < 0)); 
                
                //shot at the same place clause
                if (currentGame[x][y].alreadyShot(currentGame, x, y)) {
                    System.out.println("Sorry, that cell was already shot at.");
                    System.out.println("Make sure we don't shoot at the same thing twice, oh Captain.");
                    pressEnter();
                    clearScreen();
                }
                //shot an enemy ship clause
                else if (currentGame[x][y].revealedState(currentGame, x, y) == 's') {
                    System.out.println("Bullseye! You hit the enemy ship");
                    currentGame[x][y].setState(currentGame, x, y);
                    if (cheats) { currentGame[x][y].changeState(currentGame, x, y); }
                    compLives--;
                    pressEnter();
                    clearScreen();
                }
                //shot own ship clause
                else if (currentGame[x][y].revealedState(currentGame, x, y) == 'S') {
                    System.out.println("Oh no! You shot your own ship! What kind of captain are you?!");
                    currentGame[x][y].setState(currentGame, x, y);
                    if (cheats) { currentGame[x][y].changeState(currentGame, x, y); }
                    playerLives--;
                    pressEnter();
                    clearScreen();
                }
                //shot a grenade clause
                else if ((currentGame[x][y].revealedState(currentGame, x, y) == 'g') || (currentGame[x][y].revealedState(currentGame, x, y) == 'g')) {
                    System.out.println("BOOM! You hit a grenade! You lost a turn! Oh Captain!");
                    currentGame[x][y].setState(currentGame, x, y);
                    if (cheats) { currentGame[x][y].changeState(currentGame, x, y); }
                    playerLostTurn = true;
                    pressEnter();
                    clearScreen();
                }
                //shot at nothing clause
                else {
                    System.out.println("We hit water! Adjust sights to port, oh Captain!");
                    currentGame[x][y].setState(currentGame, x, y);
                    currentGame[x][y].changeState(currentGame, x, y);
                    pressEnter();
                    clearScreen();
                }
            }
            
            //checks if computer previously hit a grenade and lost a turn
            if (compLostTurn == true) {
                System.out.println ("The dumb computer lost a turn. It's your time to shine again, oh Captain!");
                pressEnter();
                clearScreen();
                compLostTurn = false;
            }
            else {
                compLostTurn = compTurn(currentGame);
            }
        }
        
        //checks to see who won
        if (compLives == 0) {
            clearScreen();
            printVictory();
        }
        else {
            clearScreen();
            printGameOver();
        }
        
    }
    
    //Player's turn to set ships and grenades
    public static void playerInit(Grid[][] currentGame) {
        String coords = "";
        int x, y;
        int counter = 0;
        
        //Set player ships
        while (counter < PLAYER_SHIPS) {
            clearScreen();
            revealedGrid(currentGame);
            System.out.println();
            System.out.printf("Please input the coordinates for ship #%d: ", (counter + 1));
            coords = in.next();
            x = coords.charAt(1) - 49;                                          //converts x-coord to corresponding int
            y = ((int) Character.toUpperCase(coords.charAt(0))) - 65;           //converts y-coord to corresponding int
            
            if ((x > 7) || (y > 7) || (x < 0)) {
                System.out.println("Sorry, those coordinates are beyond grid size.");
                pressEnter();
            }
            else if (currentGame[x][y].getCellState()) {
                System.out.println("Sorry, that cell is already occupied.");
                pressEnter();
            }
            else {
                currentGame[x][y].setPShip(currentGame, x, y);
                counter++;
            }
        }
        
        //Set player grenades
        counter = 0;
        while (counter < PLAYER_GRENADES) {
            clearScreen();
            revealedGrid(currentGame);
            System.out.println();
            System.out.printf("Please input the coordinates for grenade #%d: ", (counter + 1));
            coords = in.next();
            x = coords.charAt(1) - 49;                                          //converts x-coord to corresponding int
            y = ((int) Character.toUpperCase(coords.charAt(0))) - 65;           //converts y-coord to corresponding int
            
            if ((x > 7) || (y > 7) || (x < 0)) {
                System.out.println("Sorry, those coordinates are beyond grid size.");
                pressEnter();
            }
            else if (currentGame[x][y].getCellState()) {
                System.out.println("Sorry, that cell is already occupied.");
                pressEnter();
            }
            else {
                currentGame[x][y].setPGrenade(currentGame, x, y);
                counter++;
            }
        }
        clearScreen();
        
    }
    
    //Computer's turn to set ships and grenades 
    public static void compInit(Grid[][] currentGame) {
        int x, y = 0;
        int counter = 0;
        
        //Set ships
        while (counter < COMP_SHIPS) {
            do {
                x = (int) (Math.random() * (GRID_ROWS - 1)) + 1;
                y = (int) (Math.random() * (GRID_COLS - 1)) + 1;
            } while (currentGame[x][y].getCellState());
            currentGame[x][y].setCShip(currentGame, x, y);
            counter++;
        }
        
        //Set grenades
        counter = 0;
        while (counter < COMP_GRENADES) {
            do {
                x = (int) (Math.random() * (GRID_ROWS - 1)) + 1;
                y = (int) (Math.random() * (GRID_COLS - 1)) + 1;
            } while (currentGame[x][y].getCellState());
            currentGame[x][y].setCGrenade(currentGame, x, y);
            counter++;
        }
        
    }
    
    //Computer's turn to fire missiles
    public static boolean compTurn(Grid[][] currentGame) {
        int x, y = 0;
        
            do {
                x = (int) (Math.random() * (GRID_ROWS - 1)) + 1;
                y = (int) (Math.random() * (GRID_COLS - 1)) + 1;
            } while (currentGame[x][y].alreadyShot(currentGame, x, y));
            
            char xCoord = (char) (x + 48); //converts random coords to grid coords (e.g. A1)
            char yCoord = (char) (y + 64);
            
            System.out.printf("The dumb computer launched his missile at: %c%c\n", yCoord, xCoord);
            
            x--;        //sets coords to array indices
            y--;
            
            //if the computer hits one of the player's ships clause
            if (currentGame[x][y].revealedState(currentGame, x, y) == 'S') {
                System.out.println("Oh no! The dumb computer sank one your ships! It wasn't so dumb after all.");
                currentGame[x][y].setState(currentGame, x, y);
                if (cheats) { currentGame[x][y].changeState(currentGame, x, y); }
                playerLives--;
                System.out.println();
                return false;
            }
            //if the computer hits one of his own ships clause
            else if (currentGame[x][y].revealedState(currentGame, x, y) == 's') {
                System.out.println("Oh no! The dumb computer shot one of his own ships!");
                currentGame[x][y].setState(currentGame, x, y);
                if (cheats) { currentGame[x][y].changeState(currentGame, x, y); }
                compLives--;
                System.out.println();
                return false;
            }
            //if the computer hits nothing but water clause
            else if (currentGame[x][y].revealedState(currentGame, x, y) == '-') {
                System.out.println("The dumb computer hit nothing but water.");
                currentGame[x][y].setState(currentGame, x, y);
                currentGame[x][y].changeState(currentGame, x, y);
                System.out.println();
                return false;
            }
            //if computer shoots at the same thing twice clause
            else if (currentGame[x][y].alreadyShot(currentGame, x, y)) {
                System.out.println("The dumb computer tried to shoot at the same thing twice.");
                System.out.println();
                return false;
            }
            //if computer hits a grenade clause
            else if ((currentGame[x][y].revealedState(currentGame, x, y) == 'G') || (currentGame[x][y].revealedState(currentGame, x, y) == 'g')) {
                System.out.println("BOOM! The dumb computer hit a grenade! It lost a turn!");
                currentGame[x][y].setState(currentGame, x, y);
                if (cheats) { currentGame[x][y].changeState(currentGame, x, y); }
                System.out.println();
                return true;
            }
        return false;   
    }
    
    //Checks victory condition
    public static boolean checkVictory () {
        return ((playerLives > 0) && (compLives > 0)) ;
    }
    
    //Prints the grid
    public static void printGrid (Grid[][] currentGame) {
        System.out.print("  ");
        for (char ch = 'A'; ch < 'I'; ch++) {               // prints the column headings
            System.out.print(ch + " ");
        }
        System.out.println();
        for (int i = 0; i < (GRID_ROWS) ; i++) {
            System.out.print((i + 1) + " ");                // prints the row headings
            for (int j = 0; j < (GRID_COLS) ; j++) {
                System.out.print(currentGame[i][j].printState(currentGame, i, j) + " ");
            }
            System.out.println();
        }
        System.out.println("LEGEND: ");
        System.out.println("S - your ship        s - enemy ship");
        System.out.println("G - your grenade     g - enemy grenade");
        System.out.println("* - cell has already been shot");
        System.out.printf("Player Ships Left: %d\n", playerLives);
        System.out.printf("Computer Ships Left: %d\n", compLives);
        System.out.println();
    }   

    //Prints a revealed grid (cheats mode)
    public static void revealedGrid (Grid[][] currentGame) {
        System.out.print("  ");
        for (char ch = 'A'; ch < 'I'; ch++) {               // prints the column headings
            System.out.print(ch + " ");
        }
        System.out.println();
        for (int i = 0; i < (GRID_ROWS) ; i++) {
            System.out.print((i + 1) + " ");                // prints the row headings
            for (int j = 0; j < (GRID_COLS) ; j++) {
                System.out.print(currentGame[i][j].revealedState(currentGame, i, j) + " ");
            }
            System.out.println();
        }
        System.out.println("LEGEND: ");
        System.out.println("S - your ship        s - enemy ship");
        System.out.println("G - your grenade     g - enemy grenade");
        System.out.println("* - cell has already been shot");
        System.out.printf("Player Ships Left: %d\n", playerLives);
        System.out.printf("Computer Ships Left: %d\n", compLives);
        System.out.println();
    }   

    //Prints the game over screen
    public static void printGameOver () {
        System.out.println("   _____                         ____                 ");
        System.out.println(" / ____|                       / __ \\                ");               
        System.out.println("| |  __  __ _ _ __ ___   ___  | |  | |_   _____ _ __ ");
        System.out.println("| | |_ |/ _` | '_ ` _ \\ / _ \\ | |  | \\ \\ / / _ \\ '__|");
        System.out.println("| |__| | (_| | | | | | |  __/ | |__| |\\ V /  __/ |");   
        System.out.println(" \\_____|\\__,_|_| |_| |_|\\___|  \\____/  \\_/ \\___|_|"); 
        System.exit(0);
    }
    
    //Prints the victory screen
    public static void printVictory () {
        System.out.println("VVVVVVVV           VVVVVVVVIIIIIIIIII      CCCCCCCCCCCCCTTTTTTTTTTTTTTTTTTTTTTT     OOOOOOOOO     RRRRRRRRRRRRRRRRR   YYYYYYY       YYYYYYY !!!"); 
        System.out.println("V::::::V           V::::::VI::::::::I   CCC::::::::::::CT:::::::::::::::::::::T   OO:::::::::OO   R::::::::::::::::R  Y:::::Y       Y:::::Y!!:!!");
        System.out.println("V::::::V           V::::::VI::::::::I CC:::::::::::::::CT:::::::::::::::::::::T OO:::::::::::::OO R::::::RRRRRR:::::R Y:::::Y       Y:::::Y!:::!");
        System.out.println("V::::::V           V::::::VII::::::IIC:::::CCCCCCCC::::CT:::::TT:::::::TT:::::TO:::::::OOO:::::::ORR:::::R     R:::::RY::::::Y     Y::::::Y!:::!");
        System.out.println("V:::::V           V:::::V   I::::I C:::::C       CCCCCCTTTTTT  T:::::T  TTTTTTO::::::O   O::::::O  R::::R     R:::::RYYY:::::Y   Y:::::YYY!:::!");
        System.out.println("V:::::V         V:::::V    I::::IC:::::C                      T:::::T        O:::::O     O:::::O  R::::R     R:::::R   Y:::::Y Y:::::Y   !:::!");
        System.out.println("V:::::V       V:::::V     I::::IC:::::C                      T:::::T        O:::::O     O:::::O  R::::RRRRRR:::::R     Y:::::Y:::::Y    !:::!");
        System.out.println("V:::::V     V:::::V      I::::IC:::::C                      T:::::T        O:::::O     O:::::O  R:::::::::::::RR       Y:::::::::Y     !:::!");
        System.out.println("V:::::V   V:::::V       I::::IC:::::C                      T:::::T        O:::::O     O:::::O  R::::RRRRRR:::::R       Y:::::::Y      !:::!");
        System.out.println("V:::::V V:::::V        I::::IC:::::C                      T:::::T        O:::::O     O:::::O  R::::R     R:::::R       Y:::::Y       !:::!");
        System.out.println("V:::::V:::::V         I::::IC:::::C                      T:::::T        O:::::O     O:::::O  R::::R     R:::::R       Y:::::Y       !!:!!");
        System.out.println("V:::::::::V          I::::I C:::::C       CCCCCC        T:::::T        O::::::O   O::::::O  R::::R     R:::::R       Y:::::Y        !!! ");
        System.out.println("V:::::::V         II::::::IIC:::::CCCCCCCC::::C      TT:::::::TT      O:::::::OOO:::::::ORR:::::R     R:::::R       Y:::::Y");            
        System.out.println("V:::::V          I::::::::I CC:::::::::::::::C      T:::::::::T       OO:::::::::::::OO R::::::R     R:::::R    YYYY:::::YYYY     !!!"); 
        System.out.println("V:::V           I::::::::I   CCC::::::::::::C      T:::::::::T         OO:::::::::OO   R::::::R     R:::::R    Y:::::::::::Y    !!:!!");
        System.out.println("VVV            IIIIIIIIII      CCCCCCCCCCCCC      TTTTTTTTTTT           OOOOOOOOO     RRRRRRRR     RRRRRRR    YYYYYYYYYYYYY     !!!"); 
        System.exit(0);
    }
    
    //Prints the main title screen
    public static void printTitle() {
        System.out.println("8 888888888o          .8.    8888888 8888888888 8888888 8888888888 8 8888         8 8888888888     d888888o.   8 8888        8  8 8888 8 888888888o");   
        System.out.println("8 8888    `88.       .888.         8 8888             8 8888       8 8888         8 8888         .`8888:' `88. 8 8888        8  8 8888 8 8888    `88."); 
        System.out.println("8 8888     `88      :88888.        8 8888             8 8888       8 8888         8 8888         8.`8888.   Y8 8 8888        8  8 8888 8 8888     `88 ");
        System.out.println("8 8888     ,88     . `88888.       8 8888             8 8888       8 8888         8 8888         `8.`8888.     8 8888        8  8 8888 8 8888     ,88 ");
        System.out.println("8 8888.   ,88'    .8. `88888.      8 8888             8 8888       8 8888         8 888888888888  `8.`8888.    8 8888        8  8 8888 8 8888.   ,88' ");
        System.out.println("8 8888888888     .8`8. `88888.     8 8888             8 8888       8 8888         8 8888           `8.`8888.   8 8888        8  8 8888 8 888888888P'  ");
        System.out.println("8 8888    `88.  .8' `8. `88888.    8 8888             8 8888       8 8888         8 8888            `8.`8888.  8 8888888888888  8 8888 8 8888         ");
        System.out.println("8 8888      88 .8'   `8. `88888.   8 8888             8 8888       8 8888         8 8888        8b   `8.`8888. 8 8888        8  8 8888 8 8888         ");
        System.out.println("8 8888    ,88'.888888888. `88888.  8 8888             8 8888       8 8888         8 8888        `8b.  ;8.`8888 8 8888        8  8 8888 8 8888         ");
        System.out.println("8 888888888P .8'       `8. `88888. 8 8888             8 8888       8 888888888888 8 888888888888 `Y8888P ,88P' 8 8888        8  8 8888 8 8888         ");
        System.out.println();
    }
    
    //Prints the game's rules
    public static void printRules() {
        System.out.println("THE RULES");
        System.out.printf("The player has %d ships and %d grenades\n", PLAYER_SHIPS, PLAYER_GRENADES);
        System.out.println("The computer has the same number of ships and grenades.");
        System.out.println("Each player gets a turn to fire a missile on the board.");
        System.out.println("If the chosen tile has already been shot, you may pick another tile.");
        System.out.println("If the chosen tile has a grenade, the player/computer will lose a turn.");
        System.out.println("The first one to destroy all the opponent's ships wins.");
        System.out.println("Are you ready?");
        System.out.println();
        pressEnter();
        clearScreen();
        return;
    }
    
    //Clears the screen by moving down 50 lines
    public static void clearScreen () {
        System.out.println(System.lineSeparator().repeat(50));
    }
    
    //Enter key prompt to pause between events
    private static void pressEnter () {
        System.out.print("Press Enter to continue...");
        System.out.println();
        try {
            System.in.read();
        }  
        catch(Exception e)
        {}  
    }
    
}

Oh, and should you need a reference, these were the instructions. Thanks!

\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

A pretty work I have to say. Actually your class design is good, however it may as well be simplified.

Before you read:

There are many comments to do over your code, however I must clarify that: It is not my intention to make you feel as everything is wrong, you already starting to learn. Second, if I have some comments for you in your first year, I have many many comments for me in my first two/three years, so, it's normal to be quite "code verbose" in your starts.

Suggestions:

  • Circular relationships are to be evaded most times. Note that your Grid class contains various methods which requires instances of the Grid class, it's quite an exotic implementation. Analyzing it you may note that a grid does not contain itself, so, why is the grid operating multiple instances of itself? (I'm referring to the matrix)

  • Remember the principle, classes have attributes and methods related to them, the attributes:

    private boolean isGrenade;
    private boolean isPShip;
    private boolean isCShip;
    private boolean shotAt;
    private char state;

are not proper of a Grid but a cell on the grid.

  • The grid should contain a matrix cells of objects of type Cell, it will save time and if you consider it, the grid should be a class giving support to the elements on it.

  • The field isGrenade could be named isNavalMine why? Well, if you want to acquire "good" developer habits, a good one is to make every variable name the least ambiguous possible. (I'm not an English native, so it was quite strange to read)

  • You could simplify the attributes by using an Enumeration:

public enum EnumCellProperties {
    //According to the document COMP248_A4_F2020.pdf
    IS_PLAYER_GRENADE('g'),
    IS_COMPUTER_GRENADE('G'),
    IS_PLAYER_SHIP('s'),
    IS_COMPUTER_SHIP('S');

    private char initialState;

    private EnumCellProperties(char initialState) {
        this.initialState = initialState;
    }

    public getInitialState() {
        return this.initialState;
    }
}

//call it as:
public class Cell {
    private EnumCellProperties properties; //This indicates you
    private char state;
    private boolean shooted;
    //...

    public Cell(EnumCellProperties properties) {
        this.properties = properties;
        this.state = properties.getInitialState();
    }

    public char printState() {
        //boolean values are either true or false, do not compare
        //use if (booleanVariable) |---| or if (!booleanVariable) when you need == false
        if (shooted) return state;
        return '-'; //as the if has a return, there's no need to place an else
    }

    public char revealState() {
        return state;
    }
}

this will save the code inside:

    public void setPShip(Grid[][] a, int i, int j)
    public void setCShip(Grid[][] a, int i, int j)
    public void setPGrenade(Grid[][] a, int i, int j)
    public void setCGrenade(Grid[][] a, int i, int j)
    public char printState(Grid[][] a, int i, int j)
    public char revealedState(Grid[][] a, int i, int j)
  • Finally, for your BattleshipDriver class, you should separate the game logic from the printings, you could create a GameEngine class to do al the logic in the background and have a clearer BattleshipDriver class.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Hi! Thank you so much for the input! I'll analyze this as soon as exams are over to model a v2.0 for my program. My difficulty was wrapping my head around a class that had cells that had attributes, much like a spreadsheet. This was what my thought process was in making the Grid class. The circularity was something I wanted to eliminate but could not wrap my head around as well. As for the name grenade, it was a requirement from the professor. Again, many thanks! \$\endgroup\$
    – J. Ong
    Dec 15, 2020 at 23:18
3
\$\begingroup\$

@Miguel Avila has made some good points. A few other things for you to consider...

Magic Numbers

Generally I don't mind numbers in the code if they make sense, and make the code easier to read. When you use constants (text or numbers), look out for patterns which might suggest that you could benefit from encapsulating the value in a named constant. So, for example:

if ((x > 7) || (y > 7) || (x < 0))

You've already got a constant:

public static final int GRID_COLS = 8

Using the constant, or having another dependant constant:

public static final int MAX_X = GRID_COLS - 1;

Means that if you decided to make the game easier/harder by changing the number of columns you only have to go an modify one place, rather than searching in the code for instances of 7/8 which might need to be updated.

Avoid Distributed Knowledge

Your identifying what's in different grid sections by using different characters. So, 'S' is a player ship, 's' is a computer ship. This knowledge is help in at least two places at the moment. Your Grid sets the state (in setPShip, setCShip). It's also known in your driver class, when it checks what's been hit (if (currentGame[x][y].revealedState(currentGame, x, y) == 's'). This can cause issues if you decide to modify the way that the state is stored in the future. An alternative might have been to have your grid implement methods like isPlayerShip(x,y). This would centralise the knowledge of how a 'ship' is represented to the class that is responsible for that representation.

Naming's hard, but important

Finding the balance with names can be challenging, however they are one of the biggest factors that influences your codes readability. So, for example you have setPShip, with a comment above it 'sets player player ship'. Would this comment be needed if the method was simply called setPlayerShip?

In your driver you represent column/rows as variables 'x/y'. In your grid they are 'i/j', this causes an unnecessary disjoint that makes the code harder to read. Why not just use x/y in both places? One letter variable names can be ok if they make sense or are in very confined contexts, however using 'a' to represent the game grid (because it's an array?) doesn't scream readability.

Comments

Comments should be helpful and up to date. Because they aren't compiled it's very easy for comments to become out of date, so generally I avoid them unless there is something to say that isn't obvious from the code. So comments that don't add anything to what the code is saying should be avoided (//Checks for cheat mode, the if(cheats) already says this). Generally if you feel the need to comment what the code is doing, it might be a sign that you need to change variable names / encapsulate some of the logic within another named method. Useful comments are more likely to be 'why' something has to be done a certain way if it's not going to obvious from the code. However, that still might be an indication that a larger refactoring is required.

Stale / out of date comments can actively cause harm by being misleading. Consider:

//prints out the cell's state - cheat mode
public char revealedState(Grid[][] a, int i, int j) {
    return a[i][j].state;
}   

There's a few things wrong. revealedState doesn't print anything, it returns the state symbol at a given location in the supplied grid. It's true that this is used in cheat mode to print the state, however it's not what the commented method does. Importantly the method is also actually used in the main game logic to determine what's been shot.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your feedback! Our professor only touched naming conventions and best practices for 10 minutes and I tried my best to stick to them but thank you for pointing out where I still need improvement. \$\endgroup\$
    – J. Ong
    Dec 20, 2020 at 13:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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