SpaceWar game in java

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);
t

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 p

StackExchange Code Review Q#141422, answer score: 6