17

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.

  • 3
    Consider renaming the function to something like processNextInterval(). Then it is easy to guess while(processNextInterval()) {} processes intervals as long as there are still some intervals to process – user253751 Nov 30 '20 at 14:32
  • Nit: "getBeggining" is misspelled, should be "getBeginning" (yes I am that guy in PR reviews :p) – Ian Kemp Nov 30 '20 at 15:12
  • 1
    I don't see why you think it's a busy wait.. This example I actually took from a class that makes some business specific analysis over two calendars. It iterates over two lists of events, keeping the second in sync with the first, time wise, using a similar method of maybeUpdateTime. – Lucas Noetzold Nov 30 '20 at 16:55
  • 1
    I would say it is bad practice to give a function a name like "maybeUpdateTime" which gives some indication of what it might do, but no indication of what it will actually do. If no clear and simple name presents itself, it should probably be two functions. – user3153372 Dec 1 '20 at 8:55
  • 2
    List::add appears to do something even dumber, in that it may never return false, raising an exception instead. In that case, it may as well just have void as its return value, and if it returns, you can assume it succeeded. It's debatable whether it's the job of add to inform you if adding an element "failed" simply because it was already in the list. – chepner Dec 1 '20 at 14:44
17

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
}
  • 5
    I’d point out that there may well be a language construct that makes the code better, ie a repeat/until or do/while loop, which given that nothing happens inside the loop is easier to translate from code to intent even without comments, with comments, it’s almost idiomatic. – jmoreno Nov 28 '20 at 22:47
  • Now we just need 16 more votes – Basilevs Nov 30 '20 at 8:47
41

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.

  • 6
    I would move Doc's little code fragment into it's own method with a good name, perhaps bringTimeCompletelyUpToDate(). Have to admit, this makes you wonder why the original maybeUpdateTime() doesn't already have the while loop built in. It makes little sense (to me) to bring the time slightly up to date. But may be details I don't understand. – user949300 Nov 28 '20 at 21:51
  • 2
    @user949300 My example fails in this perspective, for it gives the idea that it would not make sense to advance just once, without bringing it up completely to date. Consider, instead of time related stuff, a product inventory counter: you could ask the 'responsible' object to bring it all up to date, if you just want to see the numbers, or count it one by one whilst making some other validations, or side effects, for while it still have things to count (now that I think about, it's an iterator). – Lucas Noetzold Nov 28 '20 at 22:37
  • 7
    What if splitting up maybeUpdateTime in this way makes a race condition possible, that would not be possible if a single function were responsible for both actions? (It doesn't in the exact example given in the question, but the question also specifies that this was just example code and the question was about this pattern generally.) – Daniel Wagner Nov 29 '20 at 17:26
  • 1
    @Adam: this "two step approach" is not dangerous in general for the asked question. It may not be a valid refactoring for what Daniel had in mind, but that is a different question. – Doc Brown Nov 30 '20 at 7:40
  • 2
    @Flater: this may be correct - or not, hard to say without more context. However, I did not question the OPs code; you may consider write a comment to their question, not to mine. – Doc Brown Nov 30 '20 at 12:04
22

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.

  • I don't see this as an improvement at all. 1. Why add a body just to add a comment inside? Surely adding the comment on the same line or before the while is equivalent without changing the syntax at all. 2. That's easier to solve by making the code self-documenting (rename the function). 3. Not being sufficiently clear is a smaller problem over the busy-wait scenario. In general, you'd want to be notified when to update, not just wait and poll. Might not always be an option but adding comments is not the path forward here anyway. – VLAZ Nov 30 '20 at 12:24
  • 4
    Elaborate though. // loop until queue is caught up to current time – user253751 Nov 30 '20 at 13:28
  • 6
    @VLAZ The semicolon is easy to miss, and the explicit empty body makes it less likely that someone mis-reads the code and thinks that the next statement is actually the loop body. Or, since you rarely use semicolons with while, someone could think it's a typo and erroneously delete the semicolon. Using empty braces to mark a loop as intentionally empty is a common thing seen in coding standards documents. Placement of the comment is mostly a matter of preference. – bta Nov 30 '20 at 23:41
  • I think the loop here is a bad example because everyone focuses on the busy wait, if the algorithm really loops naturally and there's no better loop construct and the while loop has only a condition and no body, than make it clear to the reader that the code is complete. don't just place a semicolon, because it is shorter. – mkie Dec 1 '20 at 1:38
  • 3
    @VLAZ some coding guidelines require braces for any dependent statement (in a loop, after if). It is somewhat pedestrian, but that 's sometimes not bad. Remember the Apple bug? Even if this article argues that braces wouldn't have saved the day, I think in many instances it would. – Peter - Reinstate Monica Dec 1 '20 at 10:16
7

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.

  • One could simply argue that std::getline is poorly designed, which I agree with. It is irrelevant that it is a part of the standard C++ library, which automatically makes it popular. Popular != perfectly fine abstraction. – freakish Nov 29 '20 at 13:57
  • @freakish Ok, I'll bite. How should it look? And remember to stay efficient. Anyway, there are others if you don't like std::getline, it doesn't depend on this one function. – Deduplicator Nov 29 '20 at 14:36
  • I would prefer to see something like loop { std::getline(std::cin, answer); if (validate_answer(answer)) break; } or using a do-while loop. Effectful functions in conditionals are difficult to reason about. (You probably want to do something besides just break if std::getline fails anyway.) – Mario Carneiro Nov 29 '20 at 17:41
  • 2
    @MarioCarneiro You know that doesn't have the same semantics? Specifically, it doesn't check for input error, like end of input. – Deduplicator Nov 29 '20 at 18:13
  • 1
    @Deduplicator I do, as I mentioned you shouldn't be just breaking on that anyway since that mixes up an IO error with a valid answer, which seems bad. To replicate that code's behavior you could write int err = getline(cin, answer); if (err) break; if (!validate_answer(answer)) break; and there are a variety of ways to compress that depending on requirements. But you should probably do something about err in the first break and the original version makes it difficult to handle the error. – Mario Carneiro Nov 29 '20 at 19:42
6

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).

  • The problem with this approach is that, as I mentioned, maybeUpdateTime could be called outside updateState, any number of times. So it should be wrapped in its own function (for instance, it could be a way more complex computation, the code above is just an example). – Lucas Noetzold Nov 28 '20 at 20:19
  • 1
    @LucasNoetzold Can you rename maybeUpdateTime so that it communicates succinctly what is the stopping criteria for the while loop? This is not an answer in itself, but a better name can illuminate a way toward a better answer. – rwong Nov 28 '20 at 20:22
  • 1
    Its a good advice @rwong, but I'll not edit the question because it already have answers mentioning the names I gave. – Lucas Noetzold Nov 28 '20 at 20:35
5

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();
}
  • 1
    This is close to the answer I was going to write. A named variable adds a lot of readability here. – bmm6o Nov 30 '20 at 17:39
4

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()) {}
}
4

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.

  • "At the very least, the loop should contain some waiting logic so you don't just blast through iterations. Something as simple as this: await Task.Delay(250);" <- this is actually really dumb. The computer has a bunch of jobs to process, and there's absolutely no reason to delay 250ms after each job. Wouldn't you prefer if the jobs were processed as quickly as possible? – user253751 Nov 30 '20 at 15:16
  • @user253751: Your comment doesn't make sense when the completion condition (i.e. breaking the loop) is not contingent on the number of iterations you've done. The loop is awaiting an external element (in this case the current time, but it could be any outside job), that job is always going to take exactly as long regardless of how quickly you iterate. In other words, it's going to take exactly as long from now until midnight, regardless of how often you check your watch. Checking your watch constantly and without delay does not make midnight come faster - but it does demand constant attention. – Flater Nov 30 '20 at 16:13
  • You also seem to misunderstand asynchronicity, as delaying one thread (as opposed to synchronously sleeping it) specifically does not block other threads (i.e. the "bunch of jobs" you're talking about). – Flater Nov 30 '20 at 16:14
  • 1
    The word "interval" in my example actually comes from intervals in production time of an industry, I didn't realize it could be interpreted as a sign of a busy loop at the time, if there weren't so many answers already I would think of a better example and edit it. But that method is an actual computation, not a busy loop. – Lucas Noetzold Nov 30 '20 at 17:21
  • 1
    @Flater Nothing in the question references the current time. I suggest you read it again. It is processing a queue of items which include timestamps. For some reason you have decided to insert a delay after processing item in the queue. – user253751 Nov 30 '20 at 17:26

Not the answer you're looking for? Browse other questions tagged or ask your own question.