Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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!

share|improve this question
    
I copy pasted the whole but could not find Direction. What is Direction? – Aseem Bansal Feb 7 at 8:48
    
@AseemBansal Sorry! Very stupid mistake. Direction is simply an Enum with 4 constants that tell the program in which direction to move the paddles. Hope this helps! – Eames Feb 7 at 11:46

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.

share|improve this answer
    
Thanks for the feedback! Just one thing though, I don't necessarily disagree with your way, but I don't really understand what's wrong with working with a 'ballX' and 'ballY' inside of the 'Model' class instead of taking a more direct approach to it by trying to change the x and y of the 'Ball'. After all, it is the model that implements the game rules and collision detection. Or is there something I'm missing in that aspect? – Eames Feb 7 at 20:02
    
@Eames The problem is simply that having ballX, ballY violates Encapsulation. At any point all the information related to Ball should be present in Ball. But in your case the information present in Ball is stale while you are working with ballX and ballY. Same with p1X etc. – Aseem Bansal Feb 8 at 5:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.