11
\$\begingroup\$

I have a task which I need to run every 6 in the morning. I have the below code which does the job and uses ScheduledExecutorService.

ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);
Date date = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);

int hour = calendar.get(Calendar.HOUR_OF_DAY);
int delayInHour = hour < 6 ? 6 - hour : 24 - (hour - 6);

System.out.println("Current Hour: "+hour);
System.out.println("Comuted Delay for next 5 AM: "+intDelayInHour);

scheduler.scheduleAtFixedRate(new MyTask(), intDelayInHour, 24, TimeUnit.HOURS);

I am opting for code review to see whether it can be improved or not.

\$\endgroup\$
11
\$\begingroup\$

According to the javadoc, a Calendar instance is initialized with the current date and time. So instead of this:

Date date = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);
int hour = calendar.get(Calendar.HOUR_OF_DAY);

You don't need to create a separate Date object, so you can simplify like this for the same effect:

Calendar calendar = Calendar.getInstance();
int hour = calendar.get(Calendar.HOUR_OF_DAY);

It would be good to split this code into multiple reusable and testable functions. For example you could have a function that returns the hours until some target hour:

private int getHoursUntilTarget(int targetHour) {
    Calendar calendar = Calendar.getInstance();
    int hour = calendar.get(Calendar.HOUR_OF_DAY);
    return hour < targetHour ? targetHour - hour : targetHour - hour + 24;
}

Now the target hour is a parameter, this is reusable and flexible, and eliminates the magic number 6 that was duplicated at multiple places. Also note that I changed the ternary expression a bit, I think targetHour - hour + 24 is more natural, and easier to understand due it's symmetry with targetHour - hour.

You can move the scheduling to another method, which can now have a single responsibility (scheduling) and not worry about calculating hours.


Here's a unit test to verify that getHoursUntilTarget works well:

@Test
public void testGetHoursUntilTarget() {
    Calendar calendar = Calendar.getInstance();
    int hour = calendar.get(Calendar.HOUR_OF_DAY);
    assertEquals(24, getHoursUntilTarget(hour));
    assertEquals(1, getHoursUntilTarget(hour + 1));
    assertEquals(23, getHoursUntilTarget(hour - 1));
}

Note that at 5:59 AM, the hours until target will be 1, even though 6 AM is only one minute away. This is how it was in your original code, I hope that's ok with you.


There is some noise in your post:

System.out.println("Comuted Delay for next 5 AM: "+intDelayInHour);
scheduler.scheduleAtFixedRate(new MyTask(), intDelayInHour, 24, TimeUnit.HOURS);

"next 5 AM" is not consistent with the rest of your code (you used 6 everywhere else), and you don't have a variable intDelayInHour but you have delayInHour. Review more carefully before you post.

\$\endgroup\$
7
\$\begingroup\$

You have an issue if you live/deploy in an area that has daylight saving times. The way you set it up is to happen every 24 hours, rather than at a particular time each day. When a time shift happens, your event will happen at 5am, or 7am depending.

In the past I have set up a time-of-day schedule for jobs, and I found the best way to do that was to set up a self-repeating wrapper for a task. When the task runs, it reschedules itself for the next iteration.

So, the way I did it was to create a Runnable that self-replicates each time it runs. The concept ends up being somewhat simple (somewhat...). So, consider the following code that will reschedule itself after running:

import java.util.Calendar;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;


public class ReschedulingDailyTask implements Runnable {

    private final ScheduledExecutorService service;

    private final Runnable task;
    private final int hour;
    private final int min;
    private final int sec;

    private final AtomicBoolean active = new AtomicBoolean(false);
    private final AtomicBoolean scheduled = new AtomicBoolean(false);

    public ReschedulingDailyTask(final ScheduledExecutorService service,
            final Runnable task, final int hour) {
        this (service, task, hour, 0, 0);
    }

    public ReschedulingDailyTask(final ScheduledExecutorService service,
            final Runnable task, final int hour, final int min, final int sec) {
        this.service = service;
        this.task = task;
        this.hour = hour;
        this.min = min;
        this.sec = sec;

    }

    public void enable() {
        if (!active.getAndSet(true)) {
            // was not enabled:
            reSchedule();
        }
    }

    public void disable() {
        active.getAndSet(false);
    }

    private void reSchedule() {
        if (!scheduled.getAndSet(true)) {
            Calendar calendar = Calendar.getInstance();
            long now = calendar.getTimeInMillis();
            calendar.set(Calendar.HOUR, hour);
            calendar.set(Calendar.MINUTE, min);
            calendar.set(Calendar.SECOND, sec);
            calendar.set(Calendar.MILLISECOND, 0);
            while (calendar.getTimeInMillis() < now) {
                // scheduled in the past, go forward one day....
                calendar.add(Calendar.DAY_OF_MONTH, 1);
            }

            // for debug, if needed....
            //System.out.println("Reschedule for " + calendar.getTimeInMillis() 
            //        + " (in " + (calendar.getTimeInMillis() - now) + "ms)" );

            // set ourselves up to run at a given time.
            service.schedule(this, calendar.getTimeInMillis() - now, TimeUnit.MILLISECONDS);
        }

    }

    @Override
    public void run() {
        // since we are running, we are no longer scheduled...
        scheduled.set(false);

        // we may have been disabled after we were enabled ...
        // you can't cancel the schedule, but you can ignore the task...
        if (!active.get()) {
            return;
        }

        // we were active, and we run the task, and force the reschedule.
        try {
            task.run();
        } finally {
            reSchedule();
        }
    }

}

I tested it with the following code:

    private static final class DaemonThreadFactory implements ThreadFactory {

        @Override
        public Thread newThread(Runnable runnable) {
            Thread thread = new Thread(runnable, "Schedule Thread");
            thread.setDaemon(true);
            return thread;
        }

    }

    public static void main(String[] args) throws InterruptedException {
        ScheduledExecutorService service = Executors.newScheduledThreadPool(1, new DaemonThreadFactory());
        Calendar cal = Calendar.getInstance();
        System.out.println("Time now: " + cal.getTimeInMillis());
        cal.add(Calendar.SECOND, 5);
        cal.set(Calendar.MILLISECOND, 0);
        System.out.println("Execute at: " + cal.getTimeInMillis());
        int hour = cal.get(Calendar.HOUR);
        int min = cal.get(Calendar.MINUTE);
        int sec = cal.get(Calendar.SECOND);
        final Object lock = new Object();
        Runnable task = new Runnable() {

            @Override
            public void run() {
                System.out.println("Time is " + Calendar.getInstance().getTimeInMillis());
                synchronized(lock) {
                    lock.notifyAll();
                }
            }

        };

        ReschedulingDailyTask daily = new ReschedulingDailyTask(service, task, hour, min, sec);
        daily.enable();

        System.out.println("Waiting");
        synchronized (lock) {
            lock.wait();
        }
        System.out.println("Done");
    }

Note that I create a Daemon thread factory for the Scheduler, so that, if the main thread closes, the application terminates. The above method will schedule the task for about 5 seconds in the future....

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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