Design – Is an Empty ‘while’ Loop Bad Practice?

clean codedesignprogramming practices

Consider the following:

public boolean maybeUpdateTime() {
    if (this.timeReference.isAfter(lastInterval.getBeginning()) {
        this.timeReference = lastInterval.getEnd();
        lastInterval = intervalIterator.next();
        return true;
    }
    return false;
}

public void updateState() {

    // do some stuff before

    while (maybeUpdateTime());

    // do more stuff afeter

}

Which roughly translates to: while it needs to be updated – update it.

Considering that maybeUpdateTime is a function/method that can be called independently, would it still be considered a bad practice to write such an empty while?

Otherwise I would have to create two functions, isTimeAfterInterval and updateTime, and call both every time.

I used this time and interval-based example just to illustrate, but the specifics don't matter to my question.

Note that maybeUpdateTime is a "public" function that can be called independently of updateState and not always inside a loop until it returns false like there.

Therefore, it is important to wrap it inside a function of its own. This code is an example and thus I kept it simple, but it could be a much bigger computation.

Best Answer

From a code-review perspective - the intuitive process

The function maybeUpdateTime does not seem to be problematic. However, it definitely needs a better name, one that conveys information to a programmer succinctly.

The while loop inside the updateState function may trigger a false-alarm for a programmer who is reading the code. That is, it makes the reader feel scary, and compels the reader to fetch more information to assure that it is not a bug.

When a programmer reads a while loop and tries to understand what it does, there are several pieces of information that the programmer needs to grasp (or guess):

  • Is this loop going to execute at least once?
    • Why does this question matter? If the loop isn't going to execute at all, I'm not too concerned about the correctness or applicability of the code inside the loop.
  • Is it going to execute a fixed number of times?
    • If the loop count is determined beforehand, I can glean more information by finding out where that loop count is derived from.
    • If there is no pre-determined loop count, I will need to find out the dynamic conditions - what is the loop termination condition?
  • If the loop termination condition is computed in another function,
    • Am I able to guess what that condition is, just by seeing the other function's name?
    • If the other function's name didn't provide adequate information (about what that loop-termination condition is), I will have to go to that function and read about it.

Altogether, these thought processes increased the cognitive load of the person reading the code (especially the while loop). If one or more issues are fixed, the cognitive load will decrease, and the while loop will become less scary to the other programmer.

From a code-review perspective - objections that may be raised

Note: this section dissects into the code example provided by OP. As OP explains, such detail is not the main focus of this question.

public boolean maybeUpdateTime() 
{
    if (this.timeReference.isAfter(lastInterval.getBeggining()) 
    {
        this.timeReference = lastInterval.getEnd();   // mutation
        lastInterval = intervalIterator.next();   // mutation
        return true;
    }
    return false;
}

In the code sample above, changes are made to two instance fields (of the this object). The function's name will need to communicate either the how or the why these field changes are made.

But how do we explain the why?

We can only explain in a manner that makes sense to fellow humans. For this reason, we (humans) create idioms and analogies, drawing from experience from our everyday, non-software-related lives.

We also try to separate a complex process into orthogonal, composable concerns.

It should be obvious that the code sample above is taken from a class that has certain iterator-like behavior. It is also obvious that the class has other responsibilities as well.

Thus, the first change we'd make is to isolate the iterator-like behavior - to make it an iterator that is extensible, without bundling with the additional responsibility. This is called the Single Responsibility Principle (SRP).

The sample code may have more than one use cases. Some examples are:

  • To iterate through the entire collection, one at a time, and perform a bit of work on each item (via the extensibility of an iterator)
  • To search for something (i.e. to iterate through the collection until a condition is met), without modifying the collection.
  • To execute an algorithm that examines the inter-relationship between adjacent items in the collection, e.g. to find consecutive items that represent time ranges that touch or overlap.

In each of the use case, it would help if the code is refactored (restructured) so that the what is being computed is made obvious, and separate from the act of iterating through.

From a coding style perspective

Given OP's desire to focus on the tension between:

  • There is already a public function; its name is NOT going to be changed;
  • There is another function that calls that public function; however, the way it is called may look scary to a fellow programmer.

In this case, the advise is to wrap that function so that the wrapper's name conveys the missing context.

Illustrated solution:

// Function name is part of public API
// Function name cannot be changed
// Function name is meaningful when seen from the API's context
// Suggestion to change this function name cannot be fulfilled
public boolean somePublicFunction()
{
    ...
}

// Because this is a thin wrapper, 
// you can change its name to give more context.
private boolean advanceIteratorOnce()
{
    somePublicFunction();
}

// If it is necessary to provide more context to the fellow programmer
// reading your code, consider making it obvious what is the stopping 
// condition for the while loop.
private void keepDoingUntilSomethingHappens()
{
    while (advanceIteratorOnce());
}

public void doMultipleThings() 
{
    // do some stuff before

    // You may use option 1, option 2, or both.
    // What is important is that the fellow programmer will not feel scared
    // because the important information about the loop has been made obvious.
    keepDoingUntilSomethingHappens();

    // do more stuff afeter
}
Related Topic