Lock/Unlock Implementation Review

[Saraswathy Krishnamoorth]

Please comment on my Lock and unlock implementation. I have a LockManager class that does locking and unlocking.
private HashMap locks;
public synchronized void lock(RemoteData client,int recordNum) {
if (locks!=null) {
recordNumber = new Integer(recordNum);
databaseLock=new Integer(LockManager.DATABASE_LOCK)
//Database Locking
if (recordNum==LockManager.DATABASE_LOCK) {
while(!locks.isEmpty()) {
wait();
}
//Record Locking
while (locks.containsKey(databaseLock) || locks.containsKey(recordNumber) ) {
wait();
}
locks.put(recordNumber,client);
}
public synchronized void unlock(RemoteData client,int recordNum) {
if (locks!=null && locks.size()>0) {
recordNumber = new Integer(recordNum);
if (locks.containsKey(recordNumber) ) {
RemoteData owner=(RemoteData)locks.get(recordNumber)
if (owner==client) {
locks.remove(recordNumber);
notifyAll();
}
}

[John Smith]

private HashMap locks;

How about private final HashMap locks, for clarity?

public synchronized void lock(RemoteData client,int recordNum) {

Since no operation is performed on client, except for adding it to the collection, a more formal declaration looks better (program to interface/superclass, not to implementation):
public synchronized void lock(Object client,int recordNum)

if (locks!=null) {

The collection object should never be null, so there is no reason to check for it every time. Instantiate it in the constructor.

databaseLock=new Integer(LockManager.DATABASE_LOCK)

Your databaseLock integer is constant, and therefore should be declared as private static final, and created just once.

if (locks!=null && locks.size()>0) {

No reason to check the size, either.

if (locks.containsKey(recordNumber) ) {
RemoteData owner=(RemoteData)locks.get(recordNumber)
if (owner==client) {

How about 1 line instead of 3:
if (locks.get(recordNumber) == client)
Eugene.
[ April 01, 2003: Message edited by: Eugene Kononov ]

[Saraswathy Krishnamoorth]

Thanks a lot Eugene. I will make the neccessary corrections. Just to Clarify..
In my RemoteDataFactoryImpl constructor I instantiate final (Data and LockManger). The getConnection method of RemoteDataFactoryImpl returns a unique RemoteData object. This RemoteData has instance variables Data and LockManger. These instance member variables have a reference to the Data and LockMangaer created in the RemoteDataFactoryImpl. Apart from the corrections you mentioned, is the rest of lock and Unlock implementation correct if I have the above Scenario.
[ April 02, 2003: Message edited by: Saraswathy Krishnamoorth ]

[John Smith]

In my RemoteDataFactoryImpl constructor I instantiate final (Data and LockManger). The getConnection method of RemoteDataFactoryImpl returns a unique RemoteData object. This RemoteData has instance variables Data and LockManger. These instance member variables have a reference to the Data and LockMangaer created in the RemoteDataFactoryImpl. Apart from the corrections you mentioned, is the rest of lock and Unlock implementation correct if I have the above Scenario.

Perfect!
Eugene.

[Saraswathy Krishnamoorth]

Thank you very much Eugene.