patterncMinor
Calling a function every 40 second inside a while loop
Viewed 0 times
whileloopfunctioneverysecondcallinginside
Problem
This code invokes a function for every 40 seconds inside a loop. I am doubtful about this condition:
Is only checking
Note: I don't want to use a Linux timer system call or any other method to invoke the function periodically.
if ((time_left interval))Is only checking
(time_left interval. Is this check required?int print_timed_op()
{
time_t time_now;
time_t time_left;
time_t time_next_interval;
int interval = 40, hit_count =10; //40 second interval, 10 times
time_next_interval = time(0) + interval;
//tight loop
while (1) {
sleep(1);
time_now = time(0);
time_left = time_next_interval - time_now;
/* here time_left > interval check required ? */
if ((time_left interval)) {
call_my_fuc();
time_next_interval = time(0) + interval;
time_left = interval;
hit_count--;
}
if(hit_count< 0)
break;
}
return 0;
}Note: I don't want to use a Linux timer system call or any other method to invoke the function periodically.
Solution
This seems like a fairly straightforward implementation with reasonable naming. It seems very maintainable, so good work there. Here are a few suggestions:
Useless variables
The
It's fewer characters, and since you're calling
Don't Repeat Yourself
Usually the "Don't Repeat Yourself" suggestion has to do with code being copied several times. You haven't done that, but you are running your loop far more frequently than you have to. Instead of sleeping for 1 second and then checking if you've reached your desired time, why not sleep for the desired amount of time? Something like this:
Use Your Loop Counter as a Loop Counter
You have a loop control variable, but you aren't directly using it as such. Instead you've written an infinite loop and then a conditional which checks the loop counter and breaks out of the infinite loop. This isn't the 1970s! We have proper control flow structures now. Just do:
Useless variables
The
time_now variable doesn't seem to serve much of a purpose. You calculate it, then immediately use it. However, you could just call time(0) directly like this:time_left = time_next_interval - time(0);It's fewer characters, and since you're calling
time(0) directly in several other places, any reader of the code will already have to know what it means.Don't Repeat Yourself
Usually the "Don't Repeat Yourself" suggestion has to do with code being copied several times. You haven't done that, but you are running your loop far more frequently than you have to. Instead of sleeping for 1 second and then checking if you've reached your desired time, why not sleep for the desired amount of time? Something like this:
time_t remainder = interval;
while(1)
{
sleep(remainder);
start_time = time(0);
call_my_func();
hit_count--;
if(hit_count< 0)
break;
remainder = (start_time + interval) - time(0);
}Use Your Loop Counter as a Loop Counter
You have a loop control variable, but you aren't directly using it as such. Instead you've written an infinite loop and then a conditional which checks the loop counter and breaks out of the infinite loop. This isn't the 1970s! We have proper control flow structures now. Just do:
while (hit_count >= 0)
{
//... etc.
hit_count--;
}Code Snippets
time_left = time_next_interval - time(0);time_t remainder = interval;
while(1)
{
sleep(remainder);
start_time = time(0);
call_my_func();
hit_count--;
if(hit_count< 0)
break;
remainder = (start_time + interval) - time(0);
}while (hit_count >= 0)
{
//... etc.
hit_count--;
}Context
StackExchange Code Review Q#149465, answer score: 3
Revisions (0)
No revisions yet.