2
\$\begingroup\$

I have some basic code which registers functions, with given variables, to be executed after a certain time has elapsed. I will roll this code into my software project when it is ready. If it works it works but since I've never done this before I am the Padawan and one of you out there, who's written code like this before, is the Jedi Master. Here is my code:

#include <unistd.h>
#include <sys/time.h>
#include <stdlib.h>
#include <stdio.h>

typedef union EventArgument {
    int i;
    float f;
} event_argument;

typedef int (*event_function_ptr)(event_argument);


typedef struct EventList {
    event_function_ptr ptr;
    long time;
    event_argument arg;
    struct EventList* next;
} event_list;

event_list events_head;

int register_event (event_function_ptr event_funciton, long in_time, event_argument arg);

int event_print_int (event_argument arg) {printf("%i\n", arg.i);}
int event_print_float (event_argument arg) {printf("%f\n", arg.f);}
int event_kill (event_argument arg) {exit(arg.i);}

int event_99_bottles (event_argument arg) {
    if (arg.i == 0) {
        printf("no more bottles of beer on the wall!");
        event_argument arg_new;
        arg_new.i = -1;
        register_event (&event_kill, 5000, arg_new);
    } else {
        printf("%i bottles of beer on the wall. Take one down and pass it around,\n", arg.i);
        event_argument arg_new;
        arg_new.i = arg.i - 1;
        register_event (&event_99_bottles, 1000, arg_new);
    }

}

long current_time;

//loop from event_head until event.next is NULL or event.next.time strictly greater than current_time + in_time then malloc and insert in list.
int register_event (event_function_ptr ptr, long in_time, event_argument arg) {
    event_list* p = &events_head;
    event_list* q = malloc(sizeof(event_list));
    q->ptr = ptr;
    q->time = current_time + in_time;
    q->arg = arg;

    while (p->next != NULL && p->next->time <= q->time) {
        p = p->next;
    }
    q->next = p->next;
    p->next = q;
}

//loop through events in order and execute functions, then remove from list until event_head.next == NULL or event_head.next.time is greater than current_time. 
int check_events (void) {
    while (events_head.next != NULL && events_head.next->time <= current_time) {
        event_list* p = events_head.next;
        p->ptr(p->arg);
        events_head.next = p->next;
        free(p);
    }
}

int update_time (void) {
    struct timeval time_val;
    gettimeofday(&time_val, 0);
    current_time = time_val.tv_usec / 1000 + time_val.tv_sec * 1000;
}

int main(void) {
    events_head.next = NULL;
    update_time();
    event_argument arg;
    arg.i = 99;
    register_event (&event_99_bottles, 20000, arg);
    arg.i = 512;
    register_event (&event_print_int, 5000, arg);
    arg.f = 3.1415;
    register_event (&event_print_float, 15000, arg);
    arg.i = 58008;
    register_event (&event_print_int, 7000, arg);

    while (1) {
        update_time();
        check_events();
        usleep(900000);
    }
}

Does this look good? Is there anything missing? Is there anything you think I should know now that will benefit me in future?

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Have you tested the code? If it doesn't work, there's not a whole lot of review that can be done. \$\endgroup\$
    – Trojan404
    Aug 10, 2016 at 13:15
  • \$\begingroup\$ Your check_events method doesn't do anything. I've voted to close, because at the moment, even as a proof of concept your code feels incomplete to me. \$\endgroup\$
    – forsvarir
    Aug 10, 2016 at 13:34
  • \$\begingroup\$ A description of how these functions will be implemented. This is not relevant to my question. Does the code look good? Is there anything missing? Is there anything you think I should know now that will benefit me in future? \$\endgroup\$ Aug 10, 2016 at 14:12
  • 1
    \$\begingroup\$ @BostonBrooks actually yes, that's relevant because you ask if something's missing. So, aside from a bunch of style-related problems (bad naming, return types with no return value and duplicated code), you're missing the part where your code actually does something. \$\endgroup\$
    – ChatterOne
    Aug 11, 2016 at 15:37

1 Answer 1

4
\$\begingroup\$

Here are some things that may help you improve your code.

Don't use usleep()

The usleep() function was part of the POSIX standard, but it was marked obsolete in 2001 and removed entirely from the 2009 version of the POSIX standard. You can use nanosleep instead.

Make it clear when loops end

The code currently uses while (1) in main, but it doesn't really loop infinitely. It's better, generally, to have the loop exit condition explicitly stated. For example we could rewrite your main() loop:

while (events_head.next) {
    update_time();
    check_events();
    usleep(900000);
}

This makes it much more clear when and why the loop will end.

Return something useful from the subroutines

Many of the functions that claim to retun an int don't return anything at all, so they should be declared void instead. Alternatively, if there's a value that could usefully be returned, modify the functions to return that instead.

Check return values and handle errors

The code calls malloc but never checks for error return values. This is a serious problem that must be addressed. If the sytem ever runs out of memory, the program will crash.

Consider re-using memory items

Each time an event is registered, new memory is allocated, and each time an event is processed, that memory is freed, even when, as in the case of event_99_bottles, the task re-registers itself. One way to do this would be to have a reregister_event call.

Use a void * as the argument

Right now, if you decide to create a new type of event which uses a new type of argument, you'll have to recompile the whole code. If instead of the EventArugment structure, the EventList contained a void *, then that part of the code would be able to remain the same, even if more elaborate data needed to be passed to specific events. It would then be up to the called function to cast this to whatever it was expecting as input.

Consider using a priority queue

What you seem to be working toward is something like the event dispatcher for a generic task scheduling routine as might be used in a simple operating system. Such task schedulers often use a priority queue to allow for higher and lower priority tasks.

Consider using an intelligent wait

The check_events() routine doesn't currently return anything useful, but it could. If it traversed the list, checking for the next deadline and returned that time, then the code could simply sleep for that amount of time, very efficiently only waking up when there was work to do.

Eliminate global variables where practical

The code currently uses two global variables (current_time and events_head) that probably don't really need to be global. Better would be to pass those as arguments to the functions that need them and/or putting them both inside an Events structure. If you decide to keep either those (or an Events structure) outside of any function, consider making them static which limits them to file scope. Generally speaking, it's good practice to make the scope of variables as small as practical.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.