patternjavaMajor
Simple Java animation with Swing
Viewed 0 times
animationsimplewithjavaswing
Problem
I am learning to use Java Swing and have made a simple animation that makes a small shape bounce around the predetermined borders of a panel. The program works well, but I am looking for feedback in terms of quality and any improvements or alternative methods that could be used in such an application.
```
import javax.swing.*;
import java.awt.*;
final public class Tester {
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 Tester().go();
}
private void go() {
frame = new JFrame("Test");
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
drawPanel = new DrawPanel();
frame.getContentPane().add(BorderLayout.CENTER, drawPanel);
frame.setVisible(true);
frame.setResizable(false);
frame.setSize(300, 300);
frame.setLocation(375, 55);
moveIt();
}
class DrawPanel extends JPanel {
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 = 259){
up = true;
down = false;
}
if(oneY <= 7){
up = false;
down = true;
}
if(up){
oneY--;
}
if(down){
oneY++;
```
import javax.swing.*;
import java.awt.*;
final public class Tester {
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 Tester().go();
}
private void go() {
frame = new JFrame("Test");
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
drawPanel = new DrawPanel();
frame.getContentPane().add(BorderLayout.CENTER, drawPanel);
frame.setVisible(true);
frame.setResizable(false);
frame.setSize(300, 300);
frame.setLocation(375, 55);
moveIt();
}
class DrawPanel extends JPanel {
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 = 259){
up = true;
down = false;
}
if(oneY <= 7){
up = false;
down = true;
}
if(up){
oneY--;
}
if(down){
oneY++;
Solution
Things I would fix:
Don't do this:
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
Don't ever do this:
Say that you do get an
It was also mentioned in the comments to catch specific
But you could also do this:
Right now there are some hard-coded values:
I would store those values in variables, and then I would make it so that it is more scalable. For example:
That way, you only have to change one value in order to scale your different values. This is just an example, and has not been implemented in my code. I left this for you to do on your own.
Recommendations that are optional:
You have a lot of
If you don't expand on them (which I don't see a need to), you can save some LOC:
Right now you are setting the location:
You don't have to do this, but I prefer the OS to set the location:
You could also set the frame to the center of the window:
But I never liked that too much. It makes your program seem like a pop-up, which I despise.
I always do
Right now your are doing this with your braces:
Since you are a beginner, I would recommend lining up your braces (I still do this, and I've been programming for a while):
This is a matter of taste, and it is perfectly fine to stay with what you are doing right now.
You might note that this:
Is a bit different than your usual:
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 =
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();
}But you could also do this:
catch (IOException | InterruptedException e)
{
e.printStackTrace();
}Right now there are some hard-coded values:
if(oneX >= 283)
if(oneX <= 7)
...I would store those values in variables, and then I would make it so that it is more scalable. For example:
int xScale = 280;
int yScale = xScale / 40;That way, you only have to change one value in order to scale your different values. This is just an example, and has not been implemented in my code. I left this for you to do on your own.
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)
{
// ...
}This is a matter of taste, and it is perfectly fine to stay with what you are doing right now.
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 =
Code Snippets
import javax.swing.*;
import java.awt.*;private static final long serialVersionUID = 1L;catch (Exception exc)
{
}catch (Exception exc)
{
exc.printStackTrace();
}catch (IOException io)
{
io.printStackTrace()
}
catch (InterruptedException ie)
{
ie.printStackTrace();
}Context
StackExchange Code Review Q#29630, answer score: 25
Revisions (0)
No revisions yet.