HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Executing a task at a particular time in the morning using ScheduledExecutorService

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
morningexecutingthetimeusingscheduledexecutorserviceparticulartask

Problem

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.

Solution

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.

Code Snippets

Date date = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);
int hour = calendar.get(Calendar.HOUR_OF_DAY);
Calendar calendar = Calendar.getInstance();
int hour = calendar.get(Calendar.HOUR_OF_DAY);
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;
}
@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));
}
System.out.println("Comuted Delay for next 5 AM: "+intDelayInHour);
scheduler.scheduleAtFixedRate(new MyTask(), intDelayInHour, 24, TimeUnit.HOURS);

Context

StackExchange Code Review Q#63520, answer score: 11

Revisions (0)

No revisions yet.