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.