Pong game in Java

I have recently written the following Pong game in Java:

Pong class:

import java.awt.Color;

import javax.swing.JFrame;

public class Pong extends JFrame {
    private final static int WIDTH = 700, HEIGHT = 450;
    private PongPanel panel;

    public Pong() {
        setSize(WIDTH, HEIGHT);
        setTitle("Pong");
        setBackground(Color.WHITE);
        setResizable(false);
        setVisible(true);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
        panel = new PongPanel(this);
        add(panel);
    }

    public PongPanel getPanel() {
        return panel;
    }

    public static void main(String[] args) {
        new Pong();
    }
}

PongPanel class:

```
import java.awt.Color;
import java.awt.Graphics;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;

import javax.swing.JPanel;
import javax.swing.Timer;

public class PongPanel extends JPanel implements ActionListener, KeyListener {
private Pong game;
private Ball ball;
private Racket player1, player2;
private int score1, score2;

public PongPanel(Pong game) {
setBackground(Color.WHITE);
this.game = game;
ball = new Ball(game);
player1 = new Racket(game, KeyEvent.VK_UP, KeyEvent.VK_DOWN, game.getWidth() - 36);
player2 = new Racket(game, KeyEvent.VK_W, KeyEvent.VK_S, 20);
Timer timer = new Timer(5, this);
timer.start();
addKeyListener(this);
setFocusable(true);
}

public Racket getPlayer(int playerNo) {
if (playerNo == 1)
return player1;
else
return player2;
}

public void increaseScore(int playerNo) {
if (playerNo == 1)
score1++;
else
score2++;
}

public int getScore(int playerNo) {
if (playerNo == 1)
return score1;
else
return score2;
}

private void upd

Overall I find your code quite good as it stands. :) You've got sensible classes with sane, short methods. However, there were a few points that you could improve. Let's look at your solution class by class:

Pong class:

When you turn on strict enough compiler warnings in your IDE, you'll notice it warns about the fields WIDTH and HEIGHT hiding other fields. This is indeed true: public static final int WIDTH and HEIGHT are defined in the ImageObserver interface high up in the inheritance hierarchy of your class. You'll notice you get no errors even if you delete the line where you define those constants. You should come up with unique names for those variables, perhaps something like PLAYING_AREA_WIDTH. That'd also be more descriptive than just plain "WIDTH", which could be the width of the window, or that of the playing area if it wasn't as big as the window, but as of now nobody can know it for sure without inspecting the code more closely and trying the program.

Constructors should also be used just for light weight initialization stuff, but here it starts the entire game by initializing the PongPanel class. This is as much a fault of the PongPanel class, though. I'd add a start() method into both and change the main method of the Pong class to say "Pong game = new Pong(); game.start();". Ignoring new instances of classes, like the main method currently does, is quite confusing. Did the developer just forget to assign it into a variable? What is supposed to happen? game.start() would make it explicit.

PongPanel class:

Here my attention first turned to the actionPerformed, keyPressed, keyReleased and keyTyped methods, as they are all missing an @Override annotation. It's really recommendable to use one, because then you know right away that the method signature is enforced elsewhere. Interfaces are more forgiving with this, as you always get a warning if you lack a required method, but suppose you wanted to override a method that already has an implementation in a super class. Then, without an @Override annotation, someone might change the signature of the method and everything would appear to be just fine, but something would still break somewhere when the subclass's implementation wouldn't be used anymore.

My second observation was the int parameter of the getPlayer, increaseScore and getScore methods. In Pong you aren't likely to have Integer.MAX_VALUE amount of players, or even more than two, so I find an enum would be pretty good here. Init it like enum PlayerId {ONE, TWO}; next to the class fields, and change the method signatures from something like getPlayer(int playerNo) to getPlayer(PlayerId player).

This would also make the increaseScore method more readable, as currently increaseScore(2) looks like you'd increase the score by two points, whereas actually you are increasing the score for player two. Contrast that with increaseScore(PlayerId.TWO), or perhaps with even renaming the method: increaseScoreForPlayer(PlayerId.TWO). Remember, bits and bytes aren't expensive to store, you can use as many of them as is required to get clear and self explaining method names. :)

At the end of the class there's the paintComponent method that correctly does have the @Override annotation. The problem with the method is that it asks the ball and the players for them to paint themselves. This is against the principle of separating the business logic and the presentation logic. The ball and the rackets should not need to know how they are rendered, the user interface should take care of that. The logic of the game stays the same no matter how fancy or crude the game actually looks, and this should be reflected in the design so that the classes with the logic should not change when the appearance of the game changes. I won't go into this with any more detail, but you should think this through and read about it.

Ball class:

In this class the update method looks rather confusing with all the if-statements and the math in their conditions. I'd create methods of the conditions: instead of writing else if (y game.getHeight() - HEIGHT - 29), I'd write else if (hasHitTopOrBottom()) and then define method like this:

private boolean hasHitTopOrBottom() {
    return y  game.getHeight() - HEIGHT - 29;
}

Do that for all the conditions and now, when you read the if-statement, you do not need to stop for a second to immediately understand what's happening in which branch.

Also note that the first two branches of the if-statement, the ones that check the ball hitting the left or right side of the playing area, have two lines of duplicate code, which you should refactor into a new method named, say, resetToMiddle().

After that if - else if -set the game checks whether either player has won. I find this should happen in a separate method, say, checkVictoryConditions(). Now it's there as if it was comparable to the action of checking if the ball hit the walls. The fi

StackExchange Code Review Q#27197, answer score: 15