please critique the code

[Arun Bommannavar]

   
Class A {

   ClassB classB = new ClassB();

   String str = "";

   public void doSometthing(){

     try{
        str = classB.getVal();
       }catch(InterruptedException e){

        }
   }

}




ClassB implements PropertyChangeListener {

    boolean valid = false;

    public synchronized String getVal() throw InterruptedException{

    while(!valid) {
	      try{
                   wait();
                  }catch(InterruptedException e) {

            }
    }
    notifyAll();
    return val;
  }

    synchronized public void setValid(boolean b) throws InterruptedException {

        valid = b;
        notifyAll();
    }

 public void propertyChange(PropertyChangeEvent evt) {

         val = "get new value from event";
              try{
                 setValid(true);
              }catch(InterruptedException e){

            }
      }
 }


 

What is/are the mistake/s in this code?

In summary, the goal is classA.doSomething() should get the val from classB and should wait until a new value for "val" is set by the propertychange listener. This value is sent by a server running in the same subnet. How best can I achieve the goal? Any comments/suggestions?

[Steve Luke]

What kind of critique do you want?

Simple things like poor variable and class names, and bad indenting?

Heinous things like uncommented catch blocks with no content, effectively hiding the fact that the set/get failed?

Automatic things that can be addressed by the compiler, since the code you show is uncompilable?


If you just want critique on the strategy we need to know a bit more about the use-case. Can the value to be returned ever be invalid once a valid value has been retrieved? Or is the local - cached value good until a new value comes along?

[Arun Bommannavar]

Steve Luke wrote:What kind of critique do you want?

If you just want critique on the strategy we need to know a bit more about the use-case. Can the value to be returned ever be invalid once a valid value has been retrieved? Or is the local - cached value good until a new value comes along?

Code snippet was meant to illustrate logic dilemna.

Here is what I wish to do. ClassB implements PropertyChangelistener. ClassA wants to get the value of the variable "val" from ClassB via getVal(). It will have to wait until the server sends a new value (that part of code is too big to post) and the listener triggers and set a new value for the variable "val" in ClassB. At this point, getVal() in ClassB should get out of wait status and provide the value to the caller whcih is classA.doSomething().

So, the question how best can I implement the solution.

Regards

[luri ron]

1. i don't see anything have to do with litener here as there is no observee here to manage the listeners

2. if you intend to implement multiple listeners subscribe a event, then you will have a Observee that maintain a list of listener (use ConcurrentSkipListSet or CopyOnWriteArrayList). the Observee will simply do a someEvent.wait (), and the notifier will simply need to do a someEvent.notify.

my 2 cents

[Steve Luke]

That is pretty much how you described it before. Let's approach it a different way.

1) Why do you think there needs to be a 'valid' flag?

2) What should happen if:
- a) Server updates the property
- b) Client calls doSomething()
- c) Client calls doSomething()
- d) Server updates the property
- e) Server updates the property

Possible scenarios:
i) When b) happens it returns the value set by a). When c) happens it returns the value set by a).
ii) When b) happens, it should return the value of a). When c) happens it should wait until d) happens since the value from a) was already retrieved, then return the value of d).
iii) When b) happens it should wait for d), because any value set prior to b) is not important. When c) happens it should also wait and get the value returned by d) since it was called before d)
iv) When b) happens it should wait for d), because any value set prior to b) in not important. When c) happens it should wait for e), since the value of d) was taken by the Client in step b).

There are probably others...


3) Are local caches safe to be used any time after they are set, until a new value is set. Or must a get always wait for the next value even if a value is already waiting?
4) Can multiple calls to doSomething get the same value, or must individual calls to getValue return subsequent values?
5) What does the client (the one 'getting the value') do? Is it just processing the value? Does it have anything to do other than work on the value presented from the server? Is this a classic producer/consumer situation where the server is the producer, 'ClassA' is the consumer, and ClassB's only role is a mediator between them?

It is really impossible to talk about the logic if we don't know how it is used. It might be right, near right, or completely off. Please provide a more real-world explanation than "The server updates ClassB and ClassA must wait for a new value".

[Arun Bommannavar]

Steve Luke wrote:
It is really impossible to talk about the logic if we don't know how it is used. It might be right, near right, or completely off. Please provide a more real-world explanation than "The server updates ClassB and ClassA must wait for a new value".

The scenario are as follows:

Server : Monitors water level measuring devices and controls motors associated with those devices.. At any given time, only some values change, , i.e. few water levels change and hence the server makes adjustments to corresponding motor positions. After it respositions the motors it sends event flag(lets name it CHANGE) to the Clients. After the event flag is sent, it then sends values of water levels and motor positions of only those that have changed.
There can be any number of clients, typically 2-5.

Client: Needs to Plot the values of water and motor positions Vs. Time. Client does not query server for values.

So here I have the following: For example if there are 3 water level monitor devices and 3 Motors, I'll have three instances of Motor class and three instances of WaterLevel class and one plot class.

In addition there is a supervisor class that recieves notifications from server. All the motor class instances, water level class instances and Plot class have registered themselves with supervisor class. They implement PropertyChangeListener.

If the server sends motor 1 value change, then supervisor gets it, figures out that it is motor 1 value and executes firePropertyChange. Since, Motor class has implemented PropertyChangeListener, the method propertyChange(PropertyChangeEvent evt) gets executed. Similarly for water level.

When the server sends a CHANGE event flag, supervisor executes firePropertyChange.When the plot class recieves CHANGE event flag, it queries all threee instances of Motor and WaterLevel class. At this point it does not know which have changed. So it will only get the latest value for motor positions and water levels. If the value didnt change, it takes the current stored value. Since, the sequence followed by the Server, is first send the values for water level and motor positions then only send the CHANGE event flag and , I'd assume those values are current. The plot class does not attempt to read values from Motor and Water level class untill CHANGE event flag has arrived.

Coming back to the code snippet. ClassA is Plot class. ClassB is motor class. Left out Water Level class.

The reason for valid flag: For the first time when the client starts the program, if all the Motor positions and water levels values are not read, then without a valid flag I presume incorrect values would be read by Plot class. So it will have to wait unitl the values are updated and valid flag is set to true. This is the part of the code that is not working correctly.

Sometimes, when Plot class tries to read Motor value and the code gets stuck in "while(!valid)" loop because first time value hasnt been read yet. Even when the value is read and it sets valid = true, the code is still stuck in "while(!valid)" loop. This I found out by print statements. I tried declaring valid as volatile but it doesnt help. In the following code, I changed wait() to wait(20) and then printed the value for valid, it still printed false. So I am not sure why it gets stuck in the while(!valid) loop.

   
 public synchronized String getVal() throw InterruptedException{  
   
     while(!valid) {  
           try{  
                    wait();  
                   }catch(InterruptedException e) {  
   
             }  
     }  
     notifyAll();  
     return val;  
   }

 

[Steve Luke]

Okay, I don't think I like that approach. In your scenario, one update from the server tells the monitor to make two events -> one tells the plot things have changed and one tells the instrument it has changed. Then the monitor triggers several events in the Plot class to get the value from the various instruments so it can determine what changed.

Let's say the server tells the monitor Motor # 1 has changed. Then this is the flow of your program:

                       -------------
                      |    Server   |
                       -------------
                             |
                             |
                             V
                       -------------
                      |    Monitor  |
                       -------------
                       /            \
                      /              \
                     \/              \/
            -----------             -----------            ----------
           |  Motor 1  |  <------  |  Plot     |  ------> | Sensor 2 |
            -----------             -----------            ----------
                                  /         |     
                                 /          |     
                                \/          V      
                       ----------        -----------  
                      | Motor 2  |      | Sensor 1  |
                       ----------        -----------

All so that Plot can get a single change to Motor 1. The way I see it, the motors and the water sensors are models, and the plot is the view. We can take a page out of the swing book, and make the model tell the view when it changed, instead of making the view ask when the model changed. In this case, the Plot is registered with the Motors and the Sensors. The Monitor tells the Motor it changes, and the Motor tells the Plot it changed. It would look like this:

                       -------------
                      |    Server   |
                       -------------
                             |
                             |
                             V
                       -------------
                      |    Monitor  |
                       -------------
                       /            
                      /              
                     \/              
            -----------             -----------
           |  Motor 1  |   ------> |  Plot     |
            -----------             -----------

One event from the server yields one event from the monitor which yields one event from the particular instrument which changed. Now, how Plot gets the values of other instruments is unimportant, it might query them, or it might hold a local cache, or maybe it doesn't need to do anything...

But specifically to your need for checking 'isValid', this really only needs to happen if you don't have a good 'default' value. If the value hasn't been set yet, provide some default value (a good default value would be one that is obvious it is pre-reading. For numbers this is usually 0 in Java, but sometimes people use -1.)

The code for Plot might be something like this:

public class Plot implements InstrumentValueChangeListener
{
    private final Map<String, Integer> instrumentValues = new HashMap<String, Integer>();

    /* Add an instrument by name, it gets added to the map and the current value is set to some default */
    public void addInstrument(String name)
    {
        instrumentValues.put(name, 0); //add a default value until it gets updated
    }

    /* gets called by an instrument when its value is changed */
    @Override public void instrumentValueChanged(String instrumentName, int newValue)
    {
        instrumentValues.put(instrumentName, newValue);
        plotNewData(instrumentName, newValue);
    }

    /* would plot a single point to a single graph */
    private void plotNewData(String graph, int value)
    {
        //whatever to get new data point on graph
    }

    /* If you need to plot all current data, then go through the map for all the instruments */
    private void plotAllCurrentData()
    {
        for (Map.Entry<String, Integer> instrumentData : instrumentValues.entrySet())
        {
            //plot value for this instrument
        }
    }
}

A Motor might look like this:

public class Motor implement PropertyChangeListener
{
    private final InstrumentValueChangeListener listener;
    private final String name;
    private Integer value;

    public void propertyChange(PropertyChangeEvent pce)
    {
        this.value = (Integer)pce.getNewValue();
        listener.instrumentValueChanged(this.name, this.value);
    }
    ...

And whatever code makes these things might look like:

        Plot plot = new Plot();
        ...
        Motor m = new Motor();
        m.listener = plot;
        plot.addInstrument(m.getName());
        ...

Now, this really has gone a long way away from what your real problem actually is. Your real problem seems to come from this:

Arun Bommannavar wrote:Sometimes, when Plot class tries to read Motor value and the code gets stuck in "while(!valid)" loop because first time value hasnt been read yet. Even when the value is read and it sets valid = true, the code is still stuck in "while(!valid)" loop. This I found out by print statements. I tried declaring valid as volatile but it doesnt help. In the following code, I changed wait() to wait(20) and then printed the value for valid, it still printed false. So I am not sure why it gets stuck in the while(!valid) loop.

My feeling for this is:
1) You are using an older version of the JVM (pre-Java 1.5) in which volatile was broken. So the value of valid may never reach from one thread to the other. Though this seems doubtful since you are synchronizing which should publish the value anyway

2) You have different instances of the object. You are calling setValid() on one instance and checking isValid on another. The one whose valid flag is being checked never changes because you never set it. For example, you have 3 motors and 3 sensors. Motor 1 gets updated. Plot checks motor 1, gets the value because its valid flag was set. It then checks Motor 2, but has to wait, because Motor 2 has not been updated. And when Motor 2 never gets updated, then Plot never gets out of the getVal() method.

Without a compilable example I can't be sure though.

[Arun Bommannavar]

Steve,

Thank you very much. I'll implement my code as per your suggestion. On the second point regarding version of JVM, I am using JBuilder2006 and it comes bundled in with 1.5.0_03b07. Most likely, I will get myself on track with using Eclipse and this way I would be able to use the latest JDK and not get stuck with what comes bundled in with IDE.

Again, I appreciate your help very much.