5
\$\begingroup\$

I have made my version of the game pong in java. This is my first Java coding project. I was curious if my code adheres to good Java coding practices. Constructive criticism is appreciated.

The main class which creates the window for the game.

import javax.swing.*;  

class PongFrame {
    static final int  HEIGHT=600;
    static final int WIDTH=800;
    private PongPanel panel;

    private PongFrame(){
        JFrame frame  = new JFrame("Pong");
        frame.setSize(WIDTH,HEIGHT);
        panel = new PongPanel();

        frame.add(panel);
        frame.setResizable(false);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setVisible(true);

    }

    public static void main(String args[]){
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                new PongFrame();
            }
        });
    }

}

The PongPanel class which is the content pane for the JFrame.

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

/**
 * This is the content pane of my JFrame.
 */
class PongPanel extends JPanel implements ActionListener,KeyListener{
    private Ball b;
    private Paddle left_pad;
    private Paddle right_pad;
    private Timer timer;
    private int DELAY = 10;
    private boolean keys[]; //Array the holds the activated keys.
    private int LEFT_UP = 0;
    private int LEFT_DOWN = 1;
    private int RIGHT_UP = 2;
    private int RIGHT_DOWN = 3;


    PongPanel(){
        setBackground(Color.BLACK);
        b = new Ball();
        left_pad = new Paddle("left");
        right_pad = new Paddle("right");
        addKeyListener(this);
        setFocusable(true);
        timer = new Timer(DELAY,this);
        timer.start();
        setDoubleBuffered(true);
        keys = new boolean[]{false,false,false,false};
    }
/*
Moving the code for moving paddles into actionPerformed make
the paddle movement super smooth.
 */
    @Override
    public void actionPerformed(ActionEvent e) {
        b.move();
        checkCollision();
        if(keys[LEFT_UP]) left_pad.moveUp();
        if(keys[LEFT_DOWN]) left_pad.moveDown();
        if(keys[RIGHT_UP]) right_pad.moveUp();
        if(keys[RIGHT_DOWN]) right_pad.moveDown();
        repaint();
    }

    private void checkCollision() {
        if(b.getX_pos() < (left_pad.getX_pos() + left_pad.getWidth())) {
            if((b.getY_pos() > left_pad.getY_pos()) && (b.getY_pos() < (left_pad.getY_pos() + left_pad.getHeight())))
                b.revVelocityX();
            else {
                b.setX_pos(PongFrame.WIDTH / 2);
                b.setY_pos(PongFrame.HEIGHT / 2);
            }
        }
        if(b.getX_pos() > (right_pad.getX_pos() - right_pad.getWidth())){
            if((b.getY_pos() > right_pad.getY_pos()) && (b.getY_pos() < (right_pad.getY_pos() + right_pad.getHeight())))
                b.revVelocityX();
            else {
                b.setX_pos(PongFrame.WIDTH / 2);
                b.setY_pos(PongFrame.HEIGHT / 2);
            }

        }
        else if(b.getY_pos()<0 || b.getY_pos()>PongFrame.HEIGHT){
            b.revVelocityY();
        }

    }

    @Override
    protected void paintComponent(Graphics g) {
        super.paintComponent(g);
        drawBall(g);
        drawPaddles(g);

    }

    private void drawPaddles(Graphics g) {
        g.setColor(Color.WHITE);
        g.fillRect(left_pad.getX_pos(),left_pad.getY_pos(),left_pad.getWidth(),left_pad.getHeight());
        g.fillRect(right_pad.getX_pos(),right_pad.getY_pos(),right_pad.getWidth(),right_pad.getHeight());
        Toolkit.getDefaultToolkit().sync();
    }

    private void drawBall(Graphics g) {
        g.setColor(Color.WHITE);
        g.fillOval(b.getX_pos(),b.getY_pos(),b.getDiameter(),b.getDiameter());
        /*
        The following line is super important.
        It smoothens the animation.
        Without it animation is chopyy in linux!
         */
        Toolkit.getDefaultToolkit().sync();
    }

    @Override
    public void keyTyped(KeyEvent keyEvent) {

    }

    @Override
    public void keyPressed(KeyEvent e) {
        /*
        Controls for right paddle are the up and down arrow keys.
        Controls for left paddle are the w and s keys.

        Used the keys array to implement simultaneous movement of paddles.
        Movement code is shifted to the actionPerformed method.
        For eg. if keys[RIGHT_UP] = true,the action performed method will move
        the right paddle up.
         */
        if(e.getKeyCode() == KeyEvent.VK_UP) {
            keys[RIGHT_UP] = true;

        }
        if(e.getKeyCode() == KeyEvent.VK_DOWN) {
            keys[RIGHT_DOWN] = true;
        }
        if(e.getKeyCode() == KeyEvent.VK_W){
            keys[LEFT_UP] = true;
        }
        if(e.getKeyCode() == KeyEvent.VK_S){
            keys[LEFT_DOWN] = true;
        }

    }

    @Override
    public void keyReleased(KeyEvent e) {
        if(e.getKeyCode() == KeyEvent.VK_UP) {
            keys[RIGHT_UP] = false;

        }
        if(e.getKeyCode() == KeyEvent.VK_DOWN) {
            keys[RIGHT_DOWN] = false;
        }
        if(e.getKeyCode() == KeyEvent.VK_W){
            keys[LEFT_UP] = false;
        }
        if(e.getKeyCode() == KeyEvent.VK_S){
            keys[LEFT_DOWN] = false;
        }
    }
}

The Paddle Class

class Paddle {
    private int height;
    private int width;
    private int x_pos;
    private int y_pos;
    private int vel;
//    private int accel; //TODO : Implement acceleration.


    int getHeight() {
        return height;
    }

    int getWidth() {
        return width;
    }

    int getX_pos() {
        return x_pos;
    }

    int getY_pos() {
        return y_pos;
    }
/*
The string parameter will tell if it is the left paddle
 or the right one.
 */
Paddle(String s) {
        vel = 10;
//        accel = 2;
        height = 150;
        width = 10;
        y_pos = (PongFrame.HEIGHT / 2) - (height / 2); //Centers the paddle
        if(s.equals("left")){
            x_pos = 10;
        }
        if(s.equals("right")){
            x_pos = PongFrame.WIDTH - 20;
        }
    }

    void moveUp(){
        if(y_pos > 0)
            y_pos-=vel;
    }

    void moveDown(){
        if(y_pos < (PongFrame.HEIGHT - height))
            y_pos +=vel;
    }
}

The Ball Class

import com.sun.xml.internal.bind.v2.TODO;

class Ball {
    private int x_pos;
    private int y_pos;

    private int diameter;

    private int x_vel;

    private int y_vel;
    Ball() {
        x_vel = 4;
        y_vel = 4;
        diameter = 15;
        x_pos = PongFrame.WIDTH/2;
        y_pos = PongFrame.HEIGHT/2;

    }

    void move(){
        x_pos += x_vel;
        y_pos += y_vel;
    }

    int getX_pos() {
        return x_pos;
    }

    int getY_pos() {
        return y_pos;
    }

    int getDiameter() {
        return diameter;
    }

    void revVelocityX(){
        x_vel = -x_vel;
    }

    void revVelocityY(){
        y_vel = -y_vel;
    }

    void setX_pos(int x_pos) {
        this.x_pos = x_pos;
    }

    void setY_pos(int y_pos) {
        this.y_pos = y_pos;
    }


}
\$\endgroup\$
  • \$\begingroup\$ Not exactly "practice", but you should be in the habit of reading the documentation for APIs. Swing is not thread safe (it says so in the documentation), so you should only be creating and modify Swing components on the EDT (Event Dispatch Thread). \$\endgroup\$ – markspace Jan 30 '18 at 3:08
  • \$\begingroup\$ have you considered using KeyBinding over KeyListener? see stackoverflow.com/questions/23486827/… \$\endgroup\$ – Martin Frank Feb 14 '18 at 13:10
4
\$\begingroup\$

Use consistent variable names

You use both camelCase and snake_case: drawPaddles and x_vel. Sometimes you even mix them together as in getX_pos. You use camelCase more, so I suggest you stick to that.
This does not mean you have to change the final variable names, there, allcaps is the convention.

Why?
Because it just makes it easier to forget exactly how you spelled a variable name, and then you have to look it up, which is just a waste of time.

No useless getters and setters

In Ball and Paddle you have private variables (x_pos, y_pos, etc.) that don't behave like privates at all, in the sense that they aren't encapsulated in any way. If you're going to let anything read/write to those variables with no 'checking' in the getters and setters, what do they do? Bloat.

Why?
Would you prefer reading/writing thing.xPos += thing.xVel; or thing.setXPos(thing.getXPos() + thing.getXVel);?

Weird whitespace

    if(b.getX_pos() > (right_pad.getX_pos() - right_pad.getWidth())){
        if((b.getY_pos() > right_pad.getY_pos()) && (b.getY_pos() < (right_pad.getY_pos() + right_pad.getHeight())))
            b.revVelocityX();
        else {
            b.setX_pos(PongFrame.WIDTH / 2);
            b.setY_pos(PongFrame.HEIGHT / 2);
        }
         <-- What is this doing here?
    }

and

Paddle(String s) { <-- Why is it not indented?
        vel = 10;
//        accel = 2;
        height = 150;
        width = 10;
        y_pos = (PongFrame.HEIGHT / 2) - (height / 2); //Centers the paddle
        if(s.equals("left")){
            x_pos = 10;
        }
        if(s.equals("right")){
            x_pos = PongFrame.WIDTH - 20;
        }
    }

are two examples of places where you have strange whitespace. For a while, the second one confused me: Was it a weird class definition? Why does it have unmatched curly braces. Use whatever indentation and blank lines you want, just keep it consistent.

And static final int HEIGHT=600; is just ugly in many ways.

Why?
It simply takes longer to read badly formatted code.

Magic strings (same as magic numbers)

What would

        if(s.equals("left")){
            x_pos = 10;
        }
        if(s.equals("right")){
            x_pos = PongFrame.WIDTH - 20;
        }

do if somewhere in your program you typed whatever = new Paddle("lrft");? Weird, unexplainable, and g̰͓̝̝̱ͫͦ́l̟̫̤͈̓ͧ͛ͦ̋i̥̱͆͛t͙͐c̈́̓̀ͨ̅h͕̻̪̫͛ͣ̄ͥ̚ŷ̻̖͍̪͔̏ behavior. Easy! Write

        if(!s.equals("left") && !s.equals("right")){
            throw new java.lang.Error("Woop woop you made a widdle typo!");
        }

Oh wait what if you mistyped that. Use more finals for "left" and "right"!

    static final String LEFT = "left";
    static final String RIGHT = "right";

That gives us:

        if(s.equals(Paddle.LRFT)){ <-- The error is found at compile time!
            x_pos = 10;                Hooray!
        }
        if(s.equals(Paddle.RIGHT)){
            x_pos = PongFrame.WIDTH - 20;
        }

And whatever = new Paddle(Paddle.LRFT); would also be caught!

Why?
Compile time errors instead of run time errors are so much better believe me. Why do you think we had static typing in the first place?

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for this. I know it's been months but I haven't checked Code Review for a long time. Very grateful for this answer and I will refactor the code as I am looking to add some new features into the program. (Play online multiplayer?) \$\endgroup\$ – Rohit Sarkar Aug 30 '18 at 5:41
1
\$\begingroup\$
  1. Methods and classes should have a documentation comment for readability.
  2. Names should be descriptive. For instance, revVelocityY(), the reader will not know what this method does unless they see the codes inside it which is deducting the value of x_vel. I immediately understand it since it only has one statement, but what if the method contains many lines?
\$\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.