Your code works, but its quality leaves some room for improvement. It should be simplified by refactoring.
Here are some suggestions:
Naming
Even if it is only an exercise, utmost care should be applied when choosing names to make sure they are descriptive and clear, but also to comply with coding conventions:
- Consider calling your class
NonReentrantLock
instead of MyLock
.
- Methods should be camel-case, starting with a lower-case character, so
Lock()
should be lock()
and Unlock()
should be unlock()
.
- Only constants should be upper-case, so
TAG
should be made a constant. But TAG
can mean anything. The solution is to refactor your code (as shown below) such that TAG
is only used once and is no longer needed as a class variable.
Constants
TAG
is not a useful constant because it leads to lots of System.out.println
calls which you want to avoid. All your logging should happen in a single place, not be spread out over your class.
However, you are using the magic value of -1
to show that nobody owns the lock at the moment. You should introduce a constant NOBODY
and use that to set owner
:
private static final long NOBODY = -1;
private long owner = NOBODY;
Redundancy
Tracking the same state twice
Look carefully at your instance variables owner
and isLocked
. Track their values during runtime with the debugger, and you will see: if owner == -1
then isLocked == false
, but if owner != -1 then isLocked == true. In other words, you are using two variable to track the same piece of state.
You should remove isLocked
and replace it it with the following method:
private synchronized boolean isLocked() {
return owner != NOBODY;
}
Doing the same string concatenations multiple times
You have used System.out.println
throughout your code, always passing TAG
, a message, and a thread id. You can extract this functionality into a new method:
private void log(String message, long threadId) {
System.out.println(String.format("MyLock: %s %s", message, threadId));
}
That way, System.out.println
is only called in a single place and can easily be disabled or replaced with a real logger. For instance, this:
System.out.println(TAG+"Lock Acquired: Owner = " + owner);
can become this:
log("lock acquired, owner =", owner);
Miscellaneous redundancies
If you throw an exception, inside an if-structure, you the else
branch is redundant, so code like
if (condition) {
throw new IllegalStateException(...);
} else {
doSomethingElse();
}
can become
if (condition) {
throw new IllegalStateException(...);
}
doSomethingElse();
You have code duplication when checking if the current thread is the owner of the lock. Refactor out this method instead:
private boolean ownerIsCurrentThread() {
return owner == Thread.currentThread().getId();
}
You should extract argument validation into a separate method:
private void throwIf(boolean condition, String message) throws IllegalStateException {
if (condition) {
throw new IllegalStateException(message);
}
}
which will allow you to replace your argument validation if-statements with code like
throwIf(ownerIsCurrentThread(), "the lock has already been acqired");
Error messages
At the beginning of unlock()
, you throw an IllegalStateException
with the same message for two completely different reasons. That's not very helpful for debugging. Instead, check them separately:
public synchronized void unlock() throws IllegalStateException {
throwIf(!isLocked(), "only a locked lock can be unlocked");
throwIf(!ownerIsCurrentThread(), "only owner can unlock the lock");
End Result
public class NonReentrantLock {
private static final long NOBODY = -1;
private long owner = NOBODY;
public synchronized void lock() throws InterruptedException, IllegalStateException {
throwIf(ownerIsCurrentThread(), "the lock has already been acqired");
long threadId = Thread.currentThread().getId();
while (isLocked()) {
log("waiting for lock on thread", threadId);
wait();
}
owner = threadId;
log("lock acquired, owner =", owner);
}
public synchronized void unlock() throws IllegalStateException {
throwIf(!isLocked(), "only a locked lock can be unlocked");
throwIf(!ownerIsCurrentThread(), "only owner can unlock the lock");
log("unlocking, owner =", owner);
owner = NOBODY;
notify();
}
private synchronized boolean isLocked() {
return owner != NOBODY;
}
private boolean ownerIsCurrentThread() {
return owner == Thread.currentThread().getId();
}
private void throwIf(boolean condition, String message) throws IllegalStateException {
if (condition) {
throw new IllegalStateException(message);
}
}
private void log(String message, long threadId) {
System.out.println(String.format("MyLock: %s %s", message, threadId));
}
}
wait()
action lock thewhile(..)
, you have to give wait[Time](inMilliseconds). \$\endgroup\$