patternjavaModerate
Executing a task at a particular time in the morning using ScheduledExecutorService
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
I am opting for code review to see whether it can be improved or not.
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
You don't need to create a separate
so you can simplify like this for the same effect:
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:
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
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
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:
"next 5 AM" is not consistent with the rest of your code (you used 6 everywhere else),
and you don't have a variable
Review more carefully before you post.
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.