3
\$\begingroup\$

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

if ((time_left <= 0 || time_left > interval))

Is only checking (time_left <= 0) fine? I referred to some websites and they have also added a check for 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 <= 0 || 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.

\$\endgroup\$
3
  • \$\begingroup\$ How important is performance to you? In many cases || is slower than |. \$\endgroup\$ Dec 10 '16 at 4:15
  • \$\begingroup\$ performance is not an issue. precision in second also fine. \$\endgroup\$
    – BEPP
    Dec 10 '16 at 4:16
  • \$\begingroup\$ Are you wanting to call the function every 40 seconds, or to call it 40 seconds after the last call completed? They aren't necessarily the same thing (what happens if the function takes 10 seconds to execute). \$\endgroup\$
    – forsvarir
    Dec 10 '16 at 9:57
3
\$\begingroup\$

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--;
}
\$\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.