Suggestions to make code (Pong) better (from the code I have, not adding additional code)

[john price]

Pong.java:

import java.awt.*;
import java.awt.Color.*;
import java.awt.event.*;
import java.awt.geom.*;

import javax.swing.*;
import javax.swing.Timer;

import java.util.*;

import java.awt.image.BufferedImage;

public class Pong extends JPanel implements ActionListener, MouseListener, KeyListener, MouseMotionListener {

    //double instead of Double
    protected double px = 20.00;
    protected double py = 20.00;
    protected double pl = 5.00; 
    protected double ph = 30.00; 
    protected double cx = 370.00;
    protected double cy = 100.00;
    protected double cl = 5.00; 
    protected double ch = 30.00;
    protected double bx = 175.00;
    protected double by = 100.00;
    protected double bl = 5.00;
    protected double bh = 5.00;
    protected int pscore, cscore, startball, startball1, speed, rand, lost, won;
    protected Boolean started = false;
    protected Boolean something = false;
    protected String initdir, upslashdown, jfs;
    protected Timer timer;

    protected Pong() {
	this.setFocusable(true);
	this.setBackground(Color.BLACK);
	this.addMouseListener(this); 
	this.addKeyListener(this);
	this.addMouseMotionListener(this);
	}
	public void mouseMoved(MouseEvent me) {
	double ytracker = me.getY();
	if (ytracker >= 142) {
	py = 142;
	}
	else {
	py = (int) ytracker;
	}
	repaint();
	}
	public void mouseDragged(MouseEvent me) {
	}
	public void mousePressed(MouseEvent me) { 
	}
    public void mouseReleased(MouseEvent e) {
    }
    public void mouseEntered(MouseEvent e) {
    }
    public void mouseExited(MouseEvent e) {
    }
    public void mouseClicked(MouseEvent ex) {
	if (started == false) {
			startball = (int) ((Math.random() * 2)+1);
			if (startball == 1) {initdir = "left";}
			else {initdir = "right";}
			startball1 = (int) ((Math.random() * 2)+1);
			if (startball1 == 1) {upslashdown = "up";}
			else {upslashdown = "down";}
			started = true;
		timer = new Timer(20, this);
		timer.start();
		speed = (int) ((Math.random() *5)+2);
          	}
	if (something == true) {
		something = false;
		startball = (int) ((Math.random() * 2)+1);
			if (startball == 1) {initdir = "left";}
			else {initdir = "right";}
			startball1 = (int) ((Math.random() * 2)+1);
			if (startball1 == 1) {upslashdown = "up";}
			else {upslashdown = "down";}
			started = true;
		repaint();
		if (cscore == 10) {
		lost ++;
		cscore = 0;
		pscore = 0;
		repaint();
		started = false;
		}
		else if (pscore == 10) {
		won ++;
		pscore = 0;
		cscore = 0;
		repaint();
		started = false;
		}
		else {
		timer = new Timer(20, this);
		timer.start();
		speed = (int) ((Math.random() *5)+2);
		}
	}
    }
	public void keyPressed(KeyEvent e) {
	char KeyCar = e.getKeyChar();
	if (KeyCar == 'q') {if (py <= 0) {} else {py -= 10;}repaint();}
	if (KeyCar == 'a') {if (py >= 142) {} else {py += 10;}repaint();}
	if (KeyCar == 'w') {if (py <= 0) {} else {py -= 10;}repaint();}
	if (KeyCar == 's') {if (py >= 142) {} else {py += 10;}repaint();}
	switch(e.getKeyCode()) {
	case KeyEvent.VK_UP : if (py <= 0) {} else {py -= 10;} repaint(); break;
	case KeyEvent.VK_DOWN : if (py >= 142) {} else {py += 10;} repaint(); break;
	}
	}
	public void keyTyped(KeyEvent e) {
	}
	public void keyReleased(KeyEvent e) {
	}
    protected void paintComponent (Graphics g) {
        super.paintComponent(g);//removes all previously drawn content and draw from scratch
        Graphics2D g2 = (Graphics2D) g;
	g.setColor(Color.white);
        Rectangle2D rec = new Rectangle2D.Double(cx, cy, cl, ch); //Computer's "paddle"
	g2.fill(rec);
	Rectangle2D rec1 = new Rectangle2D.Double(px, py, pl, ph); //Player's "paddle"
	g2.fill(rec1);
	g.drawString("Score", 175, 20);
	g.drawString("Player: " + pscore, 120, 40);
	g.drawString("Computer: " + cscore, 220, 40);
	g.drawString("Games won : " + won, 120, 60);
	g.drawString("Games lost: " + lost, 220, 60);
	Ellipse2D ball = new Ellipse2D.Double(bx, by, bl, bh); //Ball's position
	g2.fill(ball);
    }

    public void actionPerformed(ActionEvent ex) { //1
	if ((bx >= px) && (bx <= px + pl) && (by >= py) && (by <= py + ph)) { //top edge = px, bottom edge = px + ph
	initdir = "right";
	bx += (int) ((Math.random() * 10)+1);
	rand = (int) ((Math.random() *2)+1);
	if (rand == 1) {
	by -= (int) ((Math.random() *10)+1);
	}
	else {by += (int) ((Math.random() *10)+1);}
	}
	if ((bx + bh >= cx) && (bx <= cx + cl) && (by >= cy) && (by <= cy + ch)) { //top edge = px, bottom edge = px + ph
	initdir = "left";
	bx -= (int) ((Math.random() * 10)+1);
	rand = (int) ((Math.random() *2)+1);
	if (rand == 1) {
	by -= (int) ((Math.random() *10)+1);
	}
	else {by += (int) ((Math.random() *10)+1);}
	}
	if (initdir.equals("right")) { //1
	if (by > cy) { //2
	//cy += speed - 1.020;//1.05 was successful win by me, 1.00 computer won
	//cy += speed - ((Math.random() /2)+0.2);	
	//cy += speed * 0.9;
	cy += speed * 0.80 + Math.random();
	} //1
	if (by < cy) { //2
	//cy -= speed - 1.020;
	//cy -= speed - ((Math.random() /2)+0.4);
	cy -= speed * 0.80 + Math.random();
	} //1
	}
	else if (initdir.equals("left")) { //2
	cy = 75.00;
	} //1
	if (initdir.equals("left")) { //2
		if (bx <= 0) { //3
		cscore ++;
		timer.stop();
		something = true;
		bx = 175.00;
    		by = 100.00;
    		bl = 5.00;
   		bh = 5.00;
		px = 20.00;
    		py = 20.00;
    		pl = 5.00; 
    		ph = 30.00;
		} //2
			else {bx -= speed;}
			} //1
		else if (initdir.equals("right")) { //2
			if (bx >= 400 - bl) { //3
			pscore ++;
			timer.stop();
			something = true;
			bx = 175.00;
    			by = 100.00;
    			bl = 5.00;
   			bh = 5.00;
			px = 20.00;
    			py = 20.00;
    			pl = 5.00; 
    			ph = 30.00; 
			}
			else {bx += speed;}
			}
	if (upslashdown.equals("up")) {
		if (by <= 0) {
		by += (int) ((Math.random() * 10)+1);
		upslashdown = "down";
		} //2
		else {by -= speed;}
	}
	else if (upslashdown.equals("down")) {
		if (by >= 200 - bh * 6.0) {
		by -= (int) ((Math.random() * 10)+1);
		upslashdown = "up";
		}
		else {by += speed;}
	}
	repaint(); //executes no matter what
    }

    public static void main(String[] args) {
        JFrame frame = new JFrame();
        frame.setTitle("Pong");
        frame.setSize(400, 200);
	Toolkit tk = Toolkit.getDefaultToolkit();
    	Dimension d = tk.getScreenSize();
	frame.setLocation(d.width /3, d.height /4);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        Container contentPane = frame.getContentPane();
	BufferedImage cursorImg = new BufferedImage(16, 16, BufferedImage.TYPE_INT_ARGB);

// Create a new blank cursor.
Cursor blankCursor = Toolkit.getDefaultToolkit().createCustomCursor(
    cursorImg, new Point(0, 0), "blank cursor");

// Set the blank cursor to the JFrame.
frame.getContentPane().setCursor(blankCursor);
        contentPane.add(new Pong());
	frame.setResizable(false);
        frame.setVisible(true);
    }
}

Any suggestions to make this work better or faster? Is there anything I should change? I don't want to add any extra features, just make my existing code better. All comments and help are welcome. I coded this from scratch by myself. You have permission to use this, edit this, and claim it as your own if you would like.
Thanks,
cc11rocks

[john price]

updated Pong.java:

import java.awt.*;
import java.awt.Color.*;
import java.awt.event.*;
import java.awt.geom.*;

import javax.swing.*;
import javax.swing.Timer;

import java.util.*;

import java.awt.image.BufferedImage;

public class Pong extends JPanel implements ActionListener, MouseListener, KeyListener, MouseMotionListener {

    //double instead of Double
    protected double px = 20.00;
    protected double py = 20.00;
    protected double pl = 5.00; 
    protected double ph = 30.00; 
    protected double cx = 370.00;
    protected double cy = 100.00;
    protected double cl = 5.00; 
    protected double ch = 30.00;
    protected double bx = 175.00;
    protected double by = 100.00;
    protected double bl = 5.00;
    protected double bh = 5.00;
    protected int pscore, cscore, startball, startball1, speed, rand, lost, won;
    protected Boolean started = false;
    protected Boolean something = false;
    protected String initdir, upslashdown, jfs;
    protected Timer timer;

    protected Pong() {
	this.setFocusable(true);
	this.setBackground(Color.BLACK);
	this.addMouseListener(this); 
	this.addKeyListener(this);
	this.addMouseMotionListener(this);
	}
	public void mouseMoved(MouseEvent me) {
	double ytracker = me.getY();
	if (ytracker >= 142) {
	py = 142;
	}
	else {
	py = (int) ytracker;
	}
	repaint();
	}
	public void mouseDragged(MouseEvent me) {
	}
	public void mousePressed(MouseEvent me) { 
	}
    public void mouseReleased(MouseEvent e) {
    }
    public void mouseEntered(MouseEvent e) {
    }
    public void mouseExited(MouseEvent e) {
    }
    public void mouseClicked(MouseEvent ex) {
	if (started == false) {
			startball = (int) ((Math.random() * 2)+1);
			if (startball == 1) {initdir = "left";}
			else {initdir = "right";}
			startball1 = (int) ((Math.random() * 2)+1);
			if (startball1 == 1) {upslashdown = "up";}
			else {upslashdown = "down";}
			started = true;
		timer = new Timer(20, this);
		timer.start();
		speed = (int) ((Math.random() *5)+2);
          	}
	if (something == true) {
		something = false;
		startball = (int) ((Math.random() * 2)+1);
			if (startball == 1) {initdir = "left";}
			else {initdir = "right";}
			startball1 = (int) ((Math.random() * 2)+1);
			if (startball1 == 1) {upslashdown = "up";}
			else {upslashdown = "down";}
			started = true;
		repaint();
		if (cscore == 10) {
		lost ++;
		cscore = 0;
		pscore = 0;
		repaint();
		started = false;
		}
		else if (pscore == 10) {
		won ++;
		pscore = 0;
		cscore = 0;
		repaint();
		started = false;
		}
		else {
		timer = new Timer(20, this);
		timer.start();
		speed = (int) ((Math.random() *5)+2);
		}
	}
    }
	public void keyPressed(KeyEvent e) {
	char KeyCar = e.getKeyChar();
	if (KeyCar == 'q') {if (py <= 0) {} else {py -= 10;}repaint();}
	if (KeyCar == 'a') {if (py >= 142) {} else {py += 10;}repaint();}
	if (KeyCar == 'w') {if (py <= 0) {} else {py -= 10;}repaint();}
	if (KeyCar == 's') {if (py >= 142) {} else {py += 10;}repaint();}
	switch(e.getKeyCode()) {
	case KeyEvent.VK_UP : if (py <= 0) {} else {py -= 10;} repaint(); break;
	case KeyEvent.VK_DOWN : if (py >= 142) {} else {py += 10;} repaint(); break;
	}
	}
	public void keyTyped(KeyEvent e) {
	}
	public void keyReleased(KeyEvent e) {
	}
    protected void paintComponent (Graphics g) {
        super.paintComponent(g);//removes all previously drawn content and draw from scratch
        Graphics2D g2 = (Graphics2D) g;
	g.setColor(Color.white);
        Rectangle2D rec = new Rectangle2D.Double(cx, cy, cl, ch); //Computer's "paddle"
	g2.fill(rec);
	Rectangle2D rec1 = new Rectangle2D.Double(px, py, pl, ph); //Player's "paddle"
	g2.fill(rec1);
	g.drawString("Score", 175, 20);
	g.drawString("Player: " + pscore, 120, 40);
	g.drawString("Computer: " + cscore, 220, 40);
	g.drawString("Games won : " + won, 120, 60);
	g.drawString("Games lost: " + lost, 220, 60);
	Ellipse2D ball = new Ellipse2D.Double(bx, by, bl, bh); //Ball's position
	g2.fill(ball);
    }

    public void actionPerformed(ActionEvent ex) { //1
	if ((bx >= px) && (bx <= px + pl) && (by >= py) && (by <= py + ph)) { //top edge = px, bottom edge = px + ph ***2
	initdir = "right";
	bx += (int) ((Math.random() * 10)+1);
	rand = (int) ((Math.random() *2)+1);
	if (rand == 1) { //3
	by -= (int) ((Math.random() *10)+1);
	} //2
	else {by += (int) ((Math.random() *10)+1);}
	} //1
	if ((bx + bh >= cx) && (bx <= cx + cl) && (by >= cy) && (by <= cy + ch)) { //top edge = px, bottom edge = px + ph
	initdir = "left";
	bx -= (int) ((Math.random() * 10)+1);
	rand = (int) ((Math.random() *2)+1);
	if (rand == 1) {
	by -= (int) ((Math.random() *10)+1);
	}
	else {by += (int) ((Math.random() *10)+1);}
	}
	if (initdir.equals("right")) { //1
	if (by > cy) { //2
	//cy += speed - 1.020;//1.05 was successful win by me, 1.00 computer won
	//cy += speed - ((Math.random() /2)+0.2);	
	//cy += speed * 0.9;
		if (by >= 142) {
		if ((cy + speed * 0.80 + Math.random()) >= 142) {
		cy = 142;
		}
		else {
		cy += speed * 0.80 + Math.random();
		}
		}
	cy += speed * 0.80 + Math.random();
	} //1
	if (by < cy) { //2
	//cy -= speed - 1.020;
	//cy -= speed - ((Math.random() /2)+0.4);
	cy -= speed * 1.05 + Math.random();
	} //1
	}
	else if (initdir.equals("left")) { //2
	cy = 75.00;
	} //1
	if (initdir.equals("left")) { //2
		if (bx <= 0) { //3
		cscore ++;
		timer.stop();
		something = true;
		bx = 175.00;
    		by = 100.00;
    		bl = 5.00;
   		bh = 5.00;
		} //2
			else {bx -= speed;}
			} //1
		else if (initdir.equals("right")) { //2
			if (bx >= 400 - bl) { //3
			pscore ++;
			timer.stop();
			something = true;
			bx = 175.00;
    			by = 100.00;
    			bl = 5.00;
   			bh = 5.00; 
			}
			else {bx += speed;}
			}
	if (upslashdown.equals("up")) {
		if (by <= 0) {
		by += (int) ((Math.random() * 10)+1);
		upslashdown = "down";
		} //2
		else {by -= speed;}
	}
	else if (upslashdown.equals("down")) {
		if (by >= 200 - bh * 6.0) {
		by -= (int) ((Math.random() * 10)+1);
		upslashdown = "up";
		}
		else {by += speed;}
	}
	repaint(); //executes no matter what
    }

    public static void main(String[] args) {
        JFrame frame = new JFrame();
        frame.setTitle("Pong");
        frame.setSize(400, 200);
	Toolkit tk = Toolkit.getDefaultToolkit();
    	Dimension d = tk.getScreenSize();
	frame.setLocation(d.width /3, d.height /4);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        Container contentPane = frame.getContentPane();
	BufferedImage cursorImg = new BufferedImage(16, 16, BufferedImage.TYPE_INT_ARGB);

// Create a new blank cursor.
Cursor blankCursor = Toolkit.getDefaultToolkit().createCustomCursor(
    cursorImg, new Point(0, 0), "blank cursor");

// Set the blank cursor to the JFrame.
frame.getContentPane().setCursor(blankCursor);
        contentPane.add(new Pong());
	frame.setResizable(false);
        frame.setVisible(true);
    }
}

[Stephan van Hulst]

- Separate your game logic from your graphical interface. Use a simple class to keep track of where all the stuff is. The only thing the JPanel should do is draw the stuff, nothing more.
- Don't let your panel implement any listeners. Make separate listener classes, or use anonymous classes.
- Instead of MouseListener, you can use MouseAdapter.
- Use meaningful field names. "pl" means nothing. In a few months, you will have forgotten what it stands for. The same is true for your other fields, to a lesser or greater extent.
- Why are you using protected access? Don't use protected, unless you have a *really* good reason to use it.
- Why are you using the Boolean class, instead of boolean primitives?

[Andrew Monkhouse]

Instead of (int) ((Math.random() * 2) + 1) you might want to look at Random.nextInt(int n).

Instead of

            if (startball == 1) {
                initdir = "left";
            } else {
                initdir = "right";
            }

You might want to consider

            initdir = Random.nextBoolean() ? "left" : "right";

Instead of if (something == true) you might want to simplify it: if (something)

Instead of constants such as "left", "right", "up", "down"; you might want to consider an enum for directions.

You have specified that Pong extends JPanel. Is that correct? Is Pong a JPanel that other people might want to put components in? Or should this be a "has-a" relationship: Pong has a JPanel that it puts it's own components into, but nobody else gets to see.

[john price]

Instead of constants such as "left", "right", "up", "down"; you might want to consider an enum for directions.

You have specified that Pong extends JPanel. Is that correct? Is Pong a JPanel that other people might want to put components in? Or should this be a "has-a" relationship: Pong has a JPanel that it puts it's own components into, but nobody else gets to see.

Could you give an example of an enum?
Could you explain the last paragraph? I don't understand. I know what the "has-a" relationship but I don't get what you are saying.
Thank you for the if (something) comment.
Thanks,
John Price

[john price]

Stephan van Hulst wrote:- Separate your game logic from your graphical interface. Use a simple class to keep track of where all the stuff is. The only thing the JPanel should do is draw the stuff, nothing more.
- Don't let your panel implement any listeners. Make separate listener classes, or use anonymous classes.
- Instead of MouseListener, you can use MouseAdapter.
- Use meaningful field names. "pl" means nothing. In a few months, you will have forgotten what it stands for. The same is true for your other fields, to a lesser or greater extent.
- Why are you using protected access? Don't use protected, unless you have a *really* good reason to use it.
- Why are you using the Boolean class, instead of boolean primitives?

pl means player length, etc. originally had comments for saying p = player etc...will add them back
MouseListener is for Swing, MouseAdapter is for AWT. My code is Swing. Please explain further.
boolean comment taken into effect.
all protected accesses are now changed to private.
please explain first comment.
Thanks,
John Price

[john price]

updated code:

import java.awt.*;
import java.awt.Color.*;
import java.awt.event.*;
import java.awt.geom.*;

import javax.swing.*;
import javax.swing.Timer;

import java.util.*;
import java.util.Random.*;

import java.awt.image.BufferedImage;

public class Pong extends JPanel implements ActionListener, MouseListener, KeyListener, MouseMotionListener {

    //double instead of Double
    private double px = 20.00;
    private double py = 20.00;
    private double pl = 5.00; 
    private double ph = 30.00; 
    private double cx = 370.00;
    private double cy = 100.00;
    private double cl = 5.00; 
    private double ch = 30.00;
    private double bx = 175.00;
    private double by = 100.00;
    private double bl = 5.00;
    private double bh = 5.00;
    private int pscore, cscore, startball, startball1, speed, rand, lost, won;
    private boolean started = false;
    private boolean something = false;
    private String initdir, upslashdown, jfs;
    private Timer timer;
	Random rando = new Random();

    private Pong() {
	this.setFocusable(true);
	this.setBackground(Color.BLACK);
	this.addMouseListener(this); 
	this.addKeyListener(this);
	this.addMouseMotionListener(this);
	}
	public void mouseMoved(MouseEvent me) {
	double ytracker = me.getY();
	if (ytracker >= 142) {
	py = 142;
	}
	else {
	py = (int) ytracker;
	}
	repaint();
	}
	public void mouseDragged(MouseEvent me) {
	}
	public void mousePressed(MouseEvent me) { 
	}
    public void mouseReleased(MouseEvent e) {
    }
    public void mouseEntered(MouseEvent e) {
    }
    public void mouseExited(MouseEvent e) {
    }
    public void mouseClicked(MouseEvent ex) {
	if (started == false) {
			initdir = rando.nextBoolean() ? "left" : "right";
			upslashdown = rando.nextBoolean() ? "up" : "down"; 
			started = true;
		timer = new Timer(20, this);
		timer.start();
		speed = (int) ((Math.random() *5)+2);
          	}
	if (something) {
		something = false;
		initdir = rando.nextBoolean() ? "left" : "right";
		upslashdown = rando.nextBoolean() ? "up" : "down"; 
		started = true;
		repaint();
		if (cscore == 10) {
		lost ++;
		cscore = 0;
		pscore = 0;
		repaint();
		started = false;
		}
		else if (pscore == 10) {
		won ++;
		pscore = 0;
		cscore = 0;
		repaint();
		started = false;
		}
		else {
		timer = new Timer(20, this);
		timer.start();
		speed = (int) ((Math.random() *5)+2);
		}
	}
    }
	public void keyPressed(KeyEvent e) {
	char KeyCar = e.getKeyChar();
	if (KeyCar == 'q') {if (py <= 0) {} else {py -= 10;}repaint();}
	if (KeyCar == 'a') {if (py >= 142) {} else {py += 10;}repaint();}
	if (KeyCar == 'w') {if (py <= 0) {} else {py -= 10;}repaint();}
	if (KeyCar == 's') {if (py >= 142) {} else {py += 10;}repaint();}
	switch(e.getKeyCode()) {
	case KeyEvent.VK_UP : if (py <= 0) {} else {py -= 10;} repaint(); break;
	case KeyEvent.VK_DOWN : if (py >= 142) {} else {py += 10;} repaint(); break;
	}
	}
	public void keyTyped(KeyEvent e) {
	}
	public void keyReleased(KeyEvent e) {
	}
    public void paintComponent (Graphics g) {
        super.paintComponent(g);//removes all previously drawn content and draw from scratch
        Graphics2D g2 = (Graphics2D) g;
	g.setColor(Color.white);
        Rectangle2D rec = new Rectangle2D.Double(cx, cy, cl, ch); //Computer's "paddle"
	g2.fill(rec);
	Rectangle2D rec1 = new Rectangle2D.Double(px, py, pl, ph); //Player's "paddle"
	g2.fill(rec1);
	g.drawString("Score", 175, 20);
	g.drawString("Player: " + pscore, 120, 40);
	g.drawString("Computer: " + cscore, 220, 40);
	g.drawString("Games won : " + won, 120, 60);
	g.drawString("Games lost: " + lost, 220, 60);
	Ellipse2D ball = new Ellipse2D.Double(bx, by, bl, bh); //Ball's position
	g2.fill(ball);
    }

    public void actionPerformed(ActionEvent ex) { //1
	if ((bx >= px) && (bx <= px + pl) && (by >= py) && (by <= py + ph)) { //top edge = px, bottom edge = px + ph ***2
	initdir = "right";
	bx += (int) ((Math.random() * 10)+1);
	rand = (int) ((Math.random() *2)+1);
	if (rand == 1) { //3
	by -= (int) ((Math.random() *10)+1);
	} //2
	else {by += (int) ((Math.random() *10)+1);}
	} //1
	if ((bx + bh >= cx) && (bx <= cx + cl) && (by >= cy) && (by <= cy + ch)) { //top edge = px, bottom edge = px + ph
	initdir = "left";
	bx -= (int) ((Math.random() * 10)+1);
	rand = (int) ((Math.random() *2)+1);
	if (rand == 1) {
	by -= (int) ((Math.random() *10)+1);
	}
	else {by += (int) ((Math.random() *10)+1);}
	}
	if (initdir.equals("right")) { //1
	if (by > cy) { //2
	//cy += speed - 1.020;//1.05 was successful win by me, 1.00 computer won
	//cy += speed - ((Math.random() /2)+0.2);	
	//cy += speed * 0.9;
		if (by >= 142) {
		if ((cy + speed * 0.80 + Math.random()) >= 142) {
		cy = 142;
		}
		else {
		cy += speed * 0.80 + Math.random();
		}
		}
	cy += speed * 0.80 + Math.random();
	} //1
	if (by < cy) { //2
	//cy -= speed - 1.020;
	//cy -= speed - ((Math.random() /2)+0.4);
	cy -= speed * 1.05 + Math.random();
	} //1
	}
	else if (initdir.equals("left")) { //2
	cy = 75.00;
	} //1
	if (initdir.equals("left")) { //2
		if (bx <= 0) { //3
		cscore ++;
		timer.stop();
		something = true;
		bx = 175.00;
    		by = 100.00;
    		bl = 5.00;
   		bh = 5.00;
		} //2
			else {bx -= speed;}
			} //1
		else if (initdir.equals("right")) { //2
			if (bx >= 400 - bl) { //3
			pscore ++;
			timer.stop();
			something = true;
			bx = 175.00;
    			by = 100.00;
    			bl = 5.00;
   			bh = 5.00; 
			}
			else {bx += speed;}
			}
	if (upslashdown.equals("up")) {
		if (by <= 0) {
		by += (int) ((Math.random() * 10)+1);
		upslashdown = "down";
		} //2
		else {by -= speed;}
	}
	else if (upslashdown.equals("down")) {
		if (by >= 200 - bh * 6.0) {
		by -= (int) ((Math.random() * 10)+1);
		upslashdown = "up";
		}
		else {by += speed;}
	}
	repaint(); //executes no matter what
    }

    public static void main(String[] args) {
        JFrame frame = new JFrame();
        frame.setTitle("Pong");
        frame.setSize(400, 200);
	Toolkit tk = Toolkit.getDefaultToolkit();
    	Dimension d = tk.getScreenSize();
	frame.setLocation(d.width /3, d.height /4);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        Container contentPane = frame.getContentPane();
	BufferedImage cursorImg = new BufferedImage(16, 16, BufferedImage.TYPE_INT_ARGB);

// Create a new blank cursor.
Cursor blankCursor = Toolkit.getDefaultToolkit().createCustomCursor(
    cursorImg, new Point(0, 0), "blank cursor");

// Set the blank cursor to the JFrame.
frame.getContentPane().setCursor(blankCursor);
        contentPane.add(new Pong());
	frame.setResizable(false);
        frame.setVisible(true);
    }
}

[Andrew Monkhouse]

john price wrote:Could you give an example of an enum?

    enum Direction {
        LEFT, RIGHT, UP, DOWN
    }

    private Direction initdir, upslashdown;

    initdir = random.nextBoolean() ? Direction.LEFT : Direction.RIGHT;
    upslashdown = random.nextBoolean() ? Direction.UP : Direction.DOWN;

    if (upslashdown == Direction.UP) {

john price wrote:Could you explain the last paragraph? I don't understand. I know what the "has-a" relationship but I don't get what you are saying.

This relates to what IS an object, and what other objects can you say your object HAS-A instance of? (sorry for the bad grammar). A simple example might be considering something like a specific type of fruit, such as an apple, and apple seeds. In this case, we can say that an Apple IS-A fruit, and it HAS-A seed (more than one really). It would be incorrect to say that an Apple IS-A seed, or than an Apple HAS-A fruit.

This is something you need to think about when developing Object Oriented code. Is your class a real sub-class of some other class (IS-A), or does it simply use that other class (HAS-A). In the case I was asking about, you said that Pong extends JPanel. This means that Pong IS-A JPanel. A JPanel IS-A container that anyone can put anything in. So in saying that your class extends JPanel, you are saying that your class is a container that anyone can put anything in. This means that someone could create their own class that either extends or uses Pong and put objects into it that obscure your application. This is almost certainly not want you want. Whereas if you say that Pong just happens to use a JPanel, (HAS-A) then you can keep everyone else from changing it.

[Andrew Monkhouse]

john price wrote:

Stephan van Hulst wrote:- Use meaningful field names. "pl" means nothing. In a few months, you will have forgotten what it stands for. The same is true for your other fields, to a lesser or greater extent.

pl means player length, etc. originally had comments for saying p = player etc...will add them back

Two points:

It is great that you know what pl stands for today. But if you come back to this same code in six months time will you still remember what it stood for? What about 5 years from now? (Speaking from experience, if you work on 2 other projects tomorrow, then 3 days from now you will probably have forgotten what pl stands for)

One of the major advantages we have gained in the last few years is that there is no problem with having longer variable names. It was not so long ago that a variable name like pl would have been considered great, since manually typing it 60 times in one file over a slow modem would be much better than trying to type playerLength 60 times over a slow modem. These days I start typing pl and the IDE suggests that it should be playerLength, so I get code that is much easier to read, with almost no sacrifice of speed.

[john price]

So what do I need to do to fix it? For Java GUI's, I have only used Swing. It is useful and works for me. Should I be using a canvas (AWT) or something else. If my things are private, then no one can use them outside of my Application. So it would be pointless to extend Pong.
Confused,
John Price aka cc11rocks

EDIT: Also, thank you for the variable suggestion. I will change it now.

[Stephan van Hulst]

MouseListener is AWT as well. It doesn't matter. Swing and AWT play nice together. MouseAdapter is a class that implements MouseListener and MouseMotionListener with empty methods. This way, you won't have to implement them yourself, if you don't use them.

You should have a JPanel subclass that has a Pong field. Pong keeps track of the location of everything. Your JPanel subclass looks at the Pong to determine where it should draw things.

john price wrote:If my things are private, then no one can use them outside of my Application. So it would be pointless to extend Pong.

Exactly. You don't want anything to extend Pong. You want other classes to use Pong. You should try to avoid extending classes whenever possible.

[john price]

Stephan van Hulst wrote:MouseListener is AWT as well. It doesn't matter. Swing and AWT play nice together. MouseAdapter is a class that implements MouseListener and MouseMotionListener with empty methods. This way, you won't have to implement them yourself, if you don't use them.

You should have a JPanel subclass that has a Pong field. Pong keeps track of the location of everything. Your JPanel subclass looks at the Pong to determine where it should draw things.

So make Pong() JLabel()?
I don't get it.

And anyway, since I already have MouseListener set up, shouldn't I leave it alone and just use this for future reference?

Anyway, here is human friendly, easily "rememberal" code:

import java.awt.*;
import java.awt.Color.*;
import java.awt.event.*;
import java.awt.geom.*;

import javax.swing.*;
import javax.swing.Timer;

import java.util.*;
import java.util.Random.*;

import java.awt.image.BufferedImage;

public class Pong extends JPanel implements ActionListener, MouseListener, KeyListener, MouseMotionListener {

    //double instead of Double
    private double playerX_pos = 20.00;
    private double playerY_pos = 20.00;
    private double playerLength = 5.00; 
    private double playerHeight = 30.00; 
    private double computerX_pos = 370.00;
    private double computerY_pos = 100.00;
    private double computerLength = 5.00; 
    private double computerHeight = 30.00;
    private double ballX_pos = 175.00;
    private double ballY_pos = 100.00;
    private double ballLength = 5.00;
    private double ballHeight = 5.00;
    private int pscore, cscore, startball, startball1, speed, rand, lost, won;
    private boolean started = false;
    private boolean something = false;
    private String initdir, upslashdown, jfs;
    private Timer timer;
	Random rando = new Random();

    private Pong() {
	this.setFocusable(true);
	this.setBackground(Color.BLACK);
	this.addMouseListener(this); 
	this.addKeyListener(this);
	this.addMouseMotionListener(this);
	}
	public void mouseMoved(MouseEvent me) {
	double ytracker = me.getY();
	if (ytracker >= 142) {
	playerY_pos = 142;
	}
	else {
	playerY_pos = (int) ytracker;
	}
	repaint();
	}
	public void mouseDragged(MouseEvent me) {
	}
	public void mousePressed(MouseEvent me) { 
	}
    public void mouseReleased(MouseEvent e) {
    }
    public void mouseEntered(MouseEvent e) {
    }
    public void mouseExited(MouseEvent e) {
    }
    public void mouseClicked(MouseEvent ex) {
	if (started == false) {
			initdir = rando.nextBoolean() ? "left" : "right";
			upslashdown = rando.nextBoolean() ? "up" : "down"; 
			started = true;
		timer = new Timer(20, this);
		timer.start();
		speed = (int) ((Math.random() *5)+2);
          	}
	if (something) {
		something = false;
		initdir = rando.nextBoolean() ? "left" : "right";
		upslashdown = rando.nextBoolean() ? "up" : "down"; 
		started = true;
		repaint();
		if (cscore == 10) {
		lost ++;
		cscore = 0;
		pscore = 0;
		repaint();
		started = false;
		}
		else if (pscore == 10) {
		won ++;
		pscore = 0;
		cscore = 0;
		repaint();
		started = false;
		}
		else {
		timer = new Timer(20, this);
		timer.start();
		speed = (int) ((Math.random() *5)+2);
		}
	}
    }
	public void keyPressed(KeyEvent e) {
	char KeyCar = e.getKeyChar();
	if (KeyCar == 'q') {if (playerY_pos <= 0) {} else {playerY_pos -= 10;}repaint();}
	if (KeyCar == 'a') {if (playerY_pos >= 142) {} else {playerY_pos += 10;}repaint();}
	if (KeyCar == 'w') {if (playerY_pos <= 0) {} else {playerY_pos -= 10;}repaint();}
	if (KeyCar == 's') {if (playerY_pos >= 142) {} else {playerY_pos += 10;}repaint();}
	switch(e.getKeyCode()) {
	case KeyEvent.VK_UP : if (playerY_pos <= 0) {} else {playerY_pos -= 10;} repaint(); break;
	case KeyEvent.VK_DOWN : if (playerY_pos >= 142) {} else {playerY_pos += 10;} repaint(); break;
	}
	}
	public void keyTyped(KeyEvent e) {
	}
	public void keyReleased(KeyEvent e) {
	}
    public void paintComponent (Graphics g) {
        super.paintComponent(g);//removes all previously drawn content and draw from scratch
        Graphics2D g2 = (Graphics2D) g;
	g.setColor(Color.white);
        Rectangle2D rec = new Rectangle2D.Double(computerX_pos, computerY_pos, computerLength, computerHeight);
	g2.fill(rec);
	Rectangle2D rec1 = new Rectangle2D.Double(playerX_pos, playerY_pos, playerLength, playerHeight);
	g2.fill(rec1);
	g.drawString("Score", 175, 20);
	g.drawString("Player: " + pscore, 120, 40);
	g.drawString("Computer: " + cscore, 220, 40);
	g.drawString("Games won : " + won, 120, 60);
	g.drawString("Games lost: " + lost, 220, 60);
	Ellipse2D ball = new Ellipse2D.Double(ballX_pos, ballY_pos, ballLength, ballHeight);
	g2.fill(ball);
    }

    public void actionPerformed(ActionEvent ex) { //1
	if ((ballX_pos >= playerX_pos) && (ballX_pos <= playerX_pos + playerLength) && (ballY_pos >= playerY_pos) && (ballY_pos <= playerY_pos + playerHeight)) { 
	initdir = "right";
	ballX_pos += (int) ((Math.random() * 10)+1);
	rand = (int) ((Math.random() *2)+1);
	if (rand == 1) { //3
	ballY_pos -= (int) ((Math.random() *10)+1);
	} //2
	else {ballY_pos += (int) ((Math.random() *10)+1);}
	} //1
	if ((ballX_pos + ballHeight >= computerX_pos) && (ballX_pos <= computerX_pos + computerLength) && (ballY_pos >= computerY_pos) && (ballY_pos <= computerY_pos + computerHeight)) {
	initdir = "left";
	ballX_pos -= (int) ((Math.random() * 10)+1);
	rand = (int) ((Math.random() *2)+1);
	if (rand == 1) {
	ballY_pos -= (int) ((Math.random() *10)+1);
	}
	else {ballY_pos += (int) ((Math.random() *10)+1);}
	}
	if (initdir.equals("right")) { //1
	if (ballY_pos > computerY_pos) { //2
	//computerY_pos += speed - 1.020;//1.05 was successful win by me, 1.00 computer won
	//computerY_pos += speed - ((Math.random() /2)+0.2);	
	//computerY_pos += speed * 0.9;
		if (ballY_pos >= 142) {
		if ((computerY_pos + speed * 0.80 + Math.random()) >= 142) {
		computerY_pos = 142;
		}
		else {
		computerY_pos += speed * 0.80 + Math.random();
		}
		}
	computerY_pos += speed * 0.80 + Math.random();
	} //1
	if (ballY_pos < computerY_pos) { //2
	//computerY_pos -= speed - 1.020;
	//computerY_pos -= speed - ((Math.random() /2)+0.4);
	computerY_pos -= speed * 1.05 + Math.random();
	} //1
	}
	else if (initdir.equals("left")) { //2
	computerY_pos = 75.00;
	} //1
	if (initdir.equals("left")) { //2
		if (ballX_pos <= 0) { //3
		cscore ++;
		timer.stop();
		something = true;
		ballX_pos = 175.00;
    		ballY_pos = 100.00;
    		ballLength = 5.00;
   		ballHeight = 5.00;
		} //2
			else {ballX_pos -= speed;}
			} //1
		else if (initdir.equals("right")) { //2
			if (ballX_pos >= 400 - ballLength) { //3
			pscore ++;
			timer.stop();
			something = true;
			ballX_pos = 175.00;
    			ballY_pos = 100.00;
    			ballLength = 5.00;
   			ballHeight = 5.00; 
			}
			else {ballX_pos += speed;}
			}
	if (upslashdown.equals("up")) {
		if (ballY_pos <= 0) {
		ballY_pos += (int) ((Math.random() * 10)+1);
		upslashdown = "down";
		} //2
		else {ballY_pos -= speed;}
	}
	else if (upslashdown.equals("down")) {
		if (ballY_pos >= 200 - ballHeight * 6.0) {
		ballY_pos -= (int) ((Math.random() * 10)+1);
		upslashdown = "up";
		}
		else {ballY_pos += speed;}
	}
	repaint(); //executes no matter what
    }

    public static void main(String[] args) {
        JFrame frame = new JFrame();
        frame.setTitle("Pong");
        frame.setSize(400, 200);
	Toolkit tk = Toolkit.getDefaultToolkit();
    	Dimension d = tk.getScreenSize();
	frame.setLocation(d.width /3, d.height /4);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        Container contentPane = frame.getContentPane();
	BufferedImage cursorImg = new BufferedImage(16, 16, BufferedImage.TYPE_INT_ARGB);

// Create a new blank cursor
Cursor blankCursor = Toolkit.getDefaultToolkit().createCustomCursor(
    cursorImg, new Point(0, 0), "blank cursor");

// Set the blank cursor to the JFrame
frame.getContentPane().setCursor(blankCursor);
        contentPane.add(new Pong());
	frame.setResizable(false);
        frame.setVisible(true);
    }
}

[Andrew Monkhouse]

john price wrote:So what do I need to do to fix it?

Instead of saying:

public class Pong extends JPanel implements ActionListener, MouseListener, KeyListener, MouseMotionListener {
    // ....

You should say

public class Pong implements ActionListener, MouseListener, KeyListener, MouseMotionListener {
    private JPanel myDisplay = new JPanel();
    // ....

(Except that, as noted by others, you really shouldn't have Pong implement listeners either - separate listener classes and/or use of adapter classes with anonymous classes will make your code so much easier to read).

john price wrote:For Java GUI's, I have only used Swing. It is useful and works for me. Should I be using a canvas (AWT) or something else.

Nope - this is completely avoiding trying to solve the problem. What you are doing at the moment is saying something like a dog is a tail. Whereas you should be saying that a dog has a tail. Simply changing the animal type does not stop the overall problem of trying to say that a dog and a tail are the same.

john price wrote:If my things are private, then no one can use them outside of my Application. So it would be pointless to extend Pong.

Oops - I missed that you had the constructor as private. This is also considered bad style unless you have a very specific reason for doing this.

If you had not made your constructor private, I could have done some weird things to your project. For example, if I take the code you first posted, I can do a simple attempt at cheating by doing something as simple as:

import java.awt.*;
import java.awt.Color.*;
import java.awt.event.*;
import java.awt.geom.*;

import javax.swing.*;
import javax.swing.Timer;

import java.util.*;
import java.util.Random.*;

import java.awt.image.BufferedImage;

public class DumbPong extends Pong {
    public void mouseMoved(MouseEvent me) {  
        py = by;
        repaint();  
    }  

    public static void main(String[] args) {
        JFrame frame = new JFrame();
        frame.setTitle("Pong");
        frame.setSize(400, 200);
        Toolkit tk = Toolkit.getDefaultToolkit();
        Dimension d = tk.getScreenSize();
        frame.setLocation(d.width / 3, d.height / 4);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        Container contentPane = frame.getContentPane();
        BufferedImage cursorImg = new BufferedImage(16, 16, BufferedImage.TYPE_INT_ARGB);

// Create a new blank cursor.
        Cursor blankCursor = Toolkit.getDefaultToolkit().createCustomCursor(
                cursorImg, new Point(0, 0), "blank cursor");

// Set the blank cursor to the JFrame.
        frame.getContentPane().setCursor(blankCursor);
        contentPane.add(new DumbPong());
        frame.setResizable(false);
        frame.setVisible(true);
    }
}

I should not have had access to any of those methods or variables.

To put it another way - while you have protected your methods and variables, you have declared that Pong IS-A JPanel. Which means that I have access to all the fields and methods that JPanel has made public and that any ancestors of JPanel have made public.

[Stephan van Hulst]

Here is some skeleton code to give you some ideas:

final class Pong {
  // stuff related to your game
}

final class PongPanel extends JPanel {
  private Pong pong;
  private Timer timer;

  PongPanel() {
    MouseAdapter mouse = new MouseAdapter() {

      @Override
      public void mouseMoved(MouseEvent e) {
        //
      }

      @Override
      public void mouseClicked(MouseEvent e) {
        //
      }
    };

    addMouseMotionListener(mouse);
    addMouseListener(mouse);

    addKeyListener(new KeyAdapter() {

      @Override
      public void keyPressed(KeyEvent e) {
        //
      }
    });

    timer = new Timer(20, new ActionListener() {
      public void actionPerformed(ActionEvent e) {
        //
      }
    });

    timer.start();
  }

  void setPong(Pong pong) {}

  @Override
  protected void paintComponent(Graphics g) {
    super.paintComponent(g);
    if (pong != null) {
      // draw stuff from pong
    }
  }
}

[john price]

Why don't I just add the word "final" to my code. Then no subclasses can extend Pong and the "bad things" I did don't matter...

[Andrew Monkhouse]

Perhaps we misunderstood. Your original request was for suggestions on how to make the code better. Were you looking for something other than what we are providing? Performance suggestions perhaps?

[john price]

Andrew Monkhouse wrote:Perhaps we misunderstood. Your original request was for suggestions on how to make the code better. Were you looking for something other than what we are providing? Performance suggestions perhaps?

Well you pointed out a problem. Wouldn't adding the keyword "final" fix it? How could I get better performance from this?
I was thinking making the ball movement algorithm better and the AI skill level (algorithm). I want the ball to look like it's bouncing off of walls and paddles normally but be semi-random (not the same pattern every time; otherwise boring game because it's the same every time). I want the AI to be beatable but not too easy. This is what I was thinking. Also, I think my ball bouncing off of the AI's paddle algorithm is off. The ball seems to be hitting the backside of the AI paddle, the same way as the player paddle. This is what I was thinking of but any other suggestions, I would like too. I'm a noob Java programmer so I am having a tough time with your previous suggestions.
Thanks and sorry,
cc11rocks

EDIT: I've also noticed an issue with the paddle moving up. I've had to change the original:

computerY_pos -= speed * 0.80 + Math.random();

to:

computerY_pos -= speed * 1.05 + Math.random();

While when the paddle goes to hit the ball down the code is:

computerY_pos += speed * 0.80 + Math.random();

which works fine.
It should work about the same moving up or down so I'm confused on this part.

[Mich Robinson]

Wouldn't this be better as an applet rather than an application?

This bit should be defined as a function (int) ((Math.random() *x)+1)

The comparisons against "right" "left" etc should really be with integer constants.

You wanted to know how to make it faster but surely if pong won't run fast on a modern computer then there's something very wrong somewhere.

You don't want to add new features? pong is 40 years old (which in dog years is as old as it gets) I think if you're going to spend the time to write pong then you should at least add a few new features!

[Andrew Monkhouse]

Heh - OK, so we were giving you answers that were completely at odds with what you were asking. Oops.

John wrote:I want the ball to look like it's bouncing off of walls and paddles normally but be semi-random (not the same pattern every time; otherwise boring game because it's the same every time).

There are books dedicated to how to do physics in games, so believe me when I say that anything I write here is only the tip of the iceberg.

Think about what happens when you hit a ball with a bat in real life: if the ball is coming straight at you, and you hit the ball with the center of your bat, it will fly straight. If you hit it with the outer edge or the inner edge, it will go off on a different angle. Some bad graphics to demonstrate:

            |     \ |           |  /
            |      \|           | /
            v       v            v
Bat:     -------   -------  -------

(Imagine the vertical line is the ball heading towards your bat, and the angled line is the ball leaving your bat). The opposite can be considered to be true - if the ball came in on a an angle, and you hit it with the correct edge of your bat, then it might approximate a straight line leaving your bat.

A variation on this is that if a ball is coming at you on an angle, and you hit it with your bat dead center, then the angle at which it leaves added to the angle at which it enters should equal 90° to which it came in:

          \   /
           \ /
            v  
Bat:     -------

So if it came in on a 45° angle, it should leave at (90° - 45°) 45°. If it came in at 30° and you hit it with the center of your bat, it would leave at (90° - 30°) 60° In addition, you can mimic the other rules - you might decide that the very edge of your bat is the 180° angle point - any ball that hits it should leave at 180° minus the incoming angle.

Since a human is likely to vary exactly what part of their bat hits the ball at any given time, the angle with which the ball leaves the bat will vary each time. This will naturally increase the apparent randomness of the game.

To make it easier for the human, you can have the AI make it's best effort to hit the ball in such a way that it is returned almost straight on to the human (thereby giving them the easiest chance of hitting it). You will see this all the time if you watch a parent trying to teach their child tennis - the kid will hit the ball all over the place, inadvertently making the parent run for the ball, while the parent will do their best to get the ball back to the child. Of course you will want to have some randomness so that the AI does not do this perfectly every time, or else it will be boring for the human.

Hopefully that has given you some ideas. I'll be offline for a bit, so hopefully some other people will chime in with some ideas as well.

[Emmanuel Borges]

I think you should pay more attention to some concepts of Object Oriented Programming.

For example, the game Pong has one ball and two paddles, so you could create a Ball class and a Paddle class. This way, all the logic concerning these two entities would be encapsulated within their classes.

You could also create a manager class that maintains the game main loop and a class responsible for drawing entities on screen (Separation of Concerns). This way, your code would be more readable and maintainable.

Of course, you will have to think a bit to figure out how to organize all the information, but it's worth it.

[Stephan van Hulst]

Emmanuel F. Borges wrote:For example, the game Pong has one ball and two paddles, so you could create a Ball class and a Paddle class. This way, all the logic concerning these two entities would be encapsulated within their classes.

I disagree. Object Oriented programming is a means, not a goal. In this case, one class would be more than enough to hold all the game logic. It only needs to keep track of the positions and dimension of two paddles and a ball, and that's it. Making separate classes would make the code much more verbose than it has to be, in my opinion.

However, I agree that this one game class should be completely separate from everything else, like the GUI.