23
\$\begingroup\$

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 <= 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 exc){}
            frame.repaint();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ import.java.awt.BorderLayout should be more efficient \$\endgroup\$ – user54668 Oct 9 '14 at 4:19
  • \$\begingroup\$ try using a switch instead of if/else sta \$\endgroup\$ – user100802 Mar 21 '16 at 10:13
26
\$\begingroup\$

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();
}

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 = 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();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Very helpful insights! Thank you for putting that much into the answer. I didn't even know about setting location based on OS and I'm going to try that to see how I like it. It seems more reasonable in a lot of ways what with people having different screen resolutions. My only question is what does: private static final long serialVersionUID = 1L; really do? \$\endgroup\$ – Jeremy Johnson Aug 12 '13 at 9:39
  • \$\begingroup\$ I just tried setting the location based on OS and I like that a LOT better! Thank you so much for showing me that! \$\endgroup\$ – Jeremy Johnson Aug 12 '13 at 10:51
  • 2
    \$\begingroup\$ @Jeremy Johnson: serialVersionUID; stackoverflow.com/questions/285793/… \$\endgroup\$ – Gilbert Le Blanc Aug 12 '13 at 12:21
  • \$\begingroup\$ @GilbertLeBlanc appologies if I'm missing the point here, but doesn't that mean that the serialVersionUID is not indicated in a class that doesn't implement Serializable? \$\endgroup\$ – Jeremy Johnson Aug 12 '13 at 16:21
  • \$\begingroup\$ @Jeremy Johnson: A serialVersionID is not necessary unless the class, or a class you're extending, implements Serializable. \$\endgroup\$ – Gilbert Le Blanc Aug 12 '13 at 16:29
6
\$\begingroup\$

Your code has a lot of magic numbers in it.

if(oneX >= 283){
   right = false;
   left = true;
}

What is special about 283? What does it actually mean? If you define a constant that happens to have the value of 283, it will be much easier to update your code if the value needs to change.


try{
    Thread.sleep(10);
} catch (Exception exc){}

Catching Exception is bad. You always want to catch the most specific exception you can to do what you need. In this case, InterruptedException would be the correct exception to catch. This is not a great example because there is only one exception that can be thrown. However, in the future, it will prevent you for accidentally handling and unexpected exception the wrong way.

Additionally, you are not doing anything with the exception. In general, this is a bad practice. Exceptions are thrown for a reason and should not be ignored without some thought. If you know you don't care what the exception is cause by, add a comment to explain your reasoning to anyone who might see the code later.

\$\endgroup\$
  • 1
    \$\begingroup\$ I used 283 because the window is not resizable and the borders seem to make the shape meet the edge best with that number, I started with different numbers and kept changing them until it seemed to line up more accurately. I like the idea of defining something like: private int rightBorder = 283, though. Did not know that about exceptions, thank you! \$\endgroup\$ – Jeremy Johnson Aug 12 '13 at 9:35
5
\$\begingroup\$

You get a good review from @syb0rg. I'm going to tell you something that will help you in the long run. If you create any application in Swing in future try to use SwingUtilities.invokeLater() to prevent any unnatural behavior. So the code will be like this

public static void main(String[] args) 
{
    SwingUtilities.invokeLater(new Runnable() 
    {
        public void run() 
        {
            new Tester().go();
        }
    });
}

Now you will ask "Why should I use this?". Here is a good answer from StackOverflow.

\$\endgroup\$
  • 2
    \$\begingroup\$ I learn something new today, +1. \$\endgroup\$ – syb0rg Aug 30 '13 at 4:15
  • \$\begingroup\$ I imagine this is particularly useful in larger GUIs, yes? \$\endgroup\$ – Jeremy Johnson Aug 30 '13 at 12:40
  • \$\begingroup\$ @JeremyJohnson larger as well as for smaller cause you can't rely on your Swing app as all components are not thread safe. So why take chance? Take advantage of the API or you can implement you own thread safety from scratch, but I wouldn't go there. \$\endgroup\$ – Anirban Nag 'tintinmj' Aug 30 '13 at 12:54
  • \$\begingroup\$ @tintinmj I do usually use these animation GUIs in their own separate thread now when I'm working on a test that involves other operations, too. Especially if one of the other operations is sound. \$\endgroup\$ – Jeremy Johnson Aug 30 '13 at 13:03
4
\$\begingroup\$

Instead of having all the direction booleans, up, left, right and down, you can use velocityX and velocityY (which I will call xV and yV from now on). This will reduce the number of lines by quite a bit, because other than requiring only 2 variables rather than 4, you only have to check vertical and horizontal collision, and invert the velocity on impact.

In the beginning, set xV and yV to 1 and place the ball somewhere on the field:

if (x < 0 || x >= width) xV *= -1;

Same goes for the y values of course. Then, in the update method:

x += xV; y += yV;

When the ball hits the right wall, the horizontal speed will change to -1, which means x will decrease each time.

You can also change the starting values to 2 or 3 to make the ball go faster.

\$\endgroup\$
  • \$\begingroup\$ Also, Java's window's width is 6px smaller than its screen and the height 29px, so you were right about the 283 (if the height is indeed 256px). \$\endgroup\$ – Rick9399 Jul 29 '15 at 22:03
3
\$\begingroup\$

You should not sleep on the event dispatch thread (EDT) as this blocks Swing from processing events, including painting events.

You should use a javax.swing.Timer instead of a while loop:

Timer timer = new Timer(10, new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {
           // update variables
           // do not sleep
           // only need to repaint the panel, not entire frame
           drawPanel.repaint();
        }
      });
timer.start();

Other notes:

\$\endgroup\$

protected by Jamal Feb 20 '18 at 1:42

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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