Throw exception instead of return in Java method

Question

I'm writing Deque class on Java, according to Algorithms, Part 1 on Coursera. And currently my array-based Deque has method removeLast():

public Item removeLast() {
    if (size() == array.length / 4) {
        resize(array.length / 2);
    }

    if (head != tail) {
        Item tmp = array[--head];
        array[head] = null;
        return tmp;
    }
    throw new NoSuchElementException("Stack underflow");
}

If head == tail means Deque is empty and I throw exception, according to homework specification, at the end of method instead of return statement. This code gives direct intention about invariants (head != tail).

On the other hand method may be rewritten like this:

public Item removeLastRewritten() {
    if (size() == array.length / 4) {
        resize(array.length / 2);
    }

    if (head == tail) {
        throw new NoSuchElementException("Stack underflow");
    }

    Item tmp = array[--head];
    array[head] = null;
    return tmp;
}

In my opinion removeLast is more clearly written by these reasons:

1. Adhere to pessimistic scenario - always fail, only if ... which is more reliable approach, especially when method code will enlarge and become more complicated.
2. Gives more clear link between invariant tail != head and subsequent if {} code block.

I have the following questions:

1. Which is a better approach?
2. Is it considered an appropriate/good practice to write like removeLast?
3. What is considered a best practice for Java? Is there any code style about it (I couldn't find any)?

Answer 1

There is not a wrong answer. In GrepCode you can find every flavour you propose:

1. Inside a if with the != operator and at the end of the method:

E java.util.PriorityQueue.next()

public E More ...next() {
    if (... != ...)
        throw new ConcurrentModificationException();
    ...
    if (...) {
        return lastRetElt;
    }
    throw new NoSuchElementException();
}

2. Inside a if with the == operator

E org.fluentlenium.core.domain.FluentList.first()

public E More ...first() {
    if (this.size() == 0) {
        throw new NoSuchElementException("Element not found");
    }
    return this.get(0);
}

Answer 2

The reason why it looks weird is because you omit the else block of your if block, which would incorporate either leftover that comes after the if block in both your methods. The reason you can get away with it here is because the thrown exception will disrupt the flow of your method.

I think it's better not to rely on that and just be nice and use the if-else blocks intuitively.

public Item removeLastRewrittenAgain() {
    if (size() == array.length / 4) {
        resize(array.length / 2);
    }

    if (head != tail) { // invert the if-else block if preferred, it would not make any difference
        Item tmp = array[--head];
        array[head] = null;
        return tmp;
    } else {
        throw new NoSuchElementException("Stack underflow");
    }
}

Another reason why I don't really like to throw exceptions before the end of a method is that I strongly believe in and thus thoroughly use the concept of a single point of exit, meaning I don't leave the method somewhere in the middle which I believe is more difficult to read for someone not familiar with the code.

One place to return your value (or thrown exceptions): the very bottom of your method.

Answer 3

Your code becomes more readable if you explicitly mention the else, regardless of your choice.

Then there is the dilemma of

if (head != tail) {
    Item tmp = array[--head];
    array[head] = null;
    return tmp;
} else {
    throw new NoSuchElementException("Stack underflow");
}

versus

if (head == tail) {
    throw new NoSuchElementException("Stack underflow");
} else {
    Item tmp = array[--head];
    array[head] = null;
    return tmp;
}

Here I strongly prefer the second one. When I'm reading the "complicated part" of the if statement, I want to know why I'm actually inside an if. When reading the first variant the whole reason for the if only becomes apparent when you're throwing the exception.

I guess you could also solve that by writing

boolean isEmpty = head == tail;
if (!isEmpty) {
    Item tmp = array[--head];
    array[head] = null;
    return tmp;
}
throw new NoSuchElementException("Stack underflow");

but I prefer the version that throws the exception as soon as you know something is wrong.