Is an empty 'while' loop bad practice?

Question

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.

Answer 1

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
}

Answer 2

You wrote

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

So alone the fact that you feel you need to explain the code to us shows you already have understood that the code is not self-explanatory enough.

So why don't you write your code exactly with those words above, so it does not require any "translation"? This is how it could look like:

     while(timeNeedsToBeUpdated())  {
         updateTime();
     }

I am sure you can split up maybeUpdateTime into those two new functions timeNeedsToBeUpdated and updateTime by yourself. You can keep maybeUpdateTime in your code, if required at different places:

 public boolean maybeUpdateTime() {
     if (timeNeedsToBeUpdated()) {
        updateTime();
        return true;
    }
    return false;
}

Of course, there are cases where this splitting into two methods is not easily possible, like mentioned in the comments, and for those cases, an empty block with a code comment, like shown in this answer, is surely the better alternative.

Answer 3

Just replace the semicolon by an empty block with a comment so that the next person knows it is intentional:

while (maybeUpdateTime()) {
    // do nothing, wait until resolves as false
}

Of course some explanation in the problem domain would be better, but don't see enough to make sense of it.

Answer 4

Sometimes, the only work to be done is already done in the loop-condition, and if that doesn't feel forced, that is fine.
No need to break some perfectly fine abstraction.

As an example:

while (std::getline(std::cin, answer) && !validate_answer(answer));

That works.
Still, there is something seriously wrong with the above line.
Specifically, it looks like it is only the head of the loop without the body (the semicolon is easy enough to overlook), or conversely that someone indiscriminately sprinkled around some semicolons and thus supplanted the true body of the loop. It feels unnatural, and always having to re-verify the absence of fire under the smoke gets tiresome.

To make it obvious that such didn't happen, make a true body that looks like intended. While Python has pass for that task, a simple comment or a proper code-block is enough:

while (std::getline(std::cin, answer) && !validate_answer(answer))
    /**/;

while (std::getline(std::cin, answer) && !validate_answer(answer)) {
}

Remember that while your code must be acted upon by a machine, humans are very much also the target audience.

Answer 5

It's valid but problematic for debugging, performance and reading clarity.

Can you use

void updateState() {

    // do some stuff before

    while (this.timeReference.isAfter(lastInterval.getBeggining()) 
    {
        this.timeReference = lastInterval.getEnd();
        lastInterval = intervalIterator.next();
    }

    // do more stuff after

}

instead if maybeUpdateTime() is only called by updateState()? If it's called by other functions, the condition in while could be wrapped to maybeUpdateTime() and inside the loop it is wrapped to updateTime().

Or if you have performance requirements and don't want to (1) consume more CPU resources, (2) block other tasks in the same thread, maybe consider put maybeUpdateTime() into a separate thread that allows you to wait for a task to complete (update time).

Answer 6

As others have said, this doesn't communicate the intent very well and one might need to think for a few seconds to understand the logic.

If you have to keep the maybeUpdateTime() method as is, I would convert the loop into something like the following:

boolean needsUpdate = false;
do {
    needsUpdate = maybeUpdateTime();
}
while (needsUpdate);

This communicates the intent perfectly, doesn't require modifying the maybeUpdateTime() method, and doesn't force you to repeat some of the method's internal logic when calling it.

If you prefer a while loop, this can be written as:

boolean needsUpdate = maybeUpdateTime();
while (needsUpdate) {
    needsUpdate = maybeUpdateTime();
}

Answer 7

Empty while loops

An empty while loop is a bad idea.

At the very least, the loop should contain some waiting logic so you don't just blast through iterations. Something as simple as this:

while (thingIsStillRunning()) 
{ 
    await Task.Delay(250); 
}

This isn't the best approach but it is an acceptable one (in the right scenario). This ensures that you only perform the check at a reasonable interval.

Note that the precise length of that interval is up to you to decide. The bigger you make the interval, the more efficient it will be, but it will of course take longer to notice that the job has been completed. This is a matter of weighing performance vs response time.

Definitely consider working asynchronously here, as it frees up the thread during the wait.

---

Intervals

It also seems like you might be reinventing the wheel here. Intervals are a well established concept and the .NET library already has a solution for them. From MSDN:

using System;
using System.Timers;

public class Example
{
    private static Timer aTimer;

    public static void Main()
{
        // Create a timer and set a two second interval.
        aTimer = new System.Timers.Timer();
        aTimer.Interval = 2000; // every 2 seconds

        // Hook up the Elapsed event for the timer. 
        aTimer.Elapsed += OnTimedEvent;

        // Have the timer fire repeated events (true is the default)
        aTimer.AutoReset = true;

        // Start the timer
        aTimer.Enabled = true;

        Console.WriteLine("Press the Enter key to exit the program at any time... ");
        Console.ReadLine();
    }

    private static void OnTimedEvent(Object source, System.Timers.ElapsedEventArgs e)
    {
        Console.WriteLine("The Elapsed event was raised at {0}", e.SignalTime);
    }
}

The example displays output like the following:

The Elapsed event was raised at 5/20/2015 8:48:58 PM
The Elapsed event was raised at 5/20/2015 8:49:00 PM
The Elapsed event was raised at 5/20/2015 8:49:02 PM
The Elapsed event was raised at 5/20/2015 8:49:04 PM
The Elapsed event was raised at 5/20/2015 8:49:06 PM

If your interval is fixed, then this is a much more preferred approach. Even if your intervals are dynamically adjustable, it's still worth looking into manually resetting your interval timer (and reconfiguring the interval time as needed).

---

"Maybe" logic

As an aside, maybeUpdateTime is not a good way to structure your methods. Your methods shouldn't leave the reader guessing as to what is or isn't happening. Instead, your methods should simply do the work, and then the "maybe" part should shown explicitly instead of hidden inside the method.

Compare these two examples:

maybeUpdateTime();

versus

if(timeNeedsUpdating())
    updateTime();

The second example makes it clear exactly when the time will or won't be updated. The reader doesn't event need to look inside either method to know what's happening. In the first example, you'd need to dig deeper before you'd understand what is happening.

Answer 8

I think the question is good but the example is bad. In the example, updateState should advance the iterator in a single operation to the interval including the timeReference. It should not iterate.

A better example might be running jobs from a queue, with methods runOneJob() and runAllJobs(). I would have no qualms writing:

public void runAllJobs()
{
    while (runOneJob());
}

In light of some other answers, I might now write:

public void runAllJobs()
{
    while (runOneJob()) {}
}