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

Calling a function every 40 second inside a while loop

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

Problem

This code invokes a function for every 40 seconds inside a loop. I am doubtful about this condition:

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 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.