Java Non Reentrant Lock Implementation

I have implemented a Non Reentrant Lock. I want to know if this has any mistakes, race conditions etc. I am aware of the fact that existing libraries have to be used (instead of writing our own), but this is just to see if I am understanding the java concurrency correctly. Any feedback is appreciated.

public class MyLock {
private boolean isLocked = false;
private long owner = -1;
private static String TAG = "MyLock: ";

public synchronized void Lock() throws InterruptedException, IllegalStateException {
if(owner == Thread.currentThread().getId()) {
    throw new IllegalStateException("Lock already acquired. " +
                                    "This lock is not reentrant");
} else {
    while(isLocked == true) {
        System.out.println(TAG+"Waiting for Lock, Tid = " +
                Thread.currentThread().getId());
        wait();
    }
}

isLocked = true;
owner = Thread.currentThread().getId();           
System.out.println(TAG+"Lock Acquired: Owner = " + owner);
}

public synchronized void Unlock() throws IllegalStateException {
if(!isLocked || owner != Thread.currentThread().getId()) {
    throw new IllegalStateException("Only Owner can Unlock the lock");
} else {
    System.out.println(TAG+"Unlocking: Owner = " + owner);
    owner = -1;
    isLocked = false;
    notify();
}
}

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(Str

StackExchange Code Review Q#17913, answer score: 5