Lock Method Issue

[Daniel Simpson]

I have the following lock method, but I cannot compile it because it says it must return a value long. Here is the code:

public synchronized long lock(int recNo) throws RecordNotFoundException {
        try {
            if (isRecordValid(recNo)) {
                while (lockCookieHash.containsKey(new Long(recNo))) {
                    try {
                        wait();          
                    } catch (InterruptedException inEx) {
                    }
                }
                
                /* Performs record validity again because record 
                 * state could have changed while the thread was 
                 * waiting for the lock
                 */
                if (isRecordValid(recNo)) {
                    Long cookie = new Long(recNo);
        
                    /* The key-value pair are both the record number.
                     * This keeps it easy to maintain.  Consult the
                     * choices.txt for more information on this choice
                     */
            
                    // put lock cookie into hashmap
                    lockCookieHash.put(cookie, cookie);
            
                    return cookie.longValue();
                }
            }
         } catch (RecordNotFoundException rnfEx) {
             throw new RecordNotFoundException(rnfEx.getMessage());
         }
    }

It calls the method isRecordValid to test and see if the record in the cache is valid. If it is, it will return true, otherwise it will throw a RecordNotFoundException if the record is deleted or doesn't exist. However, the compiler says it must return a value....how can I fix this problem, because theoretically this code could execute and be at a spot where it is expecting some value to return. Maybe it's just late and I cannot think straight. Thanks for helping.
[ January 07, 2005: Message edited by: Daniel Simpson ]

[Dieskun Koper]

Does it compile if you change your code as follows?

} catch (RecordNotFoundException rnfEx) {
throw new RecordNotFoundException(rnfEx.getMessage());
}
return 0L; // to keep the compiler happy
}

Some compilers expect a non-void method to end with a return statement and don't take into account that in certain circumstances that line can never be reached.

[Frans Janssen]

Hi Daniel,

It calls the method isRecordValid to test and see if the record in the cache is valid. If it is, it will return true, otherwise it will throw a RecordNotFoundException if the record is deleted or doesn't exist.

I am assuming that your isRecordValid() method throws the exception if the recNo is not a valid record number (which means that the method either returns true or throws an exception, but never ever returns false).

The compiler is considering the situation where isRecordValid() does return false without throwing an exception (it is not smart enough to tell that this can never happen). In that case the lock() method is exited without an explicit return statement and that is what the compiler does not accept.

Adding an extra return statement at the end as Dieskun suggested will solve your problem. You might want to put an assert false before this return statement to demonstrate that in practice the return statement is never used.

HTH,

Frans.

[Daniel Simpson]

Originally posted by Frans Janssen:
Hi Daniel,



I am assuming that your isRecordValid() method throws the exception if the recNo is not a valid record number (which means that the method either returns true or throws an exception, but never ever returns false).

The compiler is considering the situation where isRecordValid() does return false without throwing an exception (it is not smart enough to tell that this can never happen). In that case the lock() method is exited without an explicit return statement and that is what the compiler does not accept.

Adding an extra return statement at the end as Dieskun suggested will solve your problem. You might want to put an assert false before this return statement to demonstrate that in practice the return statement is never used.

HTH,

Frans.

Thanks for the help, guys. I, too, was thinking about doing that solution, although I know returning an "error" code or a number that isn't the right number is bad software engineering. Maybe, I'll just add a comment above the line saying that this line will never execute and it only is used to satisfy the compiler. Thanks!

[Joe Borderi]

"Maybe, I'll just add a comment above the line saying that this line will never execute and it only is used to satisfy the compiler."

Maybe the return at the end of the function should never execute, but what happens if isRecordValid(recNo) returns false?

[Daniel Simpson]

Originally posted by J Borderi:
"Maybe, I'll just add a comment above the line saying that this line will never execute and it only is used to satisfy the compiler."

Maybe the return at the end of the function should never execute, but what happens if isRecordValid(recNo) returns false?

My method isRecordValid never returns false. If the record is valid, it will return true. Otherwise, it will just throw a RecordNotFoundException.

[Nikolay Petrov]

My method isRecordValid never returns false. If the record is valid, it will return true. Otherwise, it will just throw a RecordNotFoundException.

In that case do not declare it to return boolean

[Daniel Simpson]

Originally posted by Nikolay Petrov:


In that case do not declare it to return boolean

What would I change it to then? Because such methods as lock, update, delete, unlock rely on this isRecordValid to proceed.

[Frans Janssen]

Originally posted by Daniel Simpson:


What would I change it to then? Because such methods as lock, update, delete, unlock rely on this isRecordValid to proceed.

You could so something like:

private void checkRecordValid(int recNo) {
    if ([I]recNo is not a valid record number[/I])
        throw new RecordNotFoundException();
}

And your lock method would then be:

public synchronized long lock(int recNo) throws RecordNotFoundException {
    try {
        checkRecordValid(recNo);
        while (lockCookieHash.containsKey(new Long(
                recNo))) {
            try {
                wait();
            } catch (InterruptedException inEx) {}
        }
        /* Performs record validity again because record
         * state could have changed while the thread was
         * waiting for the lock
         */
        checkRecordValid(recNo));
        Long cookie = new Long(recNo);
 
        /* The key-value pair are both the record number.
         * This keeps it easy to maintain.  Consult the
         * choices.txt for more information on this choice
         */
 
        // put lock cookie into hashmap
        lockCookieHash.put(cookie, cookie);
        return cookie.longValue();
    } catch (RecordNotFoundException rnfEx) {
        throw new RecordNotFoundException(rnfEx.getMessage());
    }
}

The if-statements are eliminated and you don't need a dummy return statement.

Frans.