2
\$\begingroup\$

If someone wants to make a review of this bad code i'd be more than happy :) I do know i'm far from a programmer but i've just started and looking for small tips and stuff.

import java.util.*;

import javax.swing.JOptionPane;

public class Game {

public static void main(String[] args) {

    Scanner sc = new Scanner(System.in);

    Boolean current = false;
    int choice;
    String slaskvariabel;
    JOptionPane.showMessageDialog(null, "             START GAME");

    Player player[] = new Player[2];

    slaskvariabel = JOptionPane.showInputDialog("Do you 1. want to play against an other player or 2. Player against the computer?");

    choice = Integer.parseInt(slaskvariabel);

    while(choice<=0 || choice>2){
        slaskvariabel = JOptionPane.showInputDialog("\"1\" to play against a player \"2\" to play against the computer.");

        choice = Integer.parseInt(slaskvariabel);

    }

    if (choice == 1) {

        for (int i = 0; i < 2; i++) {

            System.out.println("Player " + (i + 1));
            System.out.println("¯ ¯ ¯ ¯ ¯ ");
            player[i] = new Player();

            player[i].setShipPositions(1);
            delay(5000);

            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
        }

        while (current == false) {
            System.out.println("Player 1");
            System.out.println("¯ ¯ ¯ ¯ ¯");
            player[0].attack(player[1], 1);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("\n");
            delay(85);
            System.out.println("Player 2");
            System.out.println("¯ ¯ ¯ ¯ ¯");
            player[1].attack(player[0], 1);

            for (int k = 0; k < 2; k++) {

                int check = 0;

                for (int i = 0; i < 7; i++) {
                    for (int j = 0; j < 7; j++) {

                        if (player[k].getShipsOnMapHealth(i, j) == 0) {
                            check++;

                            if (check == 5 && k == 0) {
                                current = true;
                                System.out.println("Player " + (k + 1) + " has won!\nHere is player " + (k + 1)
                                        + " map: ");
                                player[0].drawMap();

                            }

                            if (check == 5 && k == 1) {
                                current = true;
                                System.out.println("Player " + (k + 1) + " has won!\nHere is player " + (k + 1)
                                        + " map: ");
                                player[1].drawMap();
                            }

                        }

                    }
                }
            }

        }

    }
    if (choice == 2) {

        player[0] = new Player();

        player[0].setShipPositions(1);

        player[1] = new Player();

        player[1].setShipPositions(0);

        while (current == false) {

            player[0].attack(player[1], 1);

            player[1].attack(player[0], 2);
            for (int k = 0; k < 2; k++) {

                int check = 0;

                for (int i = 0; i < 7; i++) {
                    for (int j = 0; j < 7; j++) {

                        if (player[k].getShipsOnMapHealth(i, j) == 0) {
                            check++;

                            if (check == 5 && k == 0) {
                                current = true;
                                System.out.println("Player " + (k + 1) + " has won!\nHere is player " + (k + 1)
                                        + " map: ");
                                player[0].drawMap();

                            }

                            if (check == 5 && k == 1) {
                                current = true;
                                System.out.println("The computer has won..\nHere is the computers map");
                                player[1].drawMap();
                            }

                        }

                    }
                }
            }
        }

    }

}

public static void delay(int millis) {
    try {
        Thread.sleep(millis);
    } catch (InterruptedException exp) {
    }
}

}

import java.util.*;

import javax.swing.JOptionPane;

public class Player {

private Ship ship[] = new Ship[5];
private Map map;

public Player() {

    for (int i = 0; i < 5; i++) {

        ship[i] = new Ship();

        while (i > 1 && i < 4) {

            i++;
            ship[i] = new Ship(4);

        }

    }

    map = new Map();

}

public void setShipPositions(int temp) {
    Scanner sc = new Scanner(System.in);

    // A65-Z90, ASCII

    int y, x;
    int choice = 0;
    char cordinateAsLetter;

    if (temp == 1) {

        System.out.println("Do you 1. want to do the placement of the boats on your own or 2. randomize them?: ");
        choice = sc.nextInt();

        while (choice <= 0 || choice > 2) {
            System.out.println("\"1\" to do the placement on your own \"2\" to randomize them.");
            choice = sc.nextInt();
        }
    }

    if (choice == 1 && temp == 1) {

        System.out.print("Here is the battleground.\n");
        map.drawMap();
        System.out.println("\n");

        for (int i = 0; i < 5; i++) {
            System.out.println("Where do you want to put boat " + (i + 1) + " with " + ship[i].getHealth()
                    + "HP, on the map?\nY cordinate (1-7): \nX cordinate (A-G): ");

            y = sc.nextInt() - 1;
            cordinateAsLetter = sc.next().charAt(0);
            x = (int) cordinateAsLetter - 65;

            while (y < 0 || y > 6 || (int) cordinateAsLetter > 90 || (int) cordinateAsLetter < 65) {

                System.out.println(
                        "Choose 1-7 for Y cordinate and A-G for X cordinate (Important with big letters (A-G).");

                y = sc.nextInt() - 1;
                cordinateAsLetter = sc.next().charAt(0);
                x = (int) cordinateAsLetter - 65;
            }

            ship[i].setPositionX(x);
            ship[i].setPositionY(y);

            if (i > 0) {
                while (y == ship[i - 1].getPositionY() && x == ship[i - 1].getPositionX()) {

                    System.out.println(
                            "Y. " + (y + 1) + " and X. " + (x + 1) + " is taken, choose again. \nY (1-7): \nX (A-G):");

                    y = sc.nextInt() - 1;
                    cordinateAsLetter = sc.next().charAt(0);
                    x = (int) cordinateAsLetter - 65;

                    ship[i].setPositionX(x);
                    ship[i].setPositionY(y);

                }
            }
            map.setShipOnMap(ship[i].getPositionY(),ship[i].getPositionX(), ship[i].getHealth());
        }

        System.out.print("Here is the battleground with boats.\n");
        map.drawMap();
    }
    if (choice == 2 || temp != 1) {
        for (int i = 0; i < 5; i++) {

            Random rand = new Random();
            y = rand.nextInt(6);
            x = rand.nextInt(6);

            if (i > 0) {

                for (int j = 0; j < i; j++) {

                    while (y == ship[j].getPositionY() && x == ship[j].getPositionX()) {

                        y = rand.nextInt(6);
                        x = rand.nextInt(6);
                    }
                }

            }

            ship[i].setPositionX(x);
            ship[i].setPositionY(y);

            map.setShipOnMap(ship[i].getPositionY(), ship[i].getPositionX(), ship[i].getHealth());

        }

    }
    if (choice == 2) {
        System.out.print("Here is the battleground with boats randomized by the computer.\n");
        map.drawMap();
    }
}

public void attack(Player enemy, int temp) {
    // method to attack the other player.

    Random rand = new Random();
    Scanner sc = new Scanner(System.in);

    int y, x, health;
    char cordinateAsLetter;

    if (temp == 1) {
        // If temp is 1 means that its a player who is playing.

        for (int i = 0; i < 3; i++) {
            // Each player gets to shoot 3 times when its their turn.
            System.out
                    .println("Where do you want to attack? \nY cordinate (1-7): \nX cordinate (A-G)\n(Your map is beeing shown) ");

            map.drawMap();

            y = sc.nextInt()-1;
            cordinateAsLetter = sc.next().charAt(0);
            x = (int) cordinateAsLetter - 65;

            while (y < 0 || y > 6 || (int) cordinateAsLetter > 90 || (int) cordinateAsLetter < 65) {

                System.out.println("Y cordinate 1-7 and X cordinate A-G (Important with big letters A-G).");

                y = sc.nextInt() - 1;
                cordinateAsLetter = sc.next().charAt(0);
                x = (int) cordinateAsLetter - 65;
            }

            if (enemy.map.getKordinat(y , x ) < 10) {
                // If the hit is successfully the following will happen. 

                int damage = rand.nextInt(3);

                health = (enemy.map.getKordinat(y , x ) - damage);

                if (damage == 0) {
                    // If the hit is 0 there will be no damaged made but the boat will be localised.

                    System.out.println(
                            "Since it was a windy day with high waves the shot didn't hit the boat but atleast the boat is localised...\n");

                    map.changeStringKordinat(y+1, x+1, String.valueOf(enemy.map.getKordinat(y , x )));

                    map.drawMap();

                }
                if (damage > 0) {
                    // If the damaged is over 0 the hit will be made and the boat will be shown on your map in form of its health.

                    System.out.println("You got a hit and made " + damage
                            + " in damage! Lucky there weren't high waves and winds..");

                    if (health < 0) {
                        health = 0;
                    }
                    enemy.map.changeIntKordinat(y , x , health);
                    map.changeStringKordinat(y+1, x+1, (String.valueOf(enemy.map.getKordinat(y , x)) + " "));

                    map.drawMap();

                }

            }

            else {
                // If no boat was hit this will happen.
                System.out.println("No boat got hit.");
                map.changeStringKordinat(y+1, x+1, "! ");

                map.drawMap();
            }
        }
        delay(2000);
    }

    else {
        // If the computer shall play.
        //This doesn't work atm...
        for (int i = 0; i < 3; i++) {

            y = rand.nextInt(6);
            x = rand.nextInt(6);

            if (enemy.map.getKordinat(y, x) < 10) {

                int damage = rand.nextInt(2);

                health = (enemy.map.getKordinat(y, x) - damage);

                if (damage == 0) {

                    enemy.map.changeIntKordinat(y, x, health);
                    enemy.map.changeStringKordinat(y, x, (String.valueOf(enemy.map.getKordinat(y, x)) + " "));
                    map.changeStringKordinat(y, x, String.valueOf(enemy.map.getKordinat(y, x)));

                }
                if (damage > 0) {

                    if (health < 0) {
                        health = 0;
                    }

                    enemy.map.changeIntKordinat(y, x, health);
                    enemy.map.changeStringKordinat(y, x, (String.valueOf(enemy.map.getKordinat(y, x)) + " "));
                    map.changeStringKordinat(y, x, (String.valueOf(enemy.map.getKordinat(y, x)) + " "));

                }

            }

        }
        System.out.println("Here is your map after the attacks.");
        enemy.map.drawMap();

    }

}

public int getShipsOnMapHealth(int y, int x) {

    return map.getKordinat(y, x);
}

public void drawMap() {
    map.drawMap();
}

public static void delay(int millis) {
    try {
        Thread.sleep(millis);
    } catch (InterruptedException exp) {
    }
}
}


public class Ship {

private int health;
private int positionX;
private int positionY;

public Ship() {

    health = 3;
    positionX = 0;
    positionY = 0;

}

public Ship(int h) {

    health = h;
    positionX = 0;
    positionY = 0;

}

public void setPositionX(int position) {

    positionX = position;

}

public void setPositionY(int position) {

    positionY = position;

}

public int getHealth() {
    return health;
}

public int getPositionX() {
    return positionX;
}

public int getPositionY() {
    return positionY;
}

}    





public class Map {

private int map[][] = new int[7][7];
private String battleground[][] = new String[8][8];

public Map() {

    String column[] = { "0 ", "1 ", "2 ", "3 ", "4 ", "5 ", "6 ", "7 " };
    String row[] = { "0", " A", "B", "C", "D", "E", "F", "G" };

    for (int i = 0; i < 7; i++) {
        for (int j = 0; j < 7; j++) {
            map[i][j] = 10;
        }
    }

    for (int i = 0; i < 8; i++) {

        for (int j = 0; j < 8; j++) {
            if (i == 0) {
                battleground[i][j] = " " + row[j];
                battleground[j][i] = column[j] + " ";
            }
            if (battleground[i][j] == null) {
                battleground[i][j] = "- ";
            }
        }

    }
    battleground[0][0] = " ";
}

public void setShipOnMap(int positionY, int positionX, int health) {

    map[positionY][positionX] = health;

    battleground[positionY + 1][positionX + 1] = "V ";

}

public void drawMap() {

    System.out.println("___________________________________________");
    for (int i = 0; i < 8; i++) {
        System.out.println("");

        for (int j = 0; j < 8; j++) {

            System.out.print(battleground[i][j]);

        }
    }
    System.out.println("");
    System.out.println("__________________________________________");

}

public void changeStringKordinat(int y, int x, String boatHealth) {

    battleground[y][x] = boatHealth + "";

}

public void changeIntKordinat(int y, int x, int health) {

    map[y][x] = health;
}

public int getKordinat(int y, int x) {

    return map[y][x];
}

public String getStringKordinat(int y, int x) {

    return battleground[y][x];
}

}
\$\endgroup\$
  • 2
    \$\begingroup\$ If your code is contained in multiple files, please, make clear where one file ends and the next begins, preferably by splitting them into different code blocks. \$\endgroup\$ – Ben Steffan Apr 11 '17 at 17:11
4
\$\begingroup\$

Indentation/Spacing

Although your indentation is generally good, please consider also indenting your code when you enter a new class definition, e.g.

public class Game {

    public static void main(String[] args) {

instead of

public class Game {

public static void main(String[] args) {

Another issue which, strangely occurs only in one location has to do with operator spacing:

while(choice<=0 || choice>2){

is sub-optimal at best. Strive to always leave a space before and after each operator, before each curly brace and in between control structure keywords and their corresponding opening paren:

while (choice <= 0 || choice > 2) {

Variables

Boolean current = false;

If you don't have a good reason, always prefer boolean over Boolean, int over Integer and so on and so forth.

Another issue is you variable naming: you have variables called choice, current etc., which, obviously, have English names, next to a slaskvariabel, which is, also obviously, not English. This is bad because it makes your code harder to read for everybody who doesn't know both languages. Stick to one, preferably to English because it's usually the smallest common denominator.

Magic numbers

x = (int) cordinateAsLetter - 65;

Why do you subtract 65? Why not some other value, say 32? Using magic numbers is bad because the numbers themselves don't convey any meaning. Still, sometimes you just need to use a specific hard-coded number in your code (e.g. if you programmed a chess simulator, you would have to incorporate the fact that a chess board is 8x8 tiles big), so what do you do? Usually, this issue is circumvented by using constants; however, in your case, you can just replace your 65 with the value it represents, which is 'A'. This is definitely better, but not perfect, since it might not be obvious to everyone what subtracting 'A' from a char does. Ideally, you would thus create another method called toAscii (or another, equally fitting name) that encapsulates the functionality of converting a char to it's numerical representation.

Error handling

What happens if your player inputs a String instead of a number? The documentation tells us that Integer.parseInt() will throw an exception in that case, which you don't handle. Ergo, your program will crash. Dealing with malformed input is one of the most basic and most important tasks of error handling, and you should definitely improve your code under that aspect.

Redundancy

System.out.println("\n");
delay(85);
//7 more times

Use a for-loop. Writing the same code multiple times is considered bad practice and harms readability.

Code structure

Your main method is way too big. If I'm not mistaken, I count 8 levels of indentation there. That's not good, because it makes your code very hard to read and difficult to navigate. You should split it up into multiple methods with precisely defined tasks. This will also make debugging a lot easier.

Another problem is your use of input/output devices. Why did you decide it would be a good idea to use GUI elements exactly two times and just revert to the console for the rest of the application? To me, this makes no sense at all. Either you go through the hassle of designing a full-blown GUI application or you stay with the console. Either is fine, but mixing them wildly is not.


Since this answer has become quite long, I'll leave it here. Keep in mind, however, that this far from comprehensive and there are still many points I haven't mentioned yet. Take it as a first impression, but keep your eyes open for more suggestions by other people.

\$\endgroup\$
  • \$\begingroup\$ okey thank you very much! but when I put the main code into methods, should they all be private, static and in the Game class? \$\endgroup\$ – plznoh8 Apr 11 '17 at 20:29
  • \$\begingroup\$ @plznoh8 Ideally, you would split the code into further classes, such as a class for IO, one for the battle logic, one for the victory conditions etc. Try to come up with a scheme that makes sense to you. \$\endgroup\$ – Ben Steffan Apr 11 '17 at 20:57

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.