Can this loop code be simplified somehow?

Question

I have a problem... it's basically that my code is ugly and I don't like it. I was wondering if there was a way to simplify it (I use java 8)

I have these "code blocks" that follow this pattern, I have about 5 or 6 of them within a method so this method looks very repetitive and ugly.

The loops are all the same, just the code varies inside.

Is there any way to simplify this?

CODE BLOCK EXAMPLE

            String id = null;
            for (int i=0; i< NUM_CHECKS; i++) {

                // BEGIN VARIABLE CODE
                id = getPrice();
                if (id != null) break;
                // END VARIABLE CODE 

                // sleep between checks
                if (i < NUM_CHECKS -1) Thread.sleep(DELAY);
            }

EXAMPLE

            String id = null;
            for (int i=0; i< NUM_CHECKS; i++) {

                // BEGIN VARIABLE CODE
                id = getPrice();
                if (id != null) break;
                // END VARIABLE CODE 

                // sleep between checks
                if (i < NUM_CHECKS -1) Thread.sleep(DELAY);
            }
            
            for (int i=0; i< NUM_CHECKS; i++) {

                // BEGIN VARIABLE CODE
                x=x*2;
                if (x>25) break;
                // END VARIABLE CODE 

                // sleep between checks
                if (i < NUM_CHECKS -1) Thread.sleep(DELAY);
            } etc... a couple more blocks

Answer 1

How about coding an abstraction to contain all the boilerplate?

class MyLoop
{
    private int numChecks;

    private int delay;

    public MyLoop(int numChecks, int delay) {...}

    public void loopAndSleep(MyTask task)
    throws InterruptedException
    {
        // Update: It is important to set properly the order of the looping conditions, 
        // to stop invoking hasEnded() as soon as i<numChecks==false (Thaks to Simon Eismann).
        for (int i=0; i<numChecks && !task.hasEnded(); i++)
        {
            if (i < numChecks -1)
            {
                Thread.sleep(DELAY);
            }
        }
    }
}

interface MyTask
{
    public boolean hasEnded();
}

So, you can replace each one of your 5-6 places in your program by:

new MyLoop(NUM_CHECKS, DELAY).loopAndSleep(new MyTask(){...});

By properly extending MyTask you can give them specific status variables.

Answer 2

The structure you provide is called a "polling loop" and you are correct, it is poor programming style, as are all the replies that contain the same polling loop.

It would be far better to use events.

Look in the "getPrice()" function, get to wherever that return value is being changed, and create an event when the change happens. Then in your code write a handler and in the handler do all the stuff that currently happens after your polling loop succeeds.

Answer 3

If you want to try some operation until the return value is available, you may do the following (Java-8 way):

public static <T> Optional<T> retryWithDelay(int numberOfChecks, int delay, 
                               Supplier<Optional<T>> supplier) throws InterruptedException { 
    for(int i=0; i<numberOfChecks; i++) {
        if(i > 0)
            Thread.sleep(DELAY);
        Optional<T> result = supplier.get();
        if(result.isPresent()) return result;
    }
}

And use it like this:

String id = retryWithDelay(NUM_CHECKS, DELAY, () -> Optional.ofNullable(getPrice()))
                   .orElse(null);

Or if you don't like optionals for some reason, you can stick with null:

public static <T> T retryWithDelay(int numberOfChecks, int delay,
        Supplier<T> supplier) throws InterruptedException {
    for (int i = 0; i < numberOfChecks; i++) {
        if (i > 0)
            Thread.sleep(delay);
        T result = supplier.get();
        if (result != null)
            return result;
    }
    return null;
}

And use it like this:

String id = retryWithDelay(NUM_CHECKS, DELAY, () -> getPrice());

Or using method reference:

String id = retryWithDelay(NUM_CHECKS, DELAY, this::getPrice);

Note that the second example with x = 2*x is more difficult as it has some mutable state. It can be solved in dirty way like this:

AtomicInteger x = new AtomicInteger(1);
Integer result = retryWithDelay(NUM_CHECKS, DELAY, () -> {
    int val = x.get()*2;
    x.set(val);
    return val > 25 ? val : null;
});

However I hope this version was just for illustration, not the real code.

---

There's also somewhat more sophisticated approach which probably abuses the API, but allows more flexibility. You can create an IntStream of increasing numbers, but they are available with given delay:

public static IntStream delayedStream(int numberOfChecks, int delay) {
    return IntStream.range(0, numberOfChecks)
                    .peek(x -> {
                        if(x > 0) {
                            try {
                                Thread.sleep(delay);
                            } catch (InterruptedException e) {
                                // ignore
                            }
                        }
                    });
}

So the first problem can be solved now as:

String id = delayedStream(NUM_CHECKS, DELAY)
                  .mapToObj(x -> getPrice())
                  .filter(Objects::nonNull)
                  .findFirst().orElse(null);

And the second can be solved like this (assuming initial x value is 1):

int x = delayedStream(NUM_CHECKS, DELAY)
             .map(idx -> 1 << (idx+1))
             .filter(val -> val > 25)
             .findFirst().orElse(-1);

Answer 4

You can use recursion to make to loop reusable, but this would only make sense if you use the loop a lot.

public void loopWithDelay(int numberOfChecks, int delay, Runnable r) { 
   if (numberOfChecks != 0) {
      r.run();
      loopWithDelay(numberOfChecks - 1, delay, r);
      Thread.sleep(DELAY);
   }
}

The actual call would look something like this:

loopWithDelay(5, 1000, new Runnable() {
                  @Override
                  public void run() {
                     //Variable code goes here
                  }
             });

On a general note, are you sure you want to wait DELAY seconds after an action or have the action occur every DELAY seconds?

EDIT: I am dumb, no need for recursion, this works aswell:

public void loopWithDelay(int numberOfChecks, int delay, Runnable r) { 
   for (int i = 0; i < numberOfChecks; i++) {
      r.run();
      if (i != numberOfChecks -1)
         Thread.sleep(DELAY);
   }
}