Lawrence Kesteloot argues that if-conditions should not enqueue work, mutate state, or hide control flow—keeping the predicate a pure question makes code easier to read, test, and refactor.

Keep if clauses side-effect free

Avoid writing if clauses that have side effects:

if (enqueueMessage(message)) {
    ...
}

The only function of an if statement is to test whether a condition is true. It’s not for executing code as a side-effect of the test. One problem with using the return value directly, as in the above, is that the meaning of the returned value is unclear. Does enqueueMessage() return true if the message was enqueued or true if the queue is full? Make it explicit by using a variable:

boolean success = enqueueMessage(message);
if (success) {
    ...
}

The above code reads more like English: “If we were successful, …” Methods that don’t have side-effects are (we hope) named so that their return value is clear, such as isEmpty(). This isn’t only true of boolean-valued methods. This code isn’t very clear:

if (flushQueue() == 0) {
    ...
}

whereas this one is:

int itemsFlushed = flushQueue();
if (itemsFlushed == 0) {
    ...
}

Another drawback of calling methods with side effects in if statements is that the entire call could be missed by a reader skimming the code. Compare the two examples with flushQueue() above. In the first the reader could mistake the call for a query that returns some queue attribute. The second more clearly has two parts: in the first an action is taken, and in the second a test is performed. Consider this code I saw in production:

if (!categorySeen.add(categoryID)) continue;

I couldn’t figure where in the loop items were being added to the set. I was reading that line as:

if (!categorySeen.contains(categoryID)) continue;

because I expected the contents of an if statement to have no side effects. But even when I noticed the add() I couldn’t figure out what this did. Can you? (According to the Javadoc of Set the add() method “returns true if this set did not already contain the specified element”.) And note the extra convoluted logic because of the continue (see Avoid continue). The rest of the code will run if the categoryID was not not not already seen: one not for the continue, one not for the !, and one not as part of the API’s description. What?! How about:

boolean isNewCategory = categorySeen.add(categoryID);
if (isNewCategory) {
    ...
}

Here’s a dangerous combination of a method with side effects and abuse of short-circuit evaluation:

if (queueNeedsFlushing() && flushQueue() == 0) {
    ...
}

The second call is particularly easy to miss. Short-circuit evaluation was intended to protect errors in evaluating a side-effect-free statement, such as:

if (count > 0 && total/count >= MIN_AVERAGE) {
    ...
}

or:

if (name != null && name.endsWith(".png")) {
    ...
}

Don’t use the mechanism to avoid calling a method with side effects. That’s what if statements were invented for:

if (queueNeedsFlushing()) {
    int itemsFlushed = flushQueue();
    if (itemsFlushed == 0) {
        ...
    }
}

You’re doing yourself and future readers harm if you think that the terse version above is better than the three-line version here. Three lines is a small price to pay when you’re later having a hard time following the code because you keep missing important calls to methods.

~ See all programming posts ~

__________