0

I have a particle simulation project that I've been working on for the last many hours, it is two classes which I will post. One is the Particle class and one is the main and Canvas class. I create a canvas then get its BufferStrategy and a Graphics to draw on it. I use an update loop to update particles each frame and a render loop to render the particles each frame. Updating and rendering are both done by calling the self-render and self-update method of each Particle in the particles arraylist. Now here's my question. I have a MouseListener that clears all particles on middle click, but this creates a NullPointException because the particles ArrayList is emptied while the update method is iterating through it.

I solved this by simply surrounding the code inside the Particle's update method with a try catch with an empty catch, because there is nothing necessary to do when the exception occurs - all the particles are gone so finishing the update doesn't matter. However, I've read that that is bad form. is there a better solution?

I can't figure out how to make a code segment bold. The try catch is near the end of the Particle class.

Particle class:

import java.awt.Graphics;
import java.util.ArrayList;

public class Particle {
    static int G = 1; //gravity constant
    double xPos;
    double yPos;
    double xVel;
    double yVel;
    int radius;
    static int particleCount = 0;
    double mass;


    public Particle(int xp, int yp
            ,double xv,double yv, int r){
        xPos=xp;
        yPos=yp;
        xVel=xv;
        yVel=yv;
        radius=r;
        mass = Math.PI*Math.pow(radius,2);
        particleCount++;
    }

    void drawParticle(Graphics g){
        g.fillOval((int)Math.round(xPos), (int)Math.round(yPos), 2*radius, 2*radius);
    }
    void updateParticle(int thisParticleIndex, ArrayList<Particle> list){
        //update position
        xPos+=xVel;
        yPos+=yVel;

        //update velocity
        //F = G*m1*m2 / r^2
        double M; //let M = m1*m2
        double r;
        double Fx=0;
        double Fy=0;
        double dF;
        double dFx;
        double dFy;
        double theta;

        Particle p;
        try {
            for(int i=0; i<list.size();i++){
                if(i!=thisParticleIndex){
                    p = list.get(i);
                    r = Math.sqrt(Math.pow((p.xPos+p.radius) - (xPos + radius), 2) + 
                            Math.pow((p.yPos+p.radius) - (yPos + radius), 2));
                    if(r<5)
                        continue;
                    M = mass + p.mass;
                    dF = G*M/Math.pow(r,2);
                    theta = Math.atan2((p.yPos+p.radius) - (yPos + radius),
                            (p.xPos+p.radius) - (xPos + radius));
                    dFx = dF*Math.cos(theta);
                    dFy = dF*Math.sin(theta);
                    Fx += dFx;
                    Fy += dFy;
                }
            }
        } catch (NullPointerException e) {
            //This try catch is needed for when all particles are cleared
        }

        xVel += Fx/mass;
        yVel += Fy/mass;
    }
}

Canvas class:

public class MainAR extends Canvas implements Runnable {
    private static int width = 600;
    private static int height = 600;

    private Thread gameThread;
    private JFrame frame;
    private boolean running = false;

    private ArrayList<Particle> particles = new ArrayList<>();
    private int WIDTH = 800;
    private int HEIGHT = 800;
    private int mouseX = 0;
    private int mouseY = 0;
    private int radius=15;
    private boolean drawMouse = true;
    private boolean mouseDown = false;
    private JLabel instructions;

        public MainAR() {
                setSize(width,height);
                frame = new JFrame();
                frame.setTitle("Particle Simulator");
                frame.add(this);
                frame.pack();
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.setVisible(true);
                start();

                Particle a = new Particle((int)(0.3*WIDTH),(int)(0.3*HEIGHT),0,0,15);
                Particle b = new Particle((int)(0.3*WIDTH),(int)(0.6*HEIGHT),0,0,20);
                Particle c = new Particle((int)(0.6*WIDTH),(int)(0.3*HEIGHT),0,0,10);
                Particle d = new Particle((int)(0.6*WIDTH),(int)(0.6*HEIGHT),0,0,25);

                particles.add(a);
                particles.add(b);
                particles.add(c);
                particles.add(d);

                addMouseMotionListener(new MouseMotionListener(){
                    public void mouseDragged(MouseEvent e) {
                        mouseX = e.getX();
                        mouseY = e.getY();
                        if(SwingUtilities.isLeftMouseButton(e))
                            mouseDown = true;
                    }

                    public void mouseMoved(MouseEvent e) {
                        mouseX = e.getX();
                        mouseY = e.getY();
                    }
                });
                addMouseWheelListener(new MouseWheelListener(){
                    public void mouseWheelMoved(MouseWheelEvent e) {
                        radius -= e.getWheelRotation();
                        if(radius<1)
                            radius = 1;
                    }
                });
                addMouseListener(new MouseListener(){
                    public void mouseClicked(MouseEvent e) {
                        if(SwingUtilities.isLeftMouseButton(e))
                            particles.add(new Particle((int)(mouseX-radius),(int)(mouseY-radius),0,0,radius));
                        if(SwingUtilities.isRightMouseButton(e))
                            instructions.setVisible(false);
                    }
                    public void mouseEntered(MouseEvent e) {
                        drawMouse = true;
                        mouseX = e.getX();
                        mouseY = e.getY();
                    }
                    public void mouseExited(MouseEvent e) {
                        drawMouse = false;
                    }
                    public void mousePressed(MouseEvent e) {
                        if(SwingUtilities.isRightMouseButton(e))
                            instructions.setVisible(false);
                        if(SwingUtilities.isMiddleMouseButton(e)){
                            Particle.particleCount = 0;
                            particles.clear();
                        }

                    }
                    public void mouseReleased(MouseEvent e) {
                        mouseDown = false;
                    }
                });
        }

        public synchronized void start() {
                running = true;
                gameThread = new Thread(this, "Display");
                gameThread.start();
        }

        public synchronized void stop() {
                running = false;
                try {
                        gameThread.join();
                } catch (InterruptedException e) {
                        e.printStackTrace();
                }
        }

        public void run() {
                while(running) {
                    try {
                        Thread.sleep(2);
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                        update();
                        render();
                }
        }

        private void update() {
            if(mouseDown)
                particles.add(new Particle((int)(mouseX-radius),(int)(mouseY-radius),0,0,radius));
            for(int i=0; i<particles.size();i++)
                particles.get(i).updateParticle(i,particles);
            frame.setTitle("Particle Simulator" + particles.size() + "particles");
        }

        private void render() {
                BufferStrategy bs = getBufferStrategy();
                if (bs == null){
                        createBufferStrategy(3);
                        return;
                }

                Graphics g = bs.getDrawGraphics();
                ((Graphics2D)g).setRenderingHint(RenderingHints.KEY_ANTIALIASING,RenderingHints.VALUE_ANTIALIAS_ON);
                g.setColor(Color.WHITE);
                g.fillRect(0, 0, getWidth(), getHeight());
                g.setColor(Color.BLACK);
                g.fillOval((int)(mouseX-radius), (int)(mouseY-radius), 2*radius, 2*radius);
                for(Particle p : (ArrayList<Particle>)particles.clone()) //cloning prevents Concurrent Modification Exception error
                    p.drawParticle(g);
                g.dispose();
                bs.show();
        }

        public static void main(String[] args){
        MainAR m = new MainAR();

        }
}

PS- I have another quick secondary question. Is it bad practice to be using non-private fields in my Particles class? For example should I instead of this

if(SwingUtilities.isMiddleMouseButton(e)){
                            Particle.particleCount = 0;
                            particles.clear();
                        }

have used a static getter and setter method to access a private static int particleCount within Particle?

Tagir Valeev
  • 97,161
  • 19
  • 222
  • 334
pyjamas
  • 4,608
  • 5
  • 38
  • 70
  • 2
    Have you considered a test for null? Have you considered just catching the NullPointerException? Have you considered fixing the bug that gets you into the state where something is null? – user207421 Sep 05 '15 at 10:01
  • It _can_ be OK to catch subclasss of `RuntimeException`, but rarely, and definitely not with `NullPointerException`. See [this](http://stackoverflow.com/questions/6115896/java-checked-vs-unchecked-exception-explanation) question. – bcsb1001 Sep 05 '15 at 10:05
  • Well testing for null wouldn't work because it could get deleted right after the test is finished right? I'm pretty new to threads. The issue is I think that since the run thread and GUI thread are running at the same time, the clear particles line can be called in the middle of an update. I don't know how to solve this other than letter the exception happen an ignoring it. My question is whether that's bad – pyjamas Sep 05 '15 at 10:05
  • 1
    You're accessing a list and its elements from multiple threads, without any kind of synchronozation. That can cause a NullPointerException, or corrupt the list, or go into an infinite loop, or really anything. You need to correctly synchronize all accesses to the shared data. – JB Nizet Sep 05 '15 at 10:06
  • Things are bad practice if and only if they make life harder for you or someone else. So does it make life harder for you or someone else? Well, not yet, but it will later when you accidentally use a null pointer inside the try block and then get confused about why your code doesn't do anything. – user253751 Sep 05 '15 at 10:07

2 Answers2

2

Is using an empty catch block bad practice in this situation?

Yes. Using empty catch block is quite bad practice in almost every situation. It means you are trying to hide something wrong. You are not solving the problem but just hiding it. If flow of your program demands to have empty catch block than you must think twice before applying it and according to me there must be something wrong with the flow or requirements on which you are working.

an empty catch, because there is nothing necessary to do when the exception occurs

No. You must take any action when you face any Exception, meaning of Exception is there is something wrong going on in your code.

For example you are catching NullPointerException and without doing anything you are just moving ahead. Consider following example,

try {
  checkAeroplane();
} catch(TechnicalProblemException e) {
  //No action needed
}

flyAeroplane();//Crash!!

In multi threading environment, you may face the exception if multiple threads are manipulating your list you should use thread safe alternative of ArrayList CopyOnWriteArrayList and apart from that you should use synchronized to stop manipulating your logic at the same time by multiple threads.

akash
  • 22,664
  • 11
  • 59
  • 87
  • 1
    Your simple solution is not adequate. The list is modified from another thread, so, for example, the particle could be non-null when evaluating ìf (particle != null)`, but become null right after. – JB Nizet Sep 05 '15 at 10:15
  • 2
    Okay thank you I will learn more about synchronization and then finish this project – pyjamas Sep 05 '15 at 10:27
1

Short answer : yeah, this is terribly bad. NPE should never be caught. Fix your code, don't ignore it's broken. As for your Particle.particleCount, it is bad too because you can set it to any value independently of the list containing the particles. Therefore, it might lead (and it does, in your case) to inconsistency between Particle.particleCount and the number of items in the list.

Dici
  • 25,226
  • 7
  • 41
  • 82