CODEWITHSUNDEEP
javamultithreadingsynchronized
Daniel
Daniel

Reputation: 581

Use of the Synchronized Keyword in Java

I have read some articles (including Oracle's) on synchronized methods, and I'm not sure if I understand it correctly.

I have the following code:

public class Player extends javax.swing.JLabel implements Runnable{
    private static int off2[] = {-1, 0, 1, 0}, off1[] = {0, 1, 0, -1};
    private static final Map dir = new java.util.HashMap<Integer, Integer>();
    static{
        dir.put(KeyEvent.VK_UP, 0);
        dir.put(KeyEvent.VK_RIGHT, 1);
        dir.put(KeyEvent.VK_DOWN, 2);
        dir.put(KeyEvent.VK_LEFT, 3);
    }
    private boolean moving[] = new boolean[4];

    @Override
    public void run(){
        while(true){
            for(Object i : dir.values()){
                if(isPressed((Integer)i)) setLocation(getX() + off1[(Integer)i], getY() + off2[(Integer)i]);
            }
            try{
                Thread.sleep(10);
            }catch(java.lang.InterruptedException e){
                System.err.println("Interrupted Exception: " + e.getMessage());
            }
        }
    }
    public void start(){
        (new Thread(this)).start();
    }

    private synchronized boolean isPressed(Integer i){
        if(moving[i]) return true;
        else return false;
    }

    public synchronized void setPressed(KeyEvent evt) {
        if(dir.containsKey(evt.getKeyCode()))moving[(Integer)dir.get(evt.getKeyCode())] = true;
    }
    public synchronized void setReleased(KeyEvent evt){
        if(dir.containsKey(evt.getKeyCode()))moving[(Integer)dir.get(evt.getKeyCode())] = false;
    }
}

Right now all it does is movement. My understanding is that the setPressed and setReleased are called from the main thread, when my main form's key listener registers the keyPressed and Released events. Is this code reasonable? Is it a proper use of the synchronized keyword? (The code works without it, but I suspect it's better to have it?)

Upvotes: 3

Views: 735

Answers (2)

Stefan Haustein
Stefan Haustein

Reputation: 18793

The use of synchronized is correct, but I don't think it is necessary because in the current implementation, parallel access to the array will not cause inconsistencies.

However, I think there is a different threading issue in your code. You are not supposed interact with Swing (=call setLocation()) from a separate thread, see http://docs.oracle.com/javase/1.4.2/docs/api/javax/swing/SwingUtilities.html#invokeLater(java.lang.Runnable). So you need to wrap the update code into a Runnable:

private boolean moving[] = new boolean[4];
Runnable dirUpdate = new Runnable() {
  for(Object i : dir.values()){
    if(isPressed((Integer)i)) setLocation(getX() + off1[(Integer)i], getY() + off2[(Integer)i]);
  }
};

The invocation from the thread would then look like this:

SwingUtils.invokeLater(dirUpdate);

Note that in this case you won't need synchronized any longer because dirUpdate will be called from the event handling thread.

You may want to check for null in isPressed() to avoid exceptions on unmapped keys.

A simpler representation for the movement may be to have dx and dy variables which would be set to -1, 1 or 0 by the key events.

Upvotes: 1

Marco Massenzio
Marco Massenzio

Reputation: 3002

Not sure about the Swing side of things, but generally speaking you need the synchronized to protect those shared data (the moving[] in your case) that may be accessed by multiple threads.

In this case, you need to sync, as moving[] may be accessed on writing by either of the setXxx methods (main thread) or on read by the thread you started.

since Java 5, the facilities in the java.concurrent package are much better, and I would suggest you consider either using a Lock to protect moving[] or an AtomicBoolean.

A couple of "questions of style":

  1. don't use 'raw types' -- prefer Map<Integer, Integer> to Map (that also saves you from the unchecked -ugly- cast in the setXxxx() methods)

  2. avoid one-line if's -- make your code illegible :)

If you decide to use Lock (not a big advantage v. synchronized in this case) your isPressed() should look something like:

// ...
private Lock movingLock = new ReentrantLock();

private  boolean isPressed(Integer i){
  try {
    movingLock.acquire();
    return moving[i];
  } finally
    movingLock.release();
  }        
}

you would have to 'wrap' the assignments to moving[] in the setXxx methods with calls to Lock.acquire() and release()

Upvotes: 1

Related Questions