3
\$\begingroup\$

I made this game which should imitate a spaceship and obstacles (planets). Move spaceship and try to hit everything. I will add some others features and functionality (better images or whatever).

I have several question.

My IDE (IntelliJ) gives me some warning and I try to remove them. One of them is "access can be package-private". I know what it means, it tries to "narrow down" the access. So, according to this I should have written for example void myMethod() instead of public void myMethod(). The question : should I be obedient and change everything? I am confused because a lot of code is written in "bad" way. Simply, listen to the compiler, right?

Other warning is "field can be converted to a local variable". Don't make (private final int SOMETHING = 1231) and make this in appropiate method, right? I want to ask because some variables have to be in the first lines of class (must be accessed generally by whole class) and some in one method. For me it's cleaner when I have all variables like this in one place, but compiler tells to write some here and some in methods. Listen to the compiler one more time, right?

I would like to ask if my serialization is in proper way, and reading and writing to files.

What are the best classes which can deal with simple wav or mp3 file? I want to add some music to that.

Exceptions - I know I have to catch exceptions or add throws but what should I do with that exception at the end? Make some message like JOptionPaneor what?

Each opinion is appreciated. Thanks in advance.

Main class

package com.company;

import java.awt.BorderLayout;
import javax.swing.JFrame;

public class Main extends JFrame {
    private static int WIDTH_GAME_FRAME = 800;
    private static int HEIGHT_GAME_FRAME = 700;
    private GamePanel gamePanel;

    public Main() throws Exception {
        this.setTitle("SpaceWar");
        this.setLayout(new BorderLayout());
        this.setSize(WIDTH_GAME_FRAME, HEIGHT_GAME_FRAME);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        this.setLocationRelativeTo(null);
        gamePanel = new GamePanel();
        this.addWindowListener(new java.awt.event.WindowAdapter() {
            @Override
            public void windowClosing(java.awt.event.WindowEvent windowEvent) {
                gamePanel.exitGame();
            }
        });

        this.add(gamePanel);
        this.setResizable(false);
        this.setVisible(true);
    }

    public static void main(String[] args) {
        try {
            new Main();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    public static int getWidthGameFrame() {
        return WIDTH_GAME_FRAME;
    }

    public static int getHeightGameFrame() {
        return HEIGHT_GAME_FRAME;
    }
}

GamePanel class

package com.company;

import java.awt.*;
import java.util.LinkedList;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import javax.swing.JPanel;

public class GamePanel extends JPanel {
    public enum stateOfGame {GAME, MENU, HIGH_SCORE}

    public enum SpaceShipWay {LEFT, RIGHT}

    public static stateOfGame state = stateOfGame.MENU;
    private SpaceShip spaceShip;
    private int points = 0;
    private Menu menu;
    private String playerName;
    private AllTimeLeaders allTimeLeaders;
    private HealthBar healthBar;
    private LinkedList<PlayerScore> loadedHighScoreList;
    private HighScoreFile loadedHighScoreFile;
    private FileReaderAndWriter fileReaderAndWriter;
    private LinkedList<Bullet> bullets;
    private LinkedList<Obstacle> obstacles;
    private CollisionDetector collisionDetector;

    public GamePanel() throws Exception {
        this.setBackground(Color.BLACK);
        menu = new Menu();
        this.addMouseListener(new MouseInput(this));

        fileReaderAndWriter = new FileReaderAndWriter();
        fileReaderAndWriter.load();

        loadedHighScoreFile = fileReaderAndWriter.getHighScoreFile();

        if (loadedHighScoreFile == null)
            loadedHighScoreFile = new HighScoreFile();
        else
            loadedHighScoreList = loadedHighScoreFile.getHighScoreList();

        allTimeLeaders = new AllTimeLeaders(this, loadedHighScoreList);

        spaceShip = new SpaceShip();
        healthBar = new HealthBar();
        collisionDetector = new CollisionDetector(this, spaceShip, healthBar);

        bullets = collisionDetector.getBullets();
        obstacles = collisionDetector.getObstacles();

        KeyInput keyInput = new KeyInput(this, spaceShip);

        ScheduledThreadPoolExecutor keyExecutor, bulletExecutor, obstacleExecutor;
        keyExecutor = new ScheduledThreadPoolExecutor(5);
        bulletExecutor = new ScheduledThreadPoolExecutor(5);
        obstacleExecutor = new ScheduledThreadPoolExecutor(5);

        Runnable bulletMove = () -> {
            for (Bullet bullet : bullets)
                bullet.move();

            collisionDetector.checkBulletCollision();
        };

        Runnable obstacleMove = () -> {
            for (Obstacle obstacle : obstacles)
                if (obstacle.isObstacleActive())
                    obstacle.move();
                else
                    obstacle.setObstacle();

            collisionDetector.checkSpaceShipCollision();
            collisionDetector.checkObstacleCollision();
        };

        keyExecutor.scheduleAtFixedRate(keyInput, 0L, 10L, TimeUnit.MILLISECONDS);
        bulletExecutor.scheduleAtFixedRate(bulletMove, 0L, 15L, TimeUnit.MILLISECONDS);
        obstacleExecutor.scheduleAtFixedRate(obstacleMove, 0L, 10L, TimeUnit.MILLISECONDS);

        setFocusable(true);
    }

    @Override
    protected void paintComponent(Graphics g) {
        super.paintComponent(g);

        if (state == stateOfGame.GAME) {
            spaceShip.paint(g);
            healthBar.paint(g);

            for (Bullet bullet : bullets)
                if (bullet.isBulletActive())
                    bullet.paint(g);

            for (Obstacle obstacle : obstacles)
                if (obstacle.isObstacleActive())
                    obstacle.paint(g);
        } else if (state == stateOfGame.MENU) {
            menu.paint(g);
        } else if (state == stateOfGame.HIGH_SCORE) {
            allTimeLeaders.paint(g);
        }
    }

    public void startBullet() {
        for (Bullet bullet : bullets)
            if (!bullet.isBulletActive()) {
                bullet.setBySpaceShip(spaceShip);
                break;
            }
    }

    public void addPoints() {
        int ADD_POINTS = 5;
        points += ADD_POINTS;
    }

    public int getPoints() {
        return points;
    }

    public String getName() {
        return playerName;
    }

    public void setName(String s) {
        playerName = s;
    }

    public void endOfLife() {
        loadedHighScoreFile.add(playerName, points);
        loadedHighScoreFile.sortHighScore();
        allTimeLeaders.update(loadedHighScoreList);
        state = GamePanel.stateOfGame.HIGH_SCORE;
    }

    public void exitGame() {
        fileReaderAndWriter.save(loadedHighScoreFile);
        System.exit(0);
    }

    public void resetGame() {
        collisionDetector.resetObstacles();
        healthBar.resetHealthBar();
    }
}

Spaceship class

package com.company;

import javax.imageio.ImageIO;
import java.awt.*;
import java.awt.image.BufferedImage;

public class SpaceShip {
    private int xSpaceShipPosition = 400;
    private int ySpaceShipPosition = 600;
    private BufferedImage spaceShipImage;

    public SpaceShip() throws Exception {
        loadImages();
    }

    public void move(GamePanel.SpaceShipWay way) {
        final int SPACE_SHIP_MOVE = 5;
        if (GamePanel.SpaceShipWay.LEFT == way && xSpaceShipPosition >= 0)
            xSpaceShipPosition -= SPACE_SHIP_MOVE;
        else if (GamePanel.SpaceShipWay.RIGHT == way && xSpaceShipPosition <= 730)
            xSpaceShipPosition += SPACE_SHIP_MOVE;
    }

    public int getXSpaceShipPosition() {
        return xSpaceShipPosition;
    }

    public int getYSpaceShipPosition() {
        return ySpaceShipPosition;
    }

    public void paint(Graphics g) {
        g.drawImage(spaceShipImage, xSpaceShipPosition, ySpaceShipPosition, null);
    }

    public void loadImages() throws Exception {
        spaceShipImage = ImageIO.read(getClass().getResource
                ("/com/images/spaceship.png"));
    }
}

Bullet class

package com.company;

import javax.imageio.ImageIO;
import java.awt.*;
import java.awt.image.BufferedImage;

public class Bullet {
    private Boolean isActive = false;
    private final int BULLET_MOVE = 10;
    private final int TO_CENTER = 29;
    private int xBulletPosition, yBulletPosition;
    private BufferedImage bulletImage;

    public Bullet() throws Exception {
        loadImages();
    }

    public void setBySpaceShip(SpaceShip spaceShip) {
        xBulletPosition = spaceShip.getXSpaceShipPosition() + TO_CENTER;
        yBulletPosition = spaceShip.getYSpaceShipPosition();
        isActive = true;
    }

    public void move() {
        if (yBulletPosition < 0)
            isActive = false;

        if (isActive) {
            yBulletPosition -= BULLET_MOVE;
        }
    }

    public int getBulletXPosition() {
        return xBulletPosition;
    }

    public int getBulletYPosition() {
        return yBulletPosition;
    }

    public boolean isBulletActive() {
        return isActive;
    }

    public void changeBulletActive() {
        isActive = !isActive;
    }

    public void paint(Graphics g) {
        g.drawImage(bulletImage, xBulletPosition, yBulletPosition, null);
    }

    public void loadImages() throws Exception {
        bulletImage = ImageIO.read(getClass().getResource
                ("/com/images/bullet.png"));
    }
}

Obstacle class

package com.company;

import javax.imageio.ImageIO;
import java.awt.*;
import java.awt.image.BufferedImage;
import java.util.Random;

public class Obstacle {
    private boolean isActive = true;
    private int chooser;
    private int xObstaclePosition;
    private int yObstaclePosition;
    private int xDirection;
    private int yDirection;
    private final int RIGHT_WALL = Main.getWidthGameFrame() - 32;
    private Random randomGenerator;
    private BufferedImage obstacleImage;
    private BufferedImage venus, mars, planet;
    private BufferedImage earth, jupiter, uranus;

    public Obstacle() throws Exception {
        loadImages();
        randomGenerator = new Random();
        this.xObstaclePosition = 2 * (randomGenerator.nextInt(250) + 50);
        this.yObstaclePosition = -2 * (randomGenerator.nextInt(125) + 25);
        this.xDirection = randomGenerator.nextInt(5) + 2;
        this.yDirection = randomGenerator.nextInt(5) + 2;
        chooser = randomGenerator.nextInt(6);
        chooseImage();
    }

    public void move() {
        if (xObstaclePosition + xDirection <= 0 || xObstaclePosition + xDirection >= RIGHT_WALL)
            xDirection = -xDirection;

        if (yObstaclePosition > Main.getHeightGameFrame())
            isActive = false;

        xObstaclePosition += xDirection;
        yObstaclePosition += yDirection;
    }

    public int getXObstaclePosition() {
        return xObstaclePosition;
    }

    public int getYObstaclePosition() {
        return yObstaclePosition;
    }

    public boolean isObstacleActive() {
        return isActive;
    }

    public int getXDirection() {
        return xDirection;
    }

    public int getYDirection() {
        return yDirection;
    }

    public void changeXDirection(int xDirection) {
        this.xDirection = xDirection;
    }

    public void changeYDirection(int yDirection) {
        this.yDirection = yDirection;
    }

    public void changeObstacleActive() {
        isActive = !isActive;
    }

    public void setObstacle() {
        this.xObstaclePosition = randomGenerator.nextInt(500) + 100;
        this.yObstaclePosition = -(randomGenerator.nextInt(250) + 50);
        this.xDirection = randomGenerator.nextInt(5) + 1;
        this.yDirection = randomGenerator.nextInt(5) + 1;
        isActive = true;
    }

    private void chooseImage() {
        switch (chooser) {
            case 0:
                obstacleImage = venus;
                break;

            case 1:
                obstacleImage = earth;
                break;

            case 2:
                obstacleImage = jupiter;
                break;

            case 3:
                obstacleImage = uranus;
                break;

            case 4:
                obstacleImage = mars;
                break;

            case 5:
                obstacleImage = planet;
                break;
        }
    }

    public void paint(Graphics g) {
        g.drawImage(obstacleImage, xObstaclePosition, yObstaclePosition, null);
    }

    private void loadImages() throws Exception {
        venus = ImageIO.read(getClass().getResource
                ("/com/images/venus.png"));
        earth = ImageIO.read(getClass().getResource
                ("/com/images/earth.png"));
        jupiter = ImageIO.read(getClass().getResource
                ("/com/images/jupiter.png"));
        uranus = ImageIO.read(getClass().getResource
                ("/com/images/uranus.png"));
        mars = ImageIO.read(getClass().getResource
                ("/com/images/mars.png"));
        planet = ImageIO.read(getClass().getResource
                ("/com/images/planet.png"));
    }
}

HealthBar class

package com.company;

import java.awt.*;

public class HealthBar {
    private int health = 200;
    private int healthSubstaction = 20;

    public void substractHealth() {
        health -= healthSubstaction;
    }

    public void resetHealthBar() {
        health = 200;
    }

    public boolean isEnd() {
        return health < 10;
    }

    public void paint(Graphics g) {
        g.setColor(Color.gray);
        g.fillRect(5, 5, 200, 50);

        g.setColor(Color.green);
        g.fillRect(5, 5, health, 50);

        g.setColor(Color.white);
        g.drawRect(5, 5, 200, 50);
    }
}

Menu class

package com.company;

import java.awt.*;
import java.awt.image.BufferedImage;
import java.io.IOException;
import javax.imageio.ImageIO;

public class Menu {
    private BufferedImage playImage, helpImage, exitImage;
    public static final int X_IMAGE_POSITION = 251;

    public Menu() throws IOException {
        playImage = ImageIO.read(getClass().getResource
                ("/com/images/play.png"));
        helpImage = ImageIO.read(getClass().getResource
                ("/com/images/help.png"));
        exitImage = ImageIO.read(getClass().getResource
                ("/com/images/exit.png"));
    }

    public void paint(Graphics g) {
        g.drawImage(playImage, X_IMAGE_POSITION, 220, null);
        g.drawImage(helpImage, X_IMAGE_POSITION, 340, null);
        g.drawImage(exitImage, X_IMAGE_POSITION, 460, null);

        g.setColor(Color.white);
        Font highScore = new Font("arial", Font.BOLD, 40);
        g.setFont(highScore);
        g.drawString("High-Score", 300, 600);
        g.drawRect(300, 560, 220, 50);
    }
}

KeyInput class

package com.company;

import java.awt.event.ActionEvent;
import java.awt.event.KeyEvent;
import javax.swing.AbstractAction;
import javax.swing.ActionMap;
import javax.swing.InputMap;
import javax.swing.JComponent;
import javax.swing.KeyStroke;
import java.util.EnumMap;

public final class KeyInput implements Runnable {
    private static final String PRESSED = "pressed";
    private static final String RELEASED = "released";
    private final EnumMap<Key, Boolean> keyMap;
    private final GamePanel gPanel;
    private final SpaceShip spaceShip;
    private int countForBullet = 0;

    public KeyInput(GamePanel gPanel, SpaceShip spaceShip) {
        this.keyMap = new EnumMap<>(Key.class);
        this.gPanel = gPanel;
        this.spaceShip = spaceShip;

        resetKeyInput();

        ActionMap actionMap = gPanel.getActionMap();
        int condition = JComponent.WHEN_IN_FOCUSED_WINDOW;
        InputMap inputMap = gPanel.getInputMap(condition);

        for (Key key : Key.values()) {
            KeyStroke pressedKeyStroke = KeyStroke.getKeyStroke
                    (key.getKeyCode(), 0, false);
            KeyStroke releasedKeyStroke = KeyStroke.getKeyStroke
                    (key.getKeyCode(), 0, true);

            inputMap.put(pressedKeyStroke, key.getText() + PRESSED);
            inputMap.put(releasedKeyStroke, key.getText() + RELEASED);
            actionMap.put(key.getText() + PRESSED, new MyArrowBinding(key, false));
            actionMap.put(key.getText() + RELEASED, new MyArrowBinding(key, true));
        }
    }

    @Override
    public void run() {
        if (GamePanel.state == GamePanel.stateOfGame.GAME) {
            int order = 1;

            gPanel.repaint();

            for (Key key : keyMap.keySet()) {
                if (keyMap.get(key) && order == 1) {
                    spaceShip.move(GamePanel.SpaceShipWay.LEFT);
                    gPanel.repaint();
                } else if (keyMap.get(key) && order == 2) {
                    spaceShip.move(GamePanel.SpaceShipWay.RIGHT);
                    gPanel.repaint();
                } else if (keyMap.get(key) && order == 3) {

                    if (++countForBullet == 10) {
                        gPanel.startBullet();
                        countForBullet = 0;
                    }

                    gPanel.repaint();
                }
                order++;
            }
        }
    }

    private void resetKeyInput() {
        keyMap.put(Key.LEFT, false);
        keyMap.put(Key.RIGHT, false);
        keyMap.put(Key.SPACE, false);
    }

    private class MyArrowBinding extends AbstractAction {
        private final Key key;
        private final boolean released;

        public MyArrowBinding(Key key, boolean released) {
            this.key = key;
            this.released = released;
        }

        @Override
        public void actionPerformed(ActionEvent aEvt) {
            keyMap.put(key, !released);
        }
    }

    private enum Key {
        LEFT("Left", KeyEvent.VK_LEFT),
        RIGHT("Right", KeyEvent.VK_RIGHT),
        SPACE("Space", KeyEvent.VK_SPACE);

        private final String text;
        private final int keyCode;

        Key(String text, int keyCode) {
            this.text = text;
            this.keyCode = keyCode;
        }

        public String getText() {
            return text;
        }

        public int getKeyCode() {
            return keyCode;
        }
    }
}

MouseInput class

package com.company;

import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;
import javax.swing.JOptionPane;
import javax.swing.JTextField;

public class MouseInput extends MouseAdapter {
    private JTextField playerName = new JTextField();
    private Object[] message = {"Name of player : ", playerName};
    private final GamePanel gPanel;
    private final String helpInfo = "This is a simple SpaceWar game.\n" +
            "Use arrows to move the spaceship and Spaca to hit.\n";

    public MouseInput(GamePanel gPanel) {
        this.gPanel = gPanel;
    }

    @Override
    public void mousePressed(MouseEvent e) {
        int xMousePosition = e.getX();
        int yMousePosition = e.getY();

        if (GamePanel.state == GamePanel.stateOfGame.MENU &&
                xMousePosition > Menu.X_IMAGE_POSITION && xMousePosition < 551) {
            if (yMousePosition > 220 && yMousePosition < 290) {
                int option = JOptionPane.showConfirmDialog(null, message,
                        "Name of players", JOptionPane.OK_CANCEL_OPTION);

                if (option == JOptionPane.OK_OPTION && !playerName.getText().isEmpty()) {
                    gPanel.setName(playerName.getText());
                    gPanel.resetGame();
                    GamePanel.state = GamePanel.stateOfGame.GAME;
                }
            } else if (yMousePosition > 340 && yMousePosition < 410) {
                JOptionPane.showMessageDialog(gPanel, helpInfo,
                        "Help information", JOptionPane.INFORMATION_MESSAGE);
            } else if (yMousePosition > 460 && yMousePosition < 530) {
                gPanel.exitGame();
            } else if (yMousePosition > 560 && yMousePosition < 620)
                GamePanel.state = GamePanel.stateOfGame.HIGH_SCORE;
            gPanel.repaint();
        }

        if (GamePanel.state == GamePanel.stateOfGame.HIGH_SCORE &&
                xMousePosition > 550 && xMousePosition < 710 &&
                yMousePosition > 560 && yMousePosition < 610) {
            GamePanel.state = GamePanel.stateOfGame.MENU;
            gPanel.repaint();
        }
    }
}

PlayerScore class

package com.company;

import java.io.Serializable;

public class PlayerScore implements Comparable<PlayerScore>, Serializable {
    private String nameOfPlayer;
    private int score;
    private static final long serialVersionUID = 1L;

    PlayerScore(String nameOfPlayer, int score) {
        this.nameOfPlayer = nameOfPlayer;
        this.score = score;
    }

    public int getScore() {
        return score;
    }

    public String getNameOfPlayer() {
        return nameOfPlayer;
    }

    @Override
    public int compareTo(PlayerScore highScore) {
        if (this.score > highScore.getScore())
            return 1;
        else if (this.score < highScore.getScore())
            return -1;
        else
            return 0;
    }
}

CollisionDetector class

package com.company;

import java.util.LinkedList;

public class CollisionDetector {

    private final int IMAGE_RANGE = 60, OBSTACLE_RANGE = 32;
    private final int NUMBER_OF_OBSTACLES = 6;
    private LinkedList<Bullet> bullets;
    private LinkedList<Obstacle> obstacles;
    private SpaceShip spaceShip;
    private HealthBar healthBar;
    private GamePanel gPanel;

    public CollisionDetector(GamePanel gPanel, SpaceShip spaceShip, HealthBar healthBar) throws Exception {
        this.gPanel = gPanel;
        this.spaceShip = spaceShip;
        this.healthBar = healthBar;

        bullets = new LinkedList<>();
        obstacles = new LinkedList<>();

        fillBullet();
        fillObstacles();
    }

    public void checkSpaceShipCollision() {
        for (Obstacle obstacle : obstacles) {
            if (obstacle.isObstacleActive() && GamePanel.state == GamePanel.stateOfGame.GAME &&
                    obstacle.getYObstaclePosition() + OBSTACLE_RANGE > spaceShip.getYSpaceShipPosition()
                    && obstacle.getYObstaclePosition() < spaceShip.getYSpaceShipPosition() + IMAGE_RANGE
                    && obstacle.getXObstaclePosition() < spaceShip.getXSpaceShipPosition() + IMAGE_RANGE
                    && obstacle.getXObstaclePosition() + OBSTACLE_RANGE > spaceShip.getXSpaceShipPosition()) {

                if (!healthBar.isEnd()) {
                    healthBar.substractHealth();
                    obstacle.changeObstacleActive();
                } else {
                    gPanel.endOfLife();
                }
                gPanel.repaint();
            }
        }
    }

    public void checkObstacleCollision() {
        for (int i = 0; i < NUMBER_OF_OBSTACLES - 1; i++)
            for (int j = i + 1; j < NUMBER_OF_OBSTACLES; j++)
                if (GamePanel.state == GamePanel.stateOfGame.GAME &&
                        obstacles.get(i).getYObstaclePosition() + OBSTACLE_RANGE > obstacles.get(j).getYObstaclePosition()
                        && obstacles.get(i).getYObstaclePosition() < obstacles.get(j).getYObstaclePosition() + OBSTACLE_RANGE
                        && obstacles.get(i).getXObstaclePosition() < obstacles.get(j).getXObstaclePosition() + OBSTACLE_RANGE
                        && obstacles.get(i).getXObstaclePosition() + OBSTACLE_RANGE > obstacles.get(j).getXObstaclePosition()) {

                    int tempXDirection = obstacles.get(i).getXDirection();
                    int tempYDirection = obstacles.get(i).getYDirection();

                    obstacles.get(i).changeXDirection(obstacles.get(j).getXDirection());
                    obstacles.get(i).changeYDirection(obstacles.get(j).getYDirection());

                    obstacles.get(j).changeXDirection(tempXDirection);
                    obstacles.get(j).changeYDirection(tempYDirection);
                }
    }

    public void checkBulletCollision() {
        for (Bullet bullet : bullets)
            for (Obstacle obstacle : obstacles) {
                if (GamePanel.state == GamePanel.stateOfGame.GAME && bullet.isBulletActive() && obstacle.getXObstaclePosition() < bullet.getBulletXPosition() &&
                        bullet.getBulletXPosition() < obstacle.getXObstaclePosition() + OBSTACLE_RANGE &&
                        obstacle.getYObstaclePosition() < bullet.getBulletYPosition() &&
                        bullet.getBulletYPosition() < obstacle.getYObstaclePosition() + OBSTACLE_RANGE) {
                    bullet.changeBulletActive();
                    obstacle.changeObstacleActive();
                    gPanel.addPoints();
                }
            }
    }

    private void fillBullet() throws Exception {
        int numberOfBullets = 10;
        for (int i = 0; i < numberOfBullets; i++)
            bullets.add(new Bullet());
    }

    private void fillObstacles() throws Exception {
        for (int i = 0; i < NUMBER_OF_OBSTACLES; i++)
            obstacles.add(new Obstacle());
    }

    public void resetObstacles() {
        for (Obstacle obstacle : obstacles)
            obstacle.setObstacle();
    }

    public LinkedList<Bullet> getBullets() {
        return bullets;
    }

    public LinkedList<Obstacle> getObstacles() {
        return obstacles;
    }
}

AllTimeLeaders class

package com.company;

import java.awt.*;
import java.util.LinkedList;

public class AllTimeLeaders {
    private GamePanel gamePanel;
    private LinkedList<PlayerScore> highScoreList;

    AllTimeLeaders(GamePanel gamePanel, LinkedList<PlayerScore> highScoreList) {
        this.gamePanel = gamePanel;
        this.highScoreList = highScoreList;
    }

    public void update(LinkedList<PlayerScore> highScoreList) {
        this.highScoreList = highScoreList;
    }

    public void paint(Graphics g) {
        Font titleFont = new Font("arial", Font.BOLD, 50);
        Font resultFont = new Font("arial", Font.BOLD, 20);
        Font allTimeLeadersFont = new Font("arial", Font.BOLD, 30);
        Font backString = new Font("arial", Font.BOLD, 50);
        g.setColor(Color.WHITE);

        g.setFont(backString);
        g.drawString("BACK", 550, 600);
        g.drawRect(550, 560, 160, 50);

        g.setFont(titleFont);
        g.drawString("GAME OVER", Main.getWidthGameFrame() / 2 - 180, 100);

        g.setFont(allTimeLeadersFont);
        g.drawString("All-time results", 270, 250);

        g.setFont(resultFont);
        g.drawString("Your score : " + gamePanel.getPoints(), 50, 180);

        int i = 1;
        for (PlayerScore playerScore : highScoreList) {
            g.drawString(i + ". " + playerScore.getNameOfPlayer(), 50, 280 + 40 * i);
            g.drawString("" + playerScore.getScore(), 200, 280 + 40 * i);
            if (++i == 11)
                break;
        }
    }
}

HighScoreFile class

package com.company;

import java.io.Serializable;
import java.util.Collections;
import java.util.LinkedList;

public class HighScoreFile implements Serializable {

    private static final long serialVersionUID = 43242L;
    private LinkedList<PlayerScore> highScoreList = new LinkedList<>();

    HighScoreFile() {
        sortHighScore();
    }

    public void add(String playerName, int playerScore) {
        highScoreList.add(new PlayerScore(playerName, playerScore));
    }

    public void sortHighScore() {
        Collections.sort(highScoreList, Collections.reverseOrder());
    }

    public LinkedList<PlayerScore> getHighScoreList() {
        return highScoreList;
    }
}

FileReaderAndWriter class

package com.company;

import java.io.*;

public class FileReaderAndWriter {

    private final String fileName = "/Score.sav";
    private HighScoreFile loadedHighScoreFile = null;

    public void save(Serializable objectToSerialize) {
        try {
            FileOutputStream fileOutputStream = new FileOutputStream(createDataFolder() + fileName);
            ObjectOutputStream objectOutputStream = new ObjectOutputStream(fileOutputStream);
            objectOutputStream.writeObject(objectToSerialize);

            objectOutputStream.flush();
            objectOutputStream.close();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    public void load() throws ClassNotFoundException, IOException {
        if (checkFileExists()) {

            FileInputStream fileInputStream = new FileInputStream(createDataFolder() + fileName);
            ObjectInputStream objectInputStream = new ObjectInputStream(fileInputStream);
            loadedHighScoreFile = (HighScoreFile) objectInputStream.readObject();

            objectInputStream.close();
        }
    }

    private boolean checkFileExists() {
        return new File(createDataFolder() + fileName).isFile();
    }

    private String createDataFolder() {
        String homeLocation = System.getProperty("user.home");
        String OS = System.getProperty("os.name").toLowerCase();

        if (OS.contains("win"))
            homeLocation = System.getenv("appdata");

        else if (OS.contains("mac"))
            homeLocation += "~/Library/Application Support";

        else if (OS.contains("nix") || OS.contains("aix") || OS.contains("nux"))
            homeLocation += "~/.";

        File dir = new File(homeLocation);
        String root = "/SpaceWar";
        dir = new File(dir, root);

        if (!dir.exists())
            dir.mkdir();

        return dir.getAbsolutePath();
    }

    public HighScoreFile getHighScoreFile() {
        return loadedHighScoreFile;
    }
}
\$\endgroup\$
6
\$\begingroup\$

Well, then let's tear this apart piece by piece.

We will start with your questions:

One of them is "access can be package-private". I know what it means, it tries to "narrow down" the access. So, according to this I should have written for example void myMethod() instead of public void myMethod(). The question : should I be obedient and change everything? I am confused because a lot of code is written in "bad" way. Simply, listen to the compiler, right?

Yes, or at least mostly "yes". You still have to distinguish whether a method or property should be private by design, or if you are just not using it publicly yet. In the latter case you may ignore the warning, but in the first case, IntelliJ is most likely right.

Simple rule: High inner, low outer connectivity. Your classes and packages should only expose the bare minimum to the outside, and keep the implementation specific details hidden.

Other warning is "field can be converted to a local variable". Don't make (private final int SOMETHING = 1231) and make this in appropiate method, right?

When it's a magic number, ignore that hint. The IDE will warn because it's only used once, but being verbose doesn't hurt this time.


However, that doesn't apply e.g. to these two:

private static int WIDTH_GAME_FRAME = 800;
private static int HEIGHT_GAME_FRAME = 700;

While these two are obviously magic numbers, you are being very inconsequent by deriving a lot of other magic numbers in your codebase (essentially all screen space coordinates!) based on the value of these two.

Take e.g. the bounding boxes in MouseInput, every single occasion where you are drawing an UI element at a fixed location, or even the ingame object placement.

All of these should have been derived from these two constants!


You absolutely love writing duplicate code, don't you?

xBulletPosition = spaceShip.getXSpaceShipPosition() + TO_CENTER;
yBulletPosition = spaceShip.getYSpaceShipPosition();

In every single location in your codebase where you need to handle either 2D coordinates, or 2D bounding boxes, you always chose to store each single component in an individual variable.

Why?

But you didn't just duplicate the pattern of storing the components individually, you also typed the bounding box tests over an over again manually.

It's so simple to solve that, just group 2D coordinates into a Point object, and bounding boxes into Box object. The Box class should also contain the commonly used methods for test for Box with Box collisions, and Point in Box inclusions.


What could possibly go wrong?...

if (!healthBar.isEnd()) {
    healthBar.substractHealth();
    obstacle.changeObstacleActive();
} else {
    gPanel.endOfLife();
}

So if the player was already dead, he actually dies only on the next collision. But until you let him play, even if the health bar goes already into the negative.

Well, it doesn't, since your implementation of the health bar actually hit's exactly 0. But this starts to bug out the second you allow custom damage values for the health bar.


What happened here?

keyExecutor.scheduleAtFixedRate(keyInput, 0L, 10L, TimeUnit.MILLISECONDS);
bulletExecutor.scheduleAtFixedRate(bulletMove, 0L, 15L, TimeUnit.MILLISECONDS);
obstacleExecutor.scheduleAtFixedRate(obstacleMove, 0L, 10L, TimeUnit.MILLISECONDS)

You just spun off 3 threads. The one handling the input is acceptable, you don't want that one to be blocked by anything else.

But what about the other two?

That stuff belongs into the actual game loop. Which you don't have in your design. It would look pretty much like this, if you had one:

while (GamePanel.state == GamePanel.stateOfGame.GAME) {
    updateShip();
    updateBullets();
    updateObstacles();

    checkCollisions();

    gPanel.repaint();
    Sleep(UPDATE_INTERVAL);
}

And that also brings me to the next point:

gPanel.repaint();
gPanel.repaint();
gPanel.repaint();
gPanel.repaint();
gPanel.repaint();
gPanel.repaint();
gPanel.repaint();
gPanel.repaint();
gPanel.repaint();

Have you even counted how often you tried to repaint the panel? If you type the same command over and over again, all over the place, something smells really fishy.

In this case it was the indicator, that several components which should have been managed by the GamePanel (well, actually not even that, but by the game loop!) instead took control over it.

If you take a look at the proposed game loop - you only call that repaint method once after all components have been updated.


So this is the game surface?

public class GamePanel extends JPanel {
    ...
}

Eh, nope. Looks like you actually managed to bake the game state and logic into the class which was only supposed to be responsible for presentation.

Well, at least that means the setup of the game state is still in a single location?

bullets = collisionDetector.getBullets();
obstacles = collisionDetector.getObstacles();

WTF? Why is that part of the setup suddenly part of CollisionDetector?

This class is apparently strongly coupled with GamePanel. Once again an indicator that the guideline about low outer connectivity is violated.


Let's take a look at "CollisionDetector" then. Actually, that class would make a fine container for the game state, if it wasn't for misleading naming.

What that class actually does, is that it handles most of the actual game logic. Do yourself a favor, and strip the stuff which doesn't belong to GamePanel from that class and move if to CollisionDetector where it belongs. Oh, and give that class a proper name which actually describes it's function.


And on we go...

public void loadImages() throws Exception {
    bulletImage = ImageIO.read(getClass().getResource
            ("/com/images/bullet.png"));
}

Problem with that? You managed to do this for every single instance of Bullet. Some problem with Obstacle and Spaceship.

In your simple application, the effect may be minor. You only have 10 bullets, 6 obstacles and 1 ship in your game.

But that doesn't change the fact that you managed to keep 10 identical copies of the very same image in memory.

This is actually not so simple to solve. The problem is, that you encapsulated resource management entirely within each instance.

Solving this requires a different pattern. Treat Bullet, Obstacle and Spaceship solely as data containers, holding only the attributes unique to each instance. These containers don't know anything about being displayed.

Manage the resources, and also the painting, in a central location. This actually fit's very well into the GamePanel class. That class only needs to hold one copy of each texture, and the simply iterates over the list of positions at which the instances should be drawn.

Alternatively, at least load the resources only once per class, and not per instance.


Just to avoid confusion, the former paragraph isn't applicable for AllTimeLeaders, Menu and alike.

However, these are not exactly clean either.

Abusing a public void paint(Graphics g) to pass in the render surface of the parent JPanel is a horrible hack. Especially when you then continue to render to absolute coordinates in the parents screenspace.

At least have the decency to leave the layout to GamePanel. That is simply reached by additionally passing x and y offsets to paint, so that these components (at least internally) don't need to know the layout.

The clean solution would be not to let these components draw directly to the render surface, but to use separate panes instead, completely separating them from the parent component.


String homeLocation = System.getProperty("user.home");

else if (OS.contains("mac"))
    homeLocation += "~/Library/Application Support";

else if (OS.contains("nix") || OS.contains("aix") || OS.contains("nux"))
    homeLocation += "~/.";

Bonus points for wanting to support *nix systems. All gone for totally messing it up.

~/, if at the start of a path, is already the home directory. System.getProperty("user.home") + "~/." is not a valid path.


private boolean checkFileExists() {
    return new File(createDataFolder() + fileName).isFile();
}

Argh. You had it right further down in the same file. The correct syntax is File(createDataFolder(), fileName).

But not to worry too much about that single line. The whole class FileReaderAndWriter smells. For some strange reason, you were obsessed with passing around directory and file names as String, when the proper datatype would have been File.

String fileName = "/Score.sav";
String root = "/SpaceWar";

And please don't use a trailing / unless you know exactly what you are doing. You don't need it when using the File class to build paths. Respectively, it doesn't even do what you think it does.


LinkedList<PlayerScore> loadedHighScoreList = new LinkedList<PlayerScore>();

You know there are other implementations of the List interface than just LinkedList, right? You are not even using any implementation specific behavior, so that declaration is overly specific.

When declaring the variable, only declare it to be the interface you are going to use. The specific implementation only needs to be specified when initializing it. Or not at all, when using e.g. a factory.

In this case, the only interface you were interested in is the List one:

List<PlayerScore> loadedHighScoreList = new LinkedList<PlayerScore>();

This also goes for return types and method parameters. You should never require a specific type, if other implementations of the used interface work perfectly well either.


I'm going to stop here for now. There are still many undiscussed problems with that code base, but after resolving the already mentioned ones, the code base will already look entirely different.

| improve this answer | |
\$\endgroup\$

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.