Java Swing Pong game

Question

I recently created pong game using Java. This is my first time not copying any code from the internet or using any help. While it does work, my code feels well written but no streamlined or smooth (other than the fact that I made additional classes that would help create further code in this project). When playing it, it also does go through the paddle sometimes. Here's the code:

Main

import javax.swing.JFrame;

public class Main {

    public static void main(String[] args) {
        JFrame frame = new JFrame();
        View view = new View();
        Model model = new Model(view);

        frame.setSize(750, 750);
        frame.getContentPane().add(view);
        frame.setVisible(true);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }
}

View

import java.awt.Color;
import java.awt.Font;
import java.awt.Graphics;
import java.awt.Rectangle;

import javax.swing.AbstractAction;
import javax.swing.ActionMap;
import javax.swing.InputMap;
import javax.swing.JPanel;
import javax.swing.KeyStroke;

public class View extends JPanel {

    Model model;
    Rectangle bounds;

    public View() {
        setBackground(Color.BLACK);
        bounds = new Rectangle(0, 0, 705, 670);
    }

    public void setModel(Model model) {
        this.model = model;
    }

    public void addKeyBinding(String name, int keyEvent, boolean pressed, AbstractAction action) {
        InputMap inputMap = getInputMap(WHEN_IN_FOCUSED_WINDOW);
        ActionMap actionMap = getActionMap();

        inputMap.put(KeyStroke.getKeyStroke(keyEvent, 0, !pressed), name);
        actionMap.put(name, action);
    }

    @Override
    public void paintComponent(Graphics g) {
        super.paintComponent(g);
        if (model.getEntities() != null) {
            for (Entity entity : model.getEntities()) {
                entity.paint(g);
            }
            g.setColor(Color.BLUE);
            g.setFont(new Font("Arial", 1, 20));
            g.drawString(model.getPaddleScore(1) + " : " + model.getPaddleScore(2), 350, 20);
        } else {
            System.out.println("Something is wrong with the entities...");
        }
    }

    @Override
    public Rectangle getBounds() {
        return bounds;

    }
}

Entity

import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.Point;

public interface Entity {
    public Dimension getSize();

    public Point getLocation();

    public void setLocation(Point p);

    public void paint(Graphics g);
}

Paddle

import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.Point;
import java.awt.event.KeyEvent;

public class Paddle implements Entity {
    int score = 0;
    int paddleNum;
    int paddleX = 0, paddleY = 0;
    View view;

    Point location = new Point(0, 0);

    public Paddle(int paddleNum) {
        this.paddleNum = paddleNum;
    }

    public void createBindings() {
        if (paddleNum == 1) {
            view.addKeyBinding("leftup.pressed", KeyEvent.VK_W, true, new LeftAction(Direction.LEFT_UP, true));
            view.addKeyBinding("leftup.released", KeyEvent.VK_W, false, new LeftAction(Direction.LEFT_UP, false));
            view.addKeyBinding("leftdown.pressed", KeyEvent.VK_S, true, new LeftAction(Direction.LEFT_DOWN, true));
            view.addKeyBinding("leftdown.released", KeyEvent.VK_S, false, new LeftAction(Direction.LEFT_DOWN, false));
        } else {
            view.addKeyBinding("rightup.pressed", KeyEvent.VK_UP, true, new RightAction(Direction.RIGHT_UP, true));
            view.addKeyBinding("rightup.released", KeyEvent.VK_UP, false, new RightAction(Direction.RIGHT_UP, false));
            view.addKeyBinding("rightdown.pressed", KeyEvent.VK_DOWN, true,
                    new RightAction(Direction.RIGHT_DOWN, true));
            view.addKeyBinding("rightdown.released", KeyEvent.VK_DOWN, false,
                    new RightAction(Direction.RIGHT_DOWN, false));
        }
    }

    @Override
    public Dimension getSize() {
        return new Dimension(25, 100);
    }

    @Override
    public Point getLocation() {
        return new Point(location);
    }

    @Override
    public void setLocation(Point p) {
        location = p;
    }

    public void setView(View view) {
        this.view = view;
    }

    public void resetScore() {
        score = 0;
    }

    public void increaseScore() {
        score++;
    }

    public int getScore() {
        return score;
    }

    @Override
    public void paint(Graphics g) {
        g.setColor(Color.WHITE);
        g.fillRect(getLocation().x, getLocation().y, getSize().width, getSize().height);
    }
}

Ball

import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.Point;

public class Ball implements Entity {
    Point location = new Point(0, 0);
    int x = 0, y = 0;

    public Ball() {
    }

    @Override
    public Dimension getSize() {
        return new Dimension(20, 20);
    }

    @Override
    public Point getLocation() {
        return new Point(location);

    }

    @Override
    public void setLocation(Point p) {
        location = p;
    }

    public void setX(int x) {
        this.x = x;
    }

    public int getX() {
        return x;
    }

    public void setY(int y) {
        this.y = y;
    }

    public int getY() {
        return y;
    }

    @Override
    public void paint(Graphics g) {
        g.setColor(Color.RED);
        g.fillOval(getLocation().x, getLocation().y, getSize().width, getSize().height);
    }
}

Model

import java.awt.Point;
import java.awt.Rectangle;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import javax.swing.JOptionPane;
import javax.swing.Timer;

public class Model {
    Paddle paddle1;
    Paddle paddle2;
    Ball ball;

    static Set<Direction> keys = new HashSet<Direction>(25);
    Timer timer;
    boolean first = false;
    boolean direction = false, axis = false;
    double ballX = 0, ballY = 0;
    double p1X = 0, p1Y = 0;
    double p2X = 0, p2Y = 0;
    double incline = -0.5;
    List<Entity> entities = new ArrayList<Entity>(20);
    View view;

    public Model(View view) {
        this.view = view;
        startTimer();
        view.setModel(this);
    }

    public void startTimer() {
        timer = new Timer(2, new ActionListener() {

            @Override
            public void actionPerformed(ActionEvent arg0) {
                update(view.getBounds());
                view.repaint();
            }
        });
        timer.start();
    }

    public void update(Rectangle bounds) {
        if (paddle1 == null || paddle2 == null || ball == null) {
            paddle1 = new Paddle(1);
            paddle2 = new Paddle(2);
            ball = new Ball();
            ballX = 300;
            ballY = 300;
            p1X = 30;
            p2X = 650;
            p1Y = 350;
            p2Y = 350;
            paddle1.setView(view);
            paddle2.setView(view);
            paddle1.createBindings();
            paddle2.createBindings();
            entities.add(paddle1);
            entities.add(paddle2);
            entities.add(ball);
        }

        if (paddle1.getScore() > 7) {
            JOptionPane.showMessageDialog(view, "Player 1 has won!");
            paddle1.resetScore();
            paddle2.resetScore();
        } else if (paddle2.getScore() > 7) {
            JOptionPane.showMessageDialog(view, "Player 2 has won!");
            paddle1.resetScore();
            paddle2.resetScore();
        }

        setDirection(direction, incline);
        bounce();

        // TODO Add functionality for changing ball location...
        // TODO Don't forget to use collision detection!!!

        if (keys.contains(Direction.LEFT_UP)) {
            p1Y -= 2;
        } else if (keys.contains(Direction.LEFT_DOWN)) {
            p1Y += 2;
        }
        if (keys.contains(Direction.RIGHT_UP)) {
            p2Y -= 2;
        } else if (keys.contains(Direction.RIGHT_DOWN)) {
            p2Y += 2;
        }

        paddle1.setLocation(new Point((int) p1X, (int) p1Y));
        paddle2.setLocation(new Point((int) p2X, (int) p2Y));
        ball.setLocation(new Point((int) ballX, (int) ballY));

    }

    public Entity[] getEntities() {
        return entities.toArray(new Entity[0]);

    }

    public void bounce() {
        // TODO Paddle collision detection

        if (ballX < p1X + paddle1.getSize().width && ballY > p1Y && ballY < p1Y + paddle2.getSize().height) {
            direction = true;
        }

        if (ballX + ball.getSize().width > p2X && ballY > p2Y && ballY < p2Y + paddle1.getSize().height) {
            direction = false;
        }

        if (ballX < view.getBounds().x) {
            paddle2.increaseScore();

            direction = !direction;

            ballX = 300;
            ballY = 300;
            // direction = true;
        }
        if (ball.getLocation().x > view.getBounds().x + view.getBounds().width) {
            paddle1.increaseScore();

            direction = !direction;

            ballX = 300;
            ballY = 300;
            // direction = false;
        }
        if (ball.getLocation().y < view.getBounds().y) {
            ballY++;
            incline *= -1;

        }
        if (ball.getLocation().y > view.getBounds().height) {
            ballY--;
            incline *= -1;
        }

        /////////////
        if (paddle1.getLocation().y < view.getBounds().y) {
            p1Y = view.getBounds().x - 1;
        }
        if (paddle1.getLocation().y + paddle1.getSize().height > view.getBounds().height + 22) {
            p1Y = view.getBounds().height - paddle1.getSize().height + 22;
        }

        if (paddle2.getLocation().y < view.getBounds().y) {
            p2Y = view.getBounds().x - 1;
        }
        if (paddle2.getLocation().y + paddle2.getSize().height > view.getBounds().height + 22) {
            p2Y = view.getBounds().height - paddle2.getSize().height + 22;
        }

    }

    public void setDirection(boolean Xdir, double inc) {
        ballY += inc;
        if (Xdir) {
            ballX++;
        } else if (!Xdir) {
            ballX--;
        }
    }

    public int getPaddleScore(int paddleNum) {
        if (paddleNum == 1)
            return paddle1.getScore();
        else {
            return paddle2.getScore();
        }
    }
}

LeftAction & RightAction

import java.awt.event.ActionEvent;

import javax.swing.AbstractAction;

public class LeftAction extends AbstractAction {
    Direction dir;
    boolean pressed;

    public LeftAction(Direction dir, boolean pressed) {
        this.dir = dir;
        this.pressed = pressed;
    }

    @Override
    public void actionPerformed(ActionEvent arg0) {
        if (pressed) {
            Model.keys.add(dir);
        } else {
            Model.keys.remove(dir);
        }
    }

}

import java.awt.event.ActionEvent;

import javax.swing.AbstractAction;

public class RightAction extends AbstractAction {
    Direction dir;
    boolean pressed;

    public RightAction(Direction dir, boolean pressed) {
        this.dir = dir;
        this.pressed = pressed;
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        if (pressed) {
            Model.keys.add(dir);
        } else {
            Model.keys.remove(dir);
        }
    }

}

Direction

public enum Direction {
    LEFT_UP, LEFT_DOWN, RIGHT_UP, RIGHT_DOWN
}

Let me know what you think!

Answer 1

Note that I am not familiar with Swing, only Java.

You should remove unused variables. Like x and y in Ball. They just confuse the reader about the logic.

This can be improved.

Paddle paddle1;

Paddle paddle2;

Ball ball;

double ballX = 0, ballY = 0;

double p1X = 0, p1Y = 0;

double p2X = 0, p2Y = 0;

Paddle already has Location. You can make methods to update location like

public void updateLocation(int changeX, int changeY) {
    location.setLocation(location.x + changeX, location.y + changeY);
}

Similarly move ballX and ballY inside Ball. Remember encapsulation. Ball's location remains inside Ball.

You have this in the update method of Model

    if (paddle1 == null || paddle2 == null || ball == null) {
      //Initializations here  
    }

Why? This is something that should be present in the constructor. After you move the x and y for Paddle and Ball inside the classes then this big initialization will also move to their respective constructors.

Except their names there is no actual difference between LeftAction and RightAction. I went ahead and deleted one and things work perfectly fine.

This is copy pasted, right?

    if (paddle1.getLocation().y < view.getBounds().y) {
        p1Y = view.getBounds().x - 1;
    }
    if (paddle1.getLocation().y + paddle1.getSize().height > view.getBounds().height + 22) {
        p1Y = view.getBounds().height - paddle1.getSize().height + 22;
    }

    if (paddle2.getLocation().y < view.getBounds().y) {
        p2Y = view.getBounds().x - 1;
    }
    if (paddle2.getLocation().y + paddle2.getSize().height > view.getBounds().height + 22) {
        p2Y = view.getBounds().height - paddle2.getSize().height + 22;
    }

You can use methods instead for the 2 ifs that have been copy pasted. If you use the previous approach of updated location instead of maintaining them separately as p1x and p1y then there will no problem in assigning the results also in the method itself.

Look at the bounce method and break it down into methods giving meaningful names. Wherever you have copy pasted you can use smaller methods.

Every time you want size you are creating a Dimension object. Why? It is not changing so you can keep returning the same one.

public void setDirection(boolean Xdir, double inc) {

    ballY += inc;
    if (Xdir) {
        ballX++;
    } else if (!Xdir) {
        ballX--;
    }
}

See the else part? No need for the condition.

In Paddle I would take out the particular bindings out. The bindings should be passed from outside should not be in the paddle class.

Answer 2

First look at Aseem's answer for these small changes:

• don't initialise your paddles and ball in the update method. Do so in the constructor
• combine LeftAction and RightAction into one MoveAction.

Next I want to say that I fully agree with the rest of Aseem's post, but it seems the way he explained it isn't clear to you. I'll try to elaborate on that so that hopefully it becomes clear.

A good principle in OO design is to put the information in the information expert. In this case, the thing that should know about the position of the ball is the ball itself. The same holds for your 2 peddles. This is actually a design choice you already made yourself. By putting these variables inside the Ball class:

private Point location = new Point(0, 0);
private int x = 0, y = 0;

Note that I added the private keyword. No other class should know how the Ball stores this information internally. (Encapsulation is a good thing). If we ever want to change the xand yto doubles to have more precision we can do this safely inside the Ball class. If these are not private we have to look for all the other places in the codebase where they might be used.

Now you also store ballX and ballY as fields inside the Model class. This isn't useful since you're just duplicating information about the ball. You already have the ball object on which you can ask it's coordinates.

Your reasoning to save them locally during manipulation and checks isn't wrong though. But those should be locally inside a method. And if possible this method should be reusable as well.

Let me give you an example:

public static final int BALL_SPEED = 22;
public static final int PADDLE_SPEED = 22;

...

private boolean checkCollision(Ball ball, Paddle paddle){
    int currentBallx = ball.getX();
    //direction multiplier, use -1 to move the ball to the left.
    int direction = 1;
    if(ball.getDirection()==Direction.LEFT_UP
           || ball.getDirection()==Direction.LEFT_DOWN){
        direction = -1;
    }
    private int nextBallX = ball.getX() + direction*BALL_SPEED;
    //same for currentBallY and nextBallY

    int currentPaddleX = paddle.getX();
    int currentPaddleY = paddle.getY();

    //paddles only move up and down
    int nextPaddleX = currentPaddleX;
    int paddleDirection = 1;
    if(/*check paddle moves down*/){
        paddleDirection = -1;
    }
    int nextPaddleY = currentPaddleY + paddleDirection*PADDLE_SPEED;

    //TODO check for collision based on current and next positions of the ball and paddle
}

A few things to note about this example:

• I introduced 2 new constants BALL_SPEED and PADDLE_SPEED. This makes it a lot easier to read than your magic numbers.
• I assumed that the ball and paddle also store the direction in which they are going.
• this function does not modify any state inside the ball or paddle. It just calculates the current and next potitions locally and than uses that information to check for collision.

You can then use this function to check collision for both the paddles. Another advantage is if you want to change your game to have 10 balls for example you can just loop over the balls, and call this method, followed by the updates in direction they will be going.

for(Ball ball : balls){
    if(checkCollision(ball, leftPaddle){
        ball.setDirection(/*find out what direction and put it here*/);
    }
    if(checkCollision(ball, rightPaddle){
        ball.setDirection(...);
    }
}

---

I want to add here that everything above was based on the decision to put the position and direction inside the ball itself. There is also another option. Have the ball not know (nor care) about it's position and movement at all (except for the location on screen so it can draw itself). In that case the class becomes a really simple:

public class Ball {
    private Point location = new Point(0, 0);

    public void updateLocation(Point newLocation){
        this.location = newLocation;
    }

     @Override
    public void paint(Graphics g) {
        g.setColor(Color.RED);
        g.fillOval(location.x, location.y, getSize().width, getSize().height);
    }
}

Now you need someone else to keep track of the locations of all the objects. In your case this is the Model. Every time the ball moves the model has to ball.updateLocation(...) so it will draw in the correct place.

These are all choices you will have to make based on the application you're writing. There's no real right way to do things, but there are some guidelines on how to write more readable and maintainable code.