Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here's part of code in Java for battleships game I'm working on:

    lblGrid2 = new JLabel(new ImageIcon(myPicture));
    lblGrid2.addMouseListener(new MouseAdapter() {
        @Override
        public void mouseClicked(MouseEvent e) {
            for (int i = 0; i < shipList.size(); i++)
                if (shipList.get(i).isActive())
                    shipList.get(i).place();
        }
    });
    lblGrid2.addMouseMotionListener(new MouseMotionAdapter() {

        @Override
        public void mouseMoved(MouseEvent m) {

            for (int i = 0; i < shipList.size(); i++) {
                if (shipList.get(i).isActive()) {
                    shipList.get(i).moveShip(m.getX(), m.getY());
                }
            }
        }
    });
    lblGrid2.setText("");
    lblGrid2.setBounds(40, 40, 401, 401);
    panel.add(lblGrid2);

When placing ships on board. this listens for mouse movement and should move ship image accordingly, but that happens only after the client and server are connected and there is an active ship that can be placed. If that's not the case, for example I haven't connected to the server yet, every mouse move throws an exception.

Is this something I should be concerned about?

share|improve this question
    
My rule of thumb is: If the programmer has an attached debugger which breaks when an exception is thrown, that programmer shouldn't be annoyed by the breaks. Debugger breaks on mouse move are clearly annoying. –  CodesInChaos May 24 at 17:19
add comment

1 Answer 1

up vote 11 down vote accepted

Yes, absolutely, you should be concerned.

Exceptions should be exactly that, exceptional. and should never be part of the regular code path. If there are circumstances that you know would be normal (like not yet being connected to a server) then you should code in a condition that represents that situation, and make your code a conditional-execution path. For example, in your code, I would have an AtomicBoolean that represents the 'connected' state, and then only process mouse-events when connected.

As a side-note, Exceptions in Java are very 'expensive'. Almost all Java VM's (OpenJDK, Oracle JDK, IBM JDK, Android JDK, etc.) need to inspect the entire call-graph of the code when an exception happens, so that it can build up the stack trace for the exception. In all the VM's I know, that requires that:

  • The entire VM thread is 'suspended' so that the call-graph does not change
  • all threads are 'paused' <- looking for evidence of this still
  • the call stack is inspected and walked up.
  • all references on the call-stack are chased back in to the source (get line numbers, etc.)

This is very expensive to process. Using exceptions as part of the normal program flow is a bug, and preventing exceptions with conditional statements is always a good idea. For example, the following code example is terrible:

int val = 0;
boolean isNumber = false;
try {
   val = Integer.parseInt(somestring);
   isNumber = true;
} catch (NumberFormatException nfe) {
   // ignore not-numbers, and leave isNumber to be false
}

I see code like the above all over the place.... and it is broken. In one example, I have seen a 50% performance improvement on a CSV reading program by changing code like that to be:

int val = 0;
boolean isNumber = false;
if (someString.matches("\\d+")) {
    try {
       val = Integer.parseInt(somestring);
       isNumber = true;
    } catch (NumberFormatException nfe) {
       // ignore not-numbers, and leave isNumber to be false
    }
}

Even though the code does extra work to pre-validate that the string contains digits, it is faster! (and is even faster if you precompile the Match pattern...!)

So, in your code, I would have:

private final AtomicBoolean isConnected = new AtomicBoolean(false);

and then, in your code:

lblGrid2.addMouseMotionListener(new MouseMotionAdapter() {

    @Override
    public void mouseMoved(MouseEvent m) {

        if (!isConnected.get()) {
            return;
        }

        for (int i = 0; i < shipList.size(); i++) {
            if (shipList.get(i).isActive()) {
                shipList.get(i).moveShip(m.getX(), m.getY());
            }
        }
    }
});

Some discussions about Java Stack traces and exceptions:

share|improve this answer
    
Why is it necessary to suspend the whole VM including other threads? Wouldn't walking the the stack of the current thread be enough? –  CodesInChaos May 24 at 16:58
    
You may be right @CodesInChaos . I am looking for references, but came across this in the meantime –  rolfl May 24 at 17:13
1  
Erhm... I have to admit that I tend to catch that NumberFormatException. I promise, I'll stop that. –  Simon André Forsberg May 24 at 19:10
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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