Things I would fix:
Don't do this:
import javax.swing.*;
import java.awt.*;
It could be problematic for the compiler to import a bunch of packages at once. If two packages provide the same type, both are imported, and the type is used in the class, a compile-time error occurs. This is described in JLS 6.5.5.1:
Otherwise, if a type of that name is declared by more than one type-import-on-demand declaration of the compilation unit, then the name is ambiguous as a type name; a compile-time error occurs.
In addition, it also saves a bit of memory. And your IDE (if you use one), should have the ability to do this automatically.
The serializable class DrawPanel
does not declare a static final serialVersionUID
field of type long
(thanks to @GilbertLeBlanc for the link):
private static final long serialVersionUID = 1L;
Don't ever do this:
catch (Exception exc)
{
}
Say that you do get an Exception
thrown at you. You aren't handling it right now in any way. At least print a stack trace to help you solve problems you may encounter in the future:
catch (Exception exc)
{
exc.printStackTrace();
}
It was also mentioned in the comments to catch specific Exceptions
. Here there is no real need to do that since only an InterruptedException
is being thrown. When you do need to catch multiple Exceptions
, some people might tell you to do this:
catch (IOException io)
{
io.printStackTrace()
}
catch (InterruptedException ie)
{
ie.printStackTrace();
}
Recommendations that are optional:
You have a lot of if
statements with braces:
if(down)
{
oneY++;
}
If you don't expand on them (which I don't see a need to), you can save some LOC:
if (down) oneY++;
Right now you are setting the location:
frame.setLocation(375, 55);
You don't have to do this, but I prefer the OS to set the location:
frame.setLocationByPlatform(true);
You could also set the frame to the center of the window:
frame.setLocationRelativeTo(null);
But I never liked that too much. It makes your program seem like a pop-up, which I despise.
I always do frame.setVisible(true)
after I am completely finished setting up the frame.
Right now your are doing this with your braces:
void someMethod(int someInt){
// ...
}
Since you are a beginner, I would recommend lining up your braces (I still do this, and I've been programming for a while):
void someMethod(int someInt)
{
// ...
}
You might note that this:
public static void main(String... args)
Is a bit different than your usual:
public static void main(String[] args)
They both do the same thing, but one uses variable arity parameters.
Final code:
import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.Graphics;
import javax.swing.JFrame;
import javax.swing.JPanel;
final public class Test
{
JFrame frame;
DrawPanel drawPanel;
private int oneX = 7;
private int oneY = 7;
boolean up = false;
boolean down = true;
boolean left = false;
boolean right = true;
public static void main(String... args)
{
new Test().go();
}
private void go()
{
frame = new JFrame("Test");
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
drawPanel = new DrawPanel();
frame.getContentPane().add(BorderLayout.CENTER, drawPanel);
frame.setResizable(false);
frame.setSize(300, 300);
frame.setLocationByPlatform(true);
frame.setVisible(true);
moveIt();
}
class DrawPanel extends JPanel
{
private static final long serialVersionUID = 1L;
public void paintComponent(Graphics g)
{
g.setColor(Color.BLUE);
g.fillRect(0, 0, this.getWidth(), this.getHeight());
g.setColor(Color.RED);
g.fillRect(3, 3, this.getWidth() - 6, this.getHeight() - 6);
g.setColor(Color.WHITE);
g.fillRect(6, 6, this.getWidth() - 12, this.getHeight() - 12);
g.setColor(Color.BLACK);
g.fillRect(oneX, oneY, 6, 6);
}
}
private void moveIt()
{
while (true)
{
if (oneX >= 283)
{
right = false;
left = true;
}
if (oneX <= 7)
{
right = true;
left = false;
}
if (oneY >= 259)
{
up = true;
down = false;
}
if (oneY <= 7)
{
up = false;
down = true;
}
if (up) oneY--;
if (down) oneY++;
if (left) oneX--;
if (right) oneX++;
try
{
Thread.sleep(10);
}
catch (Exception e)
{
e.printStackTrace();
}
frame.repaint();
}
}
}