6
\$\begingroup\$

This is my first time ever making a game. It is a pong-like game, where the user has a paddle and has to prevent the ball from passing them. Since this is my first game, I want any recommendations/improvements/advice from more experienced people. The advice can range from simple Java errors I've missed to more game-development technicalities. I seek to make more advanced games in the future so any advice about this simple game can help me a lot.

In addition, I've noticed some bugs in the program, but I cannot seem to find them.

  • The game has a short but noticeable lag spike.
  • The ball gets stuck in the paddle, and that causes the score to rapidly increase.

Here is the code:

Main.java

import java.awt.EventQueue;

import javax.swing.JFrame;

public class Main extends JFrame {
    private static final long serialVersionUID = 1L;

    public Main() {
        initUI();
    }

    private void initUI() {
        add(new PaintSurface());
        this.addKeyListener(new KeyEvents());

        setTitle("Pong");

        setResizable(false);
        pack();

        setLocationRelativeTo(null);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
    }

    public static void main(String[] args) {
        EventQueue.invokeLater(() -> {
            JFrame main = new Main();
            main.setVisible(true);
        });
    }
}

PaintSurface.java

import java.awt.Color;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.RenderingHints;
import java.util.Timer;
import java.util.TimerTask;

import javax.swing.JPanel;

public class PaintSurface extends JPanel {
    private static final long serialVersionUID = 1L;
    private static final Dimension paintSurfaceDimension = new Dimension(400, 600);

    public static Ball ball;
    public static Paddle paddle;

    private Timer timer;

    public static Dimension getPaintSurfaceDimension() {
        return paintSurfaceDimension;
    }

    public PaintSurface() {
        initPaintSurface();
    }

    private void initPaintSurface() {
        setBackground(Color.BLACK);
        setPreferredSize(paintSurfaceDimension);

        generateBall();
        generatePaddle();

        timer = new Timer();
        timer.scheduleAtFixedRate(new ScheduleTask(), 100, 17);
    }

    private void generateBall() {
        final int diameter = 40;
        int x = 0, y = 0;

        ball = new Ball(x, y, diameter);
    }

    private void generatePaddle() {
        final int width = 70, height = 15;
        int x = 5;
        final int y = paintSurfaceDimension.height - height - 5;

        paddle = new Paddle(x, y, width, height);
    }

    public void paintComponent(Graphics g) {
        Graphics2D graphics = (Graphics2D) g;
        graphics.setRenderingHint(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);
        super.paintComponent(graphics);

        ball.setGraphics(graphics);
        paddle.setGraphics(graphics);

        ball.drawBall();
        paddle.drawPaddle();

        graphics.setFont(new Font("Calibri", Font.PLAIN, 30));
        graphics.drawString(Score.score + "", 200, 50);

        graphics.dispose();
    }

    private class ScheduleTask extends TimerTask {
        public void run() {
            ball.move();
            paddle.move();


            System.out.println(Score.score);
            repaint();
        }
    }
}

Ball.java

import java.awt.Color;
import java.awt.Graphics2D;
import java.awt.Toolkit;
import java.awt.geom.Rectangle2D;

public class Ball extends Rectangle2D.Float {
    private static final long serialVersionUID = 1L;

    private Color ballColor = new Color(255, 255, 255);
    float speedX = 10, speedY = 10;

    private Graphics2D graphics;
    public void setGraphics(Graphics2D graphics) {
        this.graphics = graphics;
    }

    public Ball(int x, int y, float diameter) {
        super(x, y, diameter, diameter);
    }

    public void move() {
        collisionDetection();
        scoreDetection();

        x += speedX;
        y += speedY;
    }

    private void collisionDetection() {
        if(x < 0 || x > PaintSurface.getPaintSurfaceDimension().width - width)
            speedX = -speedX;
        if(y < 0 || y > PaintSurface.getPaintSurfaceDimension().height - height)
            speedY = -speedY;

        if(this.intersects(PaintSurface.paddle)) {
            speedY = -speedY;

        }

    }

    private void scoreDetection() {
        if(y >= PaintSurface.getPaintSurfaceDimension().height - PaintSurface.paddle.height - 15)
            Score.score++;
    }

    public void drawBall() {
        graphics.setColor(ballColor);

        graphics.fill(this);

        Toolkit.getDefaultToolkit().sync();
    }
}

Paddle.java

import java.awt.Color;
import java.awt.Graphics2D;
import java.awt.geom.Rectangle2D;

public class Paddle extends Rectangle2D.Float {
    private static final long serialVersionUID = 1L;

    private Color paddleColor = new Color(255, 255, 255);
    private float speedX = 10;
    public boolean movingLeft = false, movingRight = false;

    private Graphics2D graphics;
    public void setGraphics(Graphics2D graphics) {
        this.graphics = graphics;
    }

    public Paddle(int x, int y, int width, int height) {
        super(x, y, width, height);
    }

    public void move() {
        collisionDetection();

        if(movingLeft)
            x -= speedX;
        if(movingRight)
            x += speedX;
    }

    private void collisionDetection() {
        if(x < 0) x = 0;
        if(x > PaintSurface.getPaintSurfaceDimension().width - width) x = 
PaintSurface.getPaintSurfaceDimension().width - width;
    }

    public void drawPaddle() {
        graphics.setColor(paddleColor);

        graphics.fill(this);
    }
}

KeyEvents.java

import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;

public class KeyEvents implements KeyListener {

    public void keyTyped(KeyEvent event) {}

    public void keyPressed(KeyEvent event) {
        switch(event.getKeyCode()) {
        case KeyEvent.VK_LEFT: PaintSurface.paddle.movingLeft = true; break;
        case KeyEvent.VK_RIGHT: PaintSurface.paddle.movingRight = true; break;
        }
    }

    public void keyReleased(KeyEvent event) {
        switch(event.getKeyCode()) {
        case KeyEvent.VK_LEFT: PaintSurface.paddle.movingLeft = false; break;
        case KeyEvent.VK_RIGHT: PaintSurface.paddle.movingRight = false; break;
        }
    }
}

Score.java

public class Score {
    public static int score;
}

Thank you. Any advice is appreciated.

\$\endgroup\$
5
\$\begingroup\$

Main

You have your Main class extend JFrame. This is unnecessary. You are not adding any functionality to the JFrame; you are merely using one.

You are inconsistent in your use of this.. You have it explicitly only for this.addKeyListener(...), and implicitly reference it in add(...), setTitle(...), setResizable(...), and so on. Why the distinction?

Why doesn't initUI() show the frame? Not showing it makes your code more complex, since you have to return the Main object, and then call .setVisible(...).

Simplified code:

public class Main {

    private static void initUI() {
        JFrame frame = new JFrame();

        frame.add(new PaintSurface());
        frame.addKeyListener(new KeyEvents());

        frame.setTitle("Pong");

        frame.setResizable(false);
        frame.pack();

        frame.setLocationRelativeTo(null);
        frame.setDefaultCloseOperation(EXIT_ON_CLOSE);

        frame.setVisible(true);
    }

    public static void main(String[] args) {
        EventQueue.invokeLater(Main::initUI);
    }
}

PaintSurface

Extending JPanel

PaintSurface does need to extend JPanel because it is overriding paintComponent(...). However you should actually use the @Override annotation so you can be sure you're overriding an existing method, instead of accidentally creating a new one. Also, the method should be protected, not public:

public class PaintSurface extends JPanel {
   ...

   @Override
   protected void paintComponent(Graphics g) {
       ...
   }

   ...
}

java.util.Timer & java.util.TimerTask

NO! Absolutely not! Full Stop.

Swing is not a thread-safe GUI. All interactions with Swing components must be on the Event Dispatching Thread (EDT). java.util.Timer and java.util.TimerTask objects do not execute on the EDT, so you'd need to wrap the code in your ScheduleTask::run() method inside an EventQueue.invokeLater() runnable.

Instead, just use javax.swing.Timer which executes on the EDT.

In the ActionListener, update your game state (move ball, paddle, update score) and call paint_surface.repaint().

Graphics2D

Don't store graphics contexts. Just pass them to the paint routines. So instead of:

    ball.setGraphics(graphics);
    paddle.setGraphics(graphics);

    ball.drawBall();
    paddle.drawPaddle();

use:

    ball.drawBall(graphics);
    paddle.drawPaddle(graphics);

And don't dispose of the graphics context yourself, as the code which called you paintComponent(g) method may still be using it.

Do not create objects inside of paint routines. The Font should be created once, during construction of the PaintSurface, and stored in a member variable.

Static Members

Why is ball a static member of PaintSurface? PaintSurface.ball isn't used anywhere.

Why is paddle a static member of PaintSurface? It would be better to pass the PaintSurface object to the Ball constructor, and store a copy of it there, so the ball could access the paint surface size, and paddle object without the need for a static. You could also pass the PaintSurface object to the KeyEvents constructor.

Score

Again, no. A class for a global variable is wrong. The score should simply be a member variable of the PaintSurface.

WTF

I've never used Toolkit.getDefaultToolkit().sync();. It is not necessary. Do your drawing only from the EDT, and only use Swing timers. If background processing is needed (it isn't here), use SwingWorker threads.

Bugs

In addition, I've noticed some bugs in the program, but I cannot seem to find them.

  • The game has a short but noticeable lag spike.
  • The ball gets stuck in the paddle, and that causes the score to rapidly increase.

Code Review is not for debugging help on known bugs, so you're on your own for the ball getting stuck in the paddle.

Using the javax.swing.Timer will help fix your lag issue, but may not entirely remove it. To achieve smooth animation, games typically measure the time from one update to the next, and use the duration and the objects' velocities to determine how far objects should move from their locations at the last update period.

\$\endgroup\$
4
  • \$\begingroup\$ Isn't it only the call to repaint that needs to be wrapped? E.g: private class ScheduleTask extends TimerTask { public void run() { ... SwingUtilities.invokeLater(() -> repaint()); } } \$\endgroup\$ – Björn Lindqvist Dec 30 '19 at 13:26
  • \$\begingroup\$ Just using javax.swing.Timer is the easiest solution but produces choppy animation. \$\endgroup\$ – Björn Lindqvist Dec 30 '19 at 13:42
  • \$\begingroup\$ @BjörnLindqvist ball.move() & paddle.move() don’t sound like they touch the Swing components, but are you sure? They call collisionDetection() and scoreDetection(), which in turn call PaintSurface.getPaintSurfaceDimension(), which sounds like it might query the Swing component for its size (but it doesn’t). So yes, today, you could just SwingUtilities.invokeLater(this::repaint); in this case. But it requires investigation to determine, and while it is ok with this revision of the code, is it future proof, or could a future change invalidate the research? \$\endgroup\$ – AJNeufeld Dec 30 '19 at 17:42
  • \$\begingroup\$ @BjörnLindqvist Fixing the choppy animation requires the steps indicates in the last paragraph of my answer. \$\endgroup\$ – AJNeufeld Dec 30 '19 at 17:43
2
\$\begingroup\$

A trick you can use which is very common in graphics programming is "clamping." Instead of

if(x < 0) x = 0;
    if(x > PaintSurface.getPaintSurfaceDimension().width - width)
        x = PaintSurface.getPaintSurfaceDimension().width - width;

you write:

x = Math.max(0, Math.min(x, PaintSurface.getPaintSurfaceDimension().width - width));

The idiom is so common that many Java programmers have a clamp function in their personal library of utility functions:

public static <T> T clamp(T x, T lo, T hi) {
    return Math.max(lo, Math.min(x, hi));
}
\$\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.