Please criticize this!

[jackmaverick1]

So. I've written my first little graphics game (using the Graphics API) and would like to know two things:

a) What's wrong with my collision detection? (Could you point me in the right direction?)

b) What am I doing that is not the best way. Is there a better way to do something?

I've attached my Images (Do you like them? I made them myself!), and of course, the code.

Thanks,
Jack

[NormR1]

Please post any code you have that you want us to look at here on the forum.

[jackmaverick1]

I didn't attach source to the post? Anyway, here it is:
The images are called: en_norm.jpg and cookie_big.jpg.

Entity.java:

import java.awt.Graphics;
import java.awt.Point;
import java.awt.Rectangle;

import javax.swing.ImageIcon;

public class Entity {
	ImageIcon img;
	int x = 20, y = 20, width, height;
	public Entity(String path)
	{
		img = new ImageIcon(path);
		width = img.getIconWidth();
		height = img.getIconHeight();
	}
	Point getPoint() {
		Point abc = new Point();
		abc.x = x;
		abc.y = y;
		return abc;
	}

	public void draw(Graphics g, int x, int y, int type) {
		switch (type) {
		case 1:
			g.drawImage(img.getImage(), x, y, null);
		case 2:
			g.drawImage(img.getImage(), 60, 60, null);
		}
	}
void updateRect(Rectangle e)
{
	e.x = x;
	e.y = y;
}
	boolean collisionWith(Entity e) {
		/*if(e.x > x && e.x < (width + x))
		{
			if();
		}
		//This is a new thing that I'm working on.
		*/
		return Frame.cookie.intersects(Frame.player);
	}
}

GamePanel.java

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


import javax.swing.JLabel;
import javax.swing.JPanel;


public class GamePanel extends JPanel {
	JLabel a = new JLabel("Arrow Keys to move");
public GamePanel()
{
	this.add(a);
}
public void paintComponent(Graphics g)
{
	Entity pickMeUp = new Entity("cookie_big.jpg");
	super.paintComponent(g);//Paints the JPanels stuff.
	Entity a =new Entity("en_norm.jpg");//Our object
	a.draw(g,Frame.x,Frame.y, 1);//Call the painter (he-he).
	a.x = Frame.x;
	a.y = Frame.y;
	pickMeUp.draw(g, 61, 58, 2);
	System.out.println("x: "+a.x + " y: "+a.y+"\n"+pickMeUp.collisionWith(a));
	a.updateRect(Frame.player);
	pickMeUp.updateRect(Frame.cookie);
}
}

Frame.java

import java.awt.Rectangle;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;

import javax.swing.ImageIcon;
import javax.swing.JFrame;

public class Frame extends JFrame implements KeyListener {
	/**
	 * 
	 */
	private static final long serialVersionUID = 1L;
	static int x, y;
	static Rectangle cookie = new Rectangle(), player = new Rectangle();

	Frame() {
		x = this.getWidth() / 2;
		y = this.getHeight() / 2;
		this.setSize(400, 400);
		this.setResizable(false);
		GamePanel c = new GamePanel();
		add(c);
		this.addKeyListener(this);
		this.setVisible(true);
		this.setDefaultCloseOperation(EXIT_ON_CLOSE);
		while (true) {// Game Loop
			c.repaint();
		}
	}

	public static void main(String[] args) {
		Frame k = new Frame();
	}

	public void keyPressed(KeyEvent arg0) {
		// TODO Auto-generated method stub
		// Stay in the bounds - start
		if ((x > this.getWidth() - new ImageIcon("en_norm.jpg").getIconWidth())) {
			x -= 3;
			return;
		}
		if (y > this.getHeight() - new ImageIcon("en_norm.jpg").getIconHeight()) {
			y -= 3;
			return;
		}
		if ((x < 1)) {
			x += 1;
			return;
		}
		if ((y < 1)) {
			y += 1;
			return;
		}
		// stay in the bounds - END
		// Movement - Start.
		if (arg0.getKeyCode() == KeyEvent.VK_LEFT) {
			x -= 3;
		} else if (arg0.getKeyCode() == KeyEvent.VK_UP) {
			y -= 3;
		} else if (arg0.getKeyCode() == KeyEvent.VK_DOWN) {
			y += 3;
		} else if (arg0.getKeyCode() == KeyEvent.VK_RIGHT) {
			x += 3;
		} else if (arg0.isAltDown() && arg0.getKeyCode() == KeyEvent.VK_F4) {
			System.exit(0);
		}
		// Movement - END
	}

	@Override
	public void keyReleased(KeyEvent e) {
		// TODO Auto-generated method stub

	}

	@Override
	public void keyTyped(KeyEvent e) {
		// TODO Auto-generated method stub

	}
}

[NormR1]

Problems:
use of static variables.
are the case statements meant to fall thru?

if ((x > this.getWidth() - new ImageIcon("en_norm.jpg").getIconWidth())) {

This is a terrible way to code.
Get the image ONE time and use it's properties, don't get it every time the method is executed.

[jackmaverick1]

Thank you. Is there anything else?

[zeroliken]

Are you still having problems on the collision?
Maybe this tutorial ca help ya
collision tutorial

[NormR1]

Is there anything else?

The infinite while loop should be changed to use a timer.

[JamesCherrill]

while (true) {// Game Loop
   c.repaint();
}

This is really not a good idea. You tie up a thread running as fast as it possibly can, triggering repaints at an arbitrary but very high rate. It's going to burn all the CPU it can, and will run at completely different speeds on different machines.
Here's the recommended strategy for running animated games in Swing:
Use a javax.swing.Timer to call an update routine a some suitable rate (eg every 50 mSec). In that routine update all the variables associated with the sprite's position/velocity/collisions etc. After updating all the variable call repaint();
In paintComponent re-draw everything according to their current variables. Do not update those variables in paintComponent because you have no control of exactly when or how often it will be called.