Basic Pong Game

Question

I am very new to programming still but have been learning a lot. I decided to have a go at making the game Pong from scratch, and I want to know how I could have made this more simple than I wrote it. I feel like I have wayy too many unnecessary variables and some of my code needs cleaned. I am still bad at privatizing methods when I should so most of my methods may be public or static when they shouldn't be. Also the ball in this game takes very very predictable paths so I'm not sure how I could make the ball a bit trickier to get.

My main class (I named it shapes because this started as a simple project where I made a square move).

package square;

import javax.swing.*;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;

public class Shapes extends JFrame {

public static int WIN_WIDTH = 700;
public static int WIN_HEIGHT = 400;

public Shapes() {
    Panel panel = new Panel();
    Paddle paddle = panel.paddle;

    setSize(WIN_WIDTH, WIN_HEIGHT);
    setResizable(false);
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    add(panel);

    panel.addKeyListener(new KeyListener() {
        @Override
        public void keyTyped(KeyEvent e) {

        }

        @Override
        public void keyPressed(KeyEvent e) {
            int keyCode = e.getKeyCode();
            if(keyCode == KeyEvent.VK_DOWN) {
                paddle.p_yVelocity = 4;
            }
            if(keyCode == KeyEvent.VK_UP) {
                paddle.p_yVelocity = -4;
            }
            if(keyCode == KeyEvent.VK_S) {
                paddle.o_yVelocity = 4;
            }
            if(keyCode == KeyEvent.VK_W) {
                paddle.o_yVelocity = -4;
            }
            if(keyCode == KeyEvent.VK_ENTER) {
                paddle.start();
            }
        }

        @Override
        public void keyReleased(KeyEvent e) {
            int keyCode = e.getKeyCode();
            if(keyCode == KeyEvent.VK_DOWN) {
                paddle.p_yVelocity = 0;
                System.out.println(paddle.p_yVelocity);
            }
            if(keyCode == KeyEvent.VK_UP) {
                paddle.p_yVelocity = 0;
            }
            if(keyCode == KeyEvent.VK_W) {
                paddle.o_yVelocity = 0;
            }
            if(keyCode == KeyEvent.VK_S) {
                paddle.o_yVelocity = 0;
            }
        }
    });
    setVisible(true);
}

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

My Panel class for updating and drawing objects into the frame.

package square;

import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

public class Panel extends JPanel implements ActionListener {

Paddle paddle = new Paddle();
Ball ball = new Ball();
Timer timer;

public Panel() {
    this.setFocusable(true);
    timer = new Timer(10,this);

    timer.start();
}

@Override
protected void paintComponent(Graphics g) {
    super.paintComponent(g);
    g.setColor(Color.black);
    g.fillRect(0,0, Shapes.WIN_WIDTH, Shapes.WIN_HEIGHT);
    paddle.paint(g);
}

public void update() {
    if(paddle.started) {
        ball.update();
        paddle.update();
    }
}

@Override
public void actionPerformed(ActionEvent e) {
    repaint();
    update();
}
}

And these are my Paddle and Ball class. I think I could have written some things from the Paddle class in the Ball class but I was working to make the game function so whatever worked, I stuck with.

package square;

import java.awt.*;

public class Paddle {

public Ball ball;

public static int RECT_WIDTH = 10;
public static int RECT_HEIGHT = 70;
public static int p_x = 5;
public static int o_x = 680;
public static int p_y = Shapes.WIN_HEIGHT/2-50;
public static int o_y = Shapes.WIN_HEIGHT/2-50;
public static int xVelocity;
public static int p_yVelocity, o_yVelocity;
public static boolean started = false;

public void paint(Graphics g) {
    g.setColor(Color.white);
    g.fillRect(p_x, p_y, RECT_WIDTH, RECT_HEIGHT);
    g.fillRect(o_x, o_y, RECT_WIDTH, RECT_HEIGHT);
    g.fillRect(ball.BALL_X, ball.BALL_Y, ball.BALL_WIDTH, ball.BALL_HEIGHT);

    g.setColor(Color.white);
    g.setFont(new Font("Arial", 1, 25));

    if (!started)
    {
        g.drawString("Press 'Enter' to Start", 225, 100);
    }
        g.drawString(Integer.toString(Ball.p_score), 315, 50);
        g.drawString("-", 335, 50);
        g.drawString(Integer.toString(Ball.o_score), 350, 50);
}

public void update() {
    p_x += xVelocity;
    p_y += p_yVelocity;
    o_y += o_yVelocity;
    if(p_y < 0) {
        p_y = 0;
    }
    if(p_y > 295) {
        p_y = 295;
    }
    if(o_y < 0) {
        o_y = 0;
    }
    if(o_y > 295) {
        o_y = 295;
    }
}

public static boolean start() {
    return started = true;
}

public static boolean end() {
    p_x = 5;
    o_x = 680;
    p_y = Shapes.WIN_HEIGHT/2-50;
    o_y = Shapes.WIN_HEIGHT/2-50;
    Ball.newVelocity();
    return started = false;
}
}

---

package square;

import java.util.Random;

public class Ball {

static Random ran = new Random();

public static int BALL_HEIGHT = 10;
public static int BALL_WIDTH = BALL_HEIGHT;
public static int BALL_X = Shapes.WIN_WIDTH/2 - BALL_WIDTH;
public static int BALL_Y = Shapes.WIN_HEIGHT/2 - BALL_HEIGHT;
public static int o_score = 0, p_score = 0;
public static int bxVelocity = ran.nextInt(8 - 1) - 4, byVelocity = 
ran.nextInt(8 - 1) - 4;

public void update() {

    BALL_X += bxVelocity;
    BALL_Y += byVelocity;

    while(bxVelocity == 0 || byVelocity == 0) {
        bxVelocity = ran.nextInt(8 - 1) - 4;
        byVelocity = ran.nextInt(8 - 1) - 4;
    }
    if((BALL_Y >= Paddle.p_y && BALL_Y <= Paddle.p_y + Paddle.RECT_HEIGHT) 
&& (BALL_X <= Paddle.p_x + Paddle.RECT_WIDTH)) {
        bxVelocity = -bxVelocity;
    }
    if((BALL_Y >= Paddle.o_y && BALL_Y <= Paddle.o_y + Paddle.RECT_HEIGHT) 
&& (BALL_X >= Paddle.o_x - Paddle.RECT_WIDTH)) {
        bxVelocity = -bxVelocity;
    }
    if(BALL_Y >= 355) {
        byVelocity = -byVelocity;
    }
    if(BALL_Y <= 0) {
        byVelocity = -byVelocity;
    }
    if(BALL_X <= 0) {
        BALL_X = Shapes.WIN_WIDTH/2 - BALL_WIDTH;
        BALL_Y = Shapes.WIN_HEIGHT/2 - BALL_HEIGHT;
        o_score++;
        Paddle.end();
    }
    if(BALL_X >= 700) {
        BALL_X = Shapes.WIN_WIDTH/2 - BALL_WIDTH;
        BALL_Y = Shapes.WIN_HEIGHT/2 - BALL_HEIGHT;
        p_score++;
        Paddle.end();
    }
}


public static void newVelocity() {
    bxVelocity = ran.nextInt(8 - 1) - 4;
    byVelocity = ran.nextInt(8 - 1) - 4;

    while(bxVelocity == 0 || byVelocity == 0) {
        bxVelocity = ran.nextInt(8 - 1) - 4;
        byVelocity = ran.nextInt(8 - 1) - 4;
    }
    System.out.println(bxVelocity);
}
}

I would like to change the Ball velocity to a double value so I can increment the speed by like 0.1 everytime it hits a paddle but I couldn't figure out how to make a random double value to get the initial ball velocities.

I'm not sure if this is an appropriate post for this site as it is super lengthy and asks just for review of my code, I can take it down if this isn't the place to post!

Thanks in advance

Answer 1

Your question about random doubles

I would like to change the Ball velocity to a double [...] but I couldn't figure out how to make a random double value to get the initial ball velocities.

After changing the data type, you can generate a random double value in a range by scaling the range with a double between 0.0 and 1.0. ran.nextDouble() gives you a random double value between 0.0 and 1.0:

double bxVelocity = ran.nextDouble() * 6 - 4;

Note that your ran.nextInt(8 - 1) - 4 is equivalent to ran.nextInt(7) - 4, and since nextInt is exclusive on the upper bound, this gives you numbers between -4 and +2, which I'm not sure is what you intended.

---

Class names

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

Having your main class named Shapes is unusual and confusing, since it is a Game, more specifically Pong. Either of these two names would fit much better. You explained that the name stems from a former project that evolved into this one, but most IDEs are capable of easily renaming classes, as well as their containing files, automatically without the need to search and change every occurence manually.

---

The problem with public static

Almost all of your methods and variables are public static. You address this problem in your question:

I am still bad at privatizing methods when I should so most of my methods may be public or static when they shouldn't be.

You are right in that regard, however for many beginners of object oriented programming (OOP) it is difficult to understand why this is a problem, and how to fix it, even when they know they should.

I will not go into detail about object oriented principles, although I recommend to search and read about them. Instead, I want to give you the short version: Splitting up code into classes, and hiding their details to the outside, enables you (and possibly other programmers in bigger projects) to focus on small amounts of code and to literally forget about the rest.

Your program is already fairly complex, and changing values in one class affects others as well, which should be avoided. Especially hiding information by making it private helps prevent errors. But if class A has the information privatly, and B needs it, how does B get it? The answer is: A should give it to B if needed. B should not be allowed to access it himself.

The first step to avoiding public static (or even both of them independently) is to just not use them until you need them. This applies to methods, because variables of classes should never be public and almost never be static.

---

Thinking the object oriented way

Many beginner programmers think about their program in terms of code and variables, at a very low level: "How do I get the information I need? I will just make it public and access it directly. That's the easiest way." The problem with this is that everything is connected, once you introduce errors (and you probably will) you will have a hard time trying to fix them, because even the fix of the error might introduce new ones.

Thinking in an object oriented way means thinking about your program in terms of objects or components in a system. You should be able to replace a component with an equivalent one, without having to change its surroundings. At this stage, you should not be thinking about any code, or variables, or data types. Instead you should think about attributes and behaviour.

What does the ball do? It moves around. Does the ball have a score? Probably not. The player has a score, or in your case the paddle might manage their scores, since they represent the players. In your case the ball has two scores, probably one for each player. That is nothing the ball should be concerned about.

Who tells the player to press Enter to start the game (i. e. that the game is ready to be started)? The game should do that, but in your case the paddle does it.

I think the classes that you have are fine, just not how they are used. Before writing the first line of code, think about what your program contains (i. e. which types of objects or components). In this case, you would have a Game. The Game has two players represented by Paddles, and a Ball. So far this is close to what you have. Now you think about the attributes. The Ball has a width and height. These are nobody's business, so they are neither public nor static. Make them private and instead of the Paddle drawing the Ball, have the Ball draw himself. Because he already knows everything he needs to know.

---

Rethinking the architecture

Every class should contain its own information. If it needs any further information, it should always be given to them, instead of it taking it from elsewhere.

The input, which you handled in the Shapes class could be moved to its own Input class. This one could have some static methods to ask whether a key is pressed or released. Then you do not need the Shapes class to change the paddles' velocities on key press.

Following I will try to rebuild the architecture so that it is more modular. There are surely multiple ways to do this right. I will omit parts of the code that I think you can do by yourself and might be a good practise. Also I will add some comments to explain my version. The code will not run like this and should only give you an idea how you could structure your code.

The game class:

public class Pong extends JFrame {
    // Only the game knows this. If someone else needs it, we give it to them.
    // You never change this in your code, and you setResizable(false), so we set it to final.
    private final int WIDTH = 700;
    private final int HEIGHT = 400;

    private boolean running = false;

    private Ball ball;
    private Paddle paddleLeft;
    private Paddle paddleRight;

    public Pong() {
        setSize(WIDTH, HEIGHT);
        setResizable(false);

        // Initialize Ball and Paddles here.
    }

    public void start() {
        running = true;

        while (running) {
            // This is your game loop.
            // To break out of the game loop, set running to false, e. g. when a certain key is pressed.
            // Add any cleanup code after the loop.

            // Here you should call the update methods of the other components.
            ball.update();
            paddleLeft.update();
            paddleRight.update();

            // Then you call draw() on all of them.
            // So add a draw() method to them and let them draw themself.
            ball.draw();
            paddleLeft.draw();
            paddleRight.draw();
        }
    }

    public static void main() {
        new Pong().start();
    }

The ball class:

public class Ball {
    // If the Ball draws himself, no one else needs to know these.
    private int radius; // You used a rect for the ball. I think it should be a circle.
    private int posX;
    private int posY;
    private double speedX;
    private double speedY;

    public void update() {
        // Handle movement, changes of speed and collision detection here.
    }

    // You might need to pass the Graphics instance here, as you did in the Paddle class.
    public void draw(Graphics graphics) {
        // Add rendering code here.
    }
}

The paddle class could look similar to the ball class, i. e. it has its position, size and speed, and methods for update() and draw().

---

Coding style

Every language has its conventions, which enable programmers to understand each other's code more easily. Usually the conventions include naming schemes for variables, classes, methods etc. and formatting rules. These are independent from the architecture, and also from the syntax. Names or formats that are allowed by the language might not be "allowed" by the convention.

---

public static int WIN_WIDTH = 700;
public static int WIN_HEIGHT = 400;

Variables should never be public (and rarely static), but lets assume these are just normal (private) variables (static or not). They should be written in camelCase. ALL_UPPER_CASE names are used for constants, which are declared using final. That prevents them from being changed, so when seeing a name like WIN_WIDTH you should be able to trust in it not being changed.

---

public class Shapes extends JFrame {

// ...

public Shapes() {

This formatting is confusing. The constructor is on the same indentation level as the class declaration. The same thing in the Paddle class actually made me double-check a few times which class I'm reading.

To be more readable, it should be indented like this:

public class Shapes extends JFrame {

// ...

    public Shapes() {

As a rule of thumb, whenever you have { at the end of the line, you indent everything that's following until the matching }.

That way, you also avoid things like closing braces on the same indentation level on consecutive lines, which can be confusing as well:

@Override
public void actionPerformed(ActionEvent e) {
    repaint();
    update();
}
}

---

if(paddle.started)

This should ring alarm bells, because it leaks the internal state of an object, which means that you could also change it from the outside. Instead you should always access fields (class variables) via accessor methods (and only have these if you really need to). That way you can add a getter method, that allows to read the value, without anyone being able to change it.

Inside of the class, instead of this:

public static boolean started = false;

do this:

private boolean started = false;

public boolean isStarted() {
    return started;
}

Then use it like this:

if (paddle.isStarted())