What is java pong?

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.

Code Review

Pong game in Java

Asked 8 years, 10 months ago

Modified 3 years, 2 months ago

Viewed 98k times

16

6

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 update() {

        ball.update();

        player1.update();

        player2.update();

    }

    public void actionPerformed(ActionEvent e) {

        update();

        repaint();

    }

    public void keyPressed(KeyEvent e) {

        player1.pressed(e.getKeyCode());

        player2.pressed(e.getKeyCode());

    }

    public void keyReleased(KeyEvent e) {

        player1.released(e.getKeyCode());

        player2.released(e.getKeyCode());

    }

    public void keyTyped(KeyEvent e) {

        ;

    }

    @Override

    public void paintComponent(Graphics g) {

        super.paintComponent(g);

        g.drawString(game.getPanel().getScore(1) + " : " + game.getPanel().getScore(2), game.getWidth() / 2, 10);

        ball.paint(g);

        player1.paint(g);

        player2.paint(g);

    }

}

Ball class:

import java.awt.Graphics;

import java.awt.Rectangle;

import javax.swing.JOptionPane;

public class Ball {

    private static final int WIDTH = 30, HEIGHT = 30;

    private Pong game;

    private int x, y, xa = 2, ya = 2;

    public Ball(Pong game) {

        this.game = game;

        x = game.getWidth() / 2;

        y = game.getHeight() / 2;

    }

    public void update() {

        x += xa;

        y += ya;

        if (x < 0 xss=removed xss=removed> game.getWidth() - WIDTH - 7) {

            game.getPanel().increaseScore(2);

            x = game.getWidth() / 2;

            xa = -xa;

        }

        else if (y < 0> game.getHeight() - HEIGHT - 29)

            ya = -ya;

        if (game.getPanel().getScore(1) == 10)

            JOptionPane.showMessageDialog(null, "Player 1 wins", "Pong", JOptionPane.PLAIN_MESSAGE);

        else if (game.getPanel().getScore(2) == 10)

            JOptionPane.showMessageDialog(null, "Player 2 wins", "Pong", JOptionPane.PLAIN_MESSAGE);

        checkCollision();

    }

    public void checkCollision() {

        if (game.getPanel().getPlayer(1).getBounds().intersects(getBounds()) || game.getPanel().getPlayer(2).getBounds().intersects(getBounds()))

            xa = -xa;

    }

    public Rectangle getBounds() {

        return new Rectangle(x, y, WIDTH, HEIGHT);

    }

    public void paint(Graphics g) {

        g.fillRect(x, y, WIDTH, HEIGHT);

    }

}

Racket class:

import java.awt.Graphics;

import java.awt.Rectangle;

public class Racket {

    private static final int WIDTH = 10, HEIGHT = 60;

    private Pong game;

    private int up, down;

    private int x;

    private int y, ya;

    public Racket(Pong game, int up, int down, int x) {

        this.game = game;

        this.x = x;

        y = game.getHeight() / 2;

        this.up = up;

        this.down = down;

    }

    public void update() {

        if (y > 0 && y < game xss=removed xss=removed xss=removed xss=removed xss=removed xss=removed xss=removed xss=removed xss=removed xss=removed>

}


How can I improve/optimize my code? Any thoughts are appreciated.


java


game


swing


pong


Share


Improve this question


Follow


edited Feb 7, 2019 at 14:19


user avatar


200_success


141k2121 gold badges184184 silver badges467467 bronze badges


asked Jun 9, 2013 at 12:54


user avatar


LazySloth13


1,29155 gold badges1818 silver badges3636 bronze badges


There are many instance variables which can be made final, you can start with this –


fge


 Jun 9, 2013 at 13:13


Just curious - what is the advantage of making variable names final in this context? –


Michael Zedeler


 Jun 9, 2013 at 15:40


@MichaelZedeler: The main advantage is that you show your intentions "this will never change and is not supposed to change". –


Bobby


 Jun 10, 2013 at 7:31


I'm not sure, but have you considered not fixing the size of the elements (including the JFrame), and instead make them relative? –


Bobby


 Jun 10, 2013 at 7:32


and @ZeroOne to commented only side effects, forgot for most important, 1) don't use KeyListener, use KeyBindings 2) override getPreferredSize for JPanel instead of hardcoding setSize, PreferredSize, getBounds, Rectangle.whatever ...., are uselless, all coordinates for custom painting came from getHeight/Weight, 3) apply these coordinates in paintComponent 4) Ball doesn't required own paint, 5) Timer timer = new Timer(5, this); 5miliseconds is under latency in Native OS, hard by painted in Win8 with 2GPUs, I'd use 25-33 ms 6) could be starting with –


mKorbel


 Jun 10, 2013 at 12:38


Add a comment


1 Answer


Sorted by:


Highest score (default)


15


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:


Java 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 < 0> game.getHeight() - HEIGHT - 29), I'd write else if (hasHitTopOrBottom()) and then define method like this:

private boolean hasHitTopOrBottom() {

    return y < 0> 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 final method call in update(), checkCollision(), also has a misleading name, as collisions with walls have already been checked and now it's time to only check collisions with the rackets. Actually, your update() method should probably just look like this:


public void update() {

    updateLocation();

    checkCollisionWithSides();

    checkVictoryConditions();

    checkCollisionWithRackets(); 

}Also, this class fails to reset the score when the game ends, so after either player wins, the game gets stuck showing the message box on every tick of the timer, even if you try to dismiss it. With this I also recommend you start using curly braces with if-statements, because then it's explicit where the branch block starts and ends. Currently someone who didn't use an IDE with auto formatting might break the code by trying to add the score resets within the if-statements that did not have curly braces.


Racket class:


This class has the same issue as the Ball class: the update() method looks confusing. Turning the conditions into methods and giving them descriptive names will help a lot.


Also, giving the Racket the entire Pong object in the constructor looks suspicious. A Racket implemented by a malicious player could, for example, do game.getPanel().increaseScore(1) calls every time its update() method was called. Just pass the racket its maximum and minimum height and you also avoid doing those "game.getHeight() - HEIGHT - 29" calculations in this class. All in all this would then add two parameters into the constructor and remove one, and externalize some calculations into the PongPanel class that creates those Rackets. Also, isn't it the Panel's responsibility to know the magic number "29", rather than the Racket's?