HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Ping-Pong game in Java

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gamepongjavaping

Problem

I wrote a simple game in Java using Swing. Code is working so far but I have no certainty that "spliting" into classes is done well. I would like to know what "design" problems I have in my code. Also I have a problem with understanding exceptions, I don't really know what to do when exceptions occurs. Terminate the program? Print stack trace? Print message on screen and terminate?

GameFrame.java

package tennisgame;

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

public class GameFrame extends JFrame{
    public final static  int WIDTH_GAME_FRAME = 800;
    public final static  int HEIGHT_GAME_FRAME = 700;

     public GameFrame() throws Exception{
       this.setTitle("Game");
       this.setLayout(new BorderLayout());
       this.setSize(WIDTH_GAME_FRAME, HEIGHT_GAME_FRAME);
       this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
       this.setLocationRelativeTo(null);

       Game game = new Game();
       this.add(game.getGamePanel());    
       this.setResizable(false);
       this.setVisible(true);     
    }

    public static void main(String[] args) {

        EventQueue.invokeLater(() -> {
            try{
                GameFrame tg = new GameFrame();
            }
            catch(Exception e){
                System.out.println("ERROR");
            }
        });                        
    }
}


Game.java

```
package tennisgame;

import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

public class Game implements Runnable {

public static enum State {MENU, GAME}
public static enum Direction {LEFT, RIGHT}
private final GamePanel gPanel;
private static State stateOfGame;
private final MouseInput mouseInput;
private final Ball ball = new Ball();
private final int UPDATE_INTERVAL = 15;
private final Paddle player = new Paddle(300, 600);
private final Paddle pc = new Paddle(300, 42);
private boolean win = false;
pr

Solution

Model vs UI

Your classes have basic separation of concerns. But as you can see by yourself some model elements like "Ball" or "Box" or "Paddle" or even "Game" have a dependency to the package java.awt.*. Try to reformulate your model not to point to UI elements. This is not just because you may provide another UI technology like JavaFX. Sure you can do that afterwards. But the real issue is code quality because you satisfy the "single responsibility principle". My advice is to have a look at the observer pattern.

Doing the separation of UI and model is very hard while saying to do so is easy. I even saw experienced developers struggling with it as the do not put the fact into account that a proper separation implicitly makes the UI interchangable. I often heard "we do not need to exchange the UI but we have proper separation". But that is inherent contradictory. I see UI exchangability not as a feature. I see it as a measurement for a pretended separation.
Game state

You formulate the state of the game within an enum. My suggestion is to use a full state pattern. There you not only provide an artefact that says you are in state 1 or in state 2. You also encapsulate the corresponding behaviour and use polimorphism. Currently the state specific behaviour is spread all over the place within if-then-else-statements.

Furthermore you should extend your state handling. The game is a process that follows some process states. And one other important process state is the end of the process. I do not say to model ALL states. But if you have behaviour that should handle the end of the game you should represent it as a separate state.
Application State

I think you mixed "application state" with "game state". State.MENU seems to be semantical anorganic to State.GAME if you model the game state with it. Either you should have two types of state (application state AND game state) or (my suggestion) omit the representation of the application state as the "menu" has nothing to do with the game itself.
Magic numbers

You have some magic numbers left you should name. You already did this with "HEIGHT_FRAME" or "WIDTH_FRAME". But the checks for the mouse position are comparing against magic numbers.
Game vs. game instance

You also mixed some further semantics as a game may be the set of rules you follow OR a concrete game currently in progress. I suggest to separate these responsibilities.
Direction

As the amount of directions are limitted in yur usecase using an enumeration is totally fine. But here again you have external behaviour you can internalize using polymorphism. Especially the move-behaviour can be encapsulated within the enumeration.
Avoid early return-statements

"return", "break" and "continue" are structured goto statements and they come with a heritage.

They break the control flow and say "I am not interested in the statements below me".

I do not say you have to have a single exit per method but I encourage you to have it. My point of view is: Code is never perfect. As code is never perfect it is continuously improved by refactorings. Maybe the statements below the return statement become important at some point or you want to some extract code into a new method then you will have a hard time to do so without reformulating the whole control flow.

But if you think your code fragment does currently not violate the single responsibility principle and will not in the future you will not face the burden of such a refactoring.
Swing event dispatcher thread

If you are working in a single thread environment you have no real problem. But you are working with multiple threads so the Swing-UI can asynchronously change for different reasons. The "Ball" will continue to move and your paddle as well if you press the direction keys.

To enforce a consistent UI state I suggest to make ANY change to your UI in the EDT (event dipatcher thread). Mostly you will not face the problem. But concurrency problems are those that are hard to interprete if they occur.

You already use the "EventQueue" when you start the appication.

Context

StackExchange Code Review Q#154996, answer score: 12

Revisions (0)

No revisions yet.