Event scheduler in C

Question

One of my university assignments asked us to create a program using struct in order to create a simple event scheduler. This program is for a single day only, not multiple days.

Sample Input / Output

For a few reasons, I've opted to create a .GIF to display the sample input / output:

An example save file:

8 30 dentist_appointment
14 0 pickup_friend_from_airport
17 0 buisness_meeting_at_the_office
20 30 dinner_reservation

---

Disclaimer / Notice: The following code is not to be copied without proper credit. It is licensed under cc by-sa 3.0 with attribution required.

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
#include <stdlib.h>
#include <ctype.h>

#define _MAX_EVENTS 10 // 10 Events Max
#define _MAX_DESCRIPTION 101 // 100 Character Description Max

typedef struct { // typedef a struct called event

    int hour; // Store the hour / HH
    int minute; // Store the minute / MM
    char description[_MAX_DESCRIPTION]; // Store the event description

} event;

// Print the menu selection
void printMenu() {

    puts("+------ SCHEDULER ------+\n"
        "|  1. New Event         |\n"
        "|  2. Delete Event      |\n"
        "|  3. Display Schedule  |\n"
        "|  4. Save Schedule     |\n"
        "|  5. Load Schedule     |\n"
        "|  6. Exit              |\n"
        "+-----------------------+\n");

}

// Return true if an event is NULL, false otherwise
bool isNull(const event *e) { return e == NULL; }

// Allocate memory for and initialize an event
event *initEvent() {
    event *e = (event*)malloc(sizeof(event));

    e->hour = 0;
    e->minute = 0;
    strcpy(e->description, "");

    return e;
}

// Take user input until value is between min and max inclusive, return the input
int inputRange(const int min, const int max) {

    int input = 0;
    char temp[21];
    char *prompt = "| Enter a number between %d and %d: ";

    printf(prompt, min, max);

    fgets(temp, 21, stdin);
    input = atoi(temp);

    while (input > max || input < min) { // Data validation
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    }

    return input;

}

// Setup a new event with user input and return a pointer to the same event
event* newEvent(event *e) {

    if (isNull(e)) { // If e is NULL
        e = initEvent(); // Initialize it
    }

    char *seperator = "+--------------------------------+";

    printf("\n%s\n|           NEW EVENT            |\n%s\n\n", seperator, seperator);

    puts("+---------- EVENT TIME ----------+");

    e->hour = inputRange(0, 23);
    e->minute = inputRange(0, 59);

    puts(seperator);

    puts("\n+--- EVENT DESCRIPTION ---+");

    printf("%s", "| Enter a description: ");

    fgets(e->description, _MAX_DESCRIPTION, stdin);

    puts("+-------------------------+\n");

    puts("| Event successfully added.\n");

    return e;

}

// Add an event to an event list at a specified index
void addEventAtIndex(event list[], const event e, const int i) {

    if (isNull(&e)) { // if our event is NULL, return
        return;
    }

    list[i].hour = e.hour;
    list[i].minute = e.minute;
    strcpy(list[i].description, e.description);

}

// Insertion sort by swapping struct members
void sort(event list[], const int size) {

    for (int i = 1; i < size; i++) {
        for (int j = i; j > 0 && (list[j - 1].hour > list[j].hour || (list[j - 1].hour == list[j].hour && list[j - 1].minute > list[j].minute)); j--) {
            int hourJ = list[j].hour;
            int minuteJ = list[j].minute;
            char descriptionJ[_MAX_DESCRIPTION];
            strcpy(descriptionJ, list[j].description);

            int hourJMinus1 = list[j - 1].hour;
            int minuteJMinus1 = list[j - 1].minute;
            char descriptionJMinus1[_MAX_DESCRIPTION];
            strcpy(descriptionJMinus1, list[j - 1].description);

            list[j].hour = hourJMinus1;
            list[j].minute = minuteJMinus1;
            strcpy(list[j].description, descriptionJMinus1);

            list[j - 1].hour = hourJ;
            list[j - 1].minute = minuteJ;
            strcpy(list[j - 1].description, descriptionJ);
        }
    }

}

// Add an event to an event list by sorting it into position
void sortInsert(event list[], int *size, event e) {

    addEventAtIndex(list, e, *size); // Add event to the end of the list

    (*size)++; // Increment size

    // Insertion Sort
    sort(list, *size);

}

// Display an event in a readable format: [ID] HH:MM - DESCRIPTION
void printEvent(const event e) {

    char h1 = { (e.hour / 10) + '0' }; // Extract the first digit and convert to char (if any, else 0)
    char h2 = { (e.hour - (e.hour / 10) * 10) + '0' }; // Extract the second digit and convert to char

    char m1 = { (e.minute / 10) + '0' };
    char m2 = { (e.minute - (e.minute / 10) * 10) + '0' };

    printf("%c%c:%c%c - %s", h1, h2, m1, m2, e.description);

}

// Display all events in an event list
void printEventList(const event list[], const int size) {

    if (size == 0) {
        puts("\n| You have no events scheduled!\n");
        return;
    }

    char *seperator = "+--------------------------------+";

    printf("\n%s\n|          MY SCHEDULE           |\n%s\n\n", seperator, seperator);

    for (int i = 0; i < size; i++) {
        printf("| [%d] ", i);
        printEvent(list[i]);

    }

    putchar('\n');

}

// Delete an event from an event list
void deleteEvent(event list[], int *size) {

    if (*size == 0) { // If list is empty
        puts("\n| Event list already empty.\n");
        return;
    }

    char temp[21];
    int id;

    char *seperator = "\n+--------------------------------+";
    printf("%s\n|          DELETE EVENT          |%s\n\n", seperator, seperator);

    for (int i = 0; i < *size; i++) { // Display the event list so the user can see which event to delete
        printf("| [%d] ", i);
        printEvent(list[i]);
    }

    printf("%s", "\n| Enter the ID of an event to delete: ");

    fgets(temp, 21, stdin);
    id = atoi(temp);

    if (id > *size - 1) {
        printf("\n| No event located at %d\n", id);
        return;
    }

    printf("| Event [%d] deleted successfully.\n\n", id);

    // Set hour and minute to some trivially large value for sorting purposes
    list[id].hour = 99;
    list[id].minute = 99;
    strcpy(list[id].description, "");

    if (id != (*size - 1)) { // If the event to remove is already last, there's no need to sort it to last
        sort(list, *size);
    }

    (*size)--; // Decrement the size of the list

}

// Replace all spaces in a string with an underscore
char *encode(char *s) {

    for (int i = 0; i < strlen(s); i++) {
        if (s[i] == ' ') {
            s[i] = '_';
        }
    }

    return s;

}

// Replace all underscores in a string with an spaces
char *decode(char *s) {

    for (int i = 0; i < strlen(s); i++) {
        if (s[i] == '_') {
            s[i] = ' ';
        }
    }

    return s;

}

// Save an event list to file
void saveEventList(char *filename, event list[], int size) {

    FILE *f = fopen(filename, "w");

    if (f == NULL) { // If our file is NULL, return
        return;
    }

    for (int i = 0; i < size; i++) {
        fprintf(f, "%d %d %s", list[i].hour, list[i].minute, encode(list[i].description)); // Encode the description (replace spaces with underscores) before saving it into the file
    }

    printf("\n| %d %s successfully saved into \"%s\".\n\n", size, (size == 1) ? "event" : "events", filename); // Tenary expression to make sure we're grammatically correct

    fclose(f);

}

// Load an event list from file
void loadEventList(char *filename, event list[], int *size) {

    FILE *f = fopen(filename, "r");
    char temp[6 + _MAX_DESCRIPTION]; // ## ## MAX_DESCRIPTION_LENGTH

    if (f == NULL) {
        printf("\n| File \"%s\" not found.\n\n", filename);
        return;
    }

    *size = 0; // Set size to 0

    while (fgets(temp, sizeof(temp), f)) {

        char *word = strtok(temp, " "); // Use space as the token delimiter, get the first token (hour)
        list[*size].hour = atoi(word); // Store the token into the list

        word = strtok(NULL, " "); // Get the second token (minute)
        list[*size].minute = atoi(word);

        word = strtok(NULL, " "); // Get the third token (description)
        strcpy(list[*size].description, decode(word)); // Decode our word before copying it (remove underscores)

        (*size)++; // Increment size with each line (event) added

    }

    printf("\n| %d %s successfully loaded from \"%s\".\n", *size, (*size == 1) ? "event" : "events", filename);

    printEventList(list, *size); // Display the event list when finished, show the user what's been loaded

}

int main() {

    event list[_MAX_EVENTS];
    int index = 0; // Number of elements in list
    int selection = 0;
    char file[FILENAME_MAX];
    char response = 'Y';
    char temp[21];

    while (selection != 6) {

        printMenu(); // Print the menu

        printf("%s", "| Please select an option: "); // Prompt for input
        fgets(temp, 21, stdin);
        selection = atoi(temp); // Convert string input to int

        switch (selection) {

        case 1: // New Event
            if (index + 1 > _MAX_EVENTS) {
                printf("| You can only have %d active events at one time!\n\n", index);
                break;
            }
            sortInsert(list, &index, *newEvent(&list[index]));
            break;
        case 2: // Delete Event
            deleteEvent(list, &index);
            break;
        case 3: // Display Schedule
            printEventList(list, index);
            break;
        case 4: // Save Schedule
            if (index == 0) { // No events, don't save anything
                puts("| You have no events in your schedule!\n");
            }
            else {
                printf("%s", "| Please enter a \"filename.txt\": ");
                fgets(file, FILENAME_MAX, stdin);
                strtok(file, "\n"); // Strip newline from filename
                saveEventList(file, list, index);
            }
            break;
        case 5: // Load Schedule
            if (index > 0) {
                printf("%s", "| Are you sure you want to discard your current schedule? (Y/N): ");
                response = toupper(getc(stdin));
                char c;
                while (((c = getchar()) != '\n') && (c != EOF)); // Clear buffer, from getc();
            }
            if (response == 'Y') {
                printf("%s", "| Please enter a \"filename.txt\": ");
                fgets(file, FILENAME_MAX, stdin);
                strtok(file, "\n"); // Strip newline from filename
                loadEventList(file, list, &index);
            }
            break;
        case 6: // Exit Program
            puts("\n| Thank you!\n");
            break;
        default: // Error
            puts("\n| Error in selection\n");
            break;

        }

    }

}

Questions / Concerns

Hopefully I've improved since my last major code review. I've tried to keep the comments useful, so tell me if I'm still being redundant or need to tone that down. There's a few things I have kept in mind while writing this, including not repeating myself as best as possible.

Was the ASCII formatting when printing a good idea? Or should that just be left when showing the menu? In what ways can I improve, what shouldn't I be doing, and what should I be doing?

Answer 1

First of all: Nice piece of code you delivered here. Good start! I think you did quite well - the overall structure is solid and I think the intent of most of the variables are clear to the reader.

Here are some things I noticed, while looking at your code in no particular order:

• Method isNull: I think the method isNull isn't really necessary, since the only thing it does is check, whether something is equal to NULL. I think if (somePointer == NULL) is just as clear as if (isNull(somePointer)). You could even use if (!somePtr), which does exactly the same and I think is clear enough as well. I know this piece of software is not about performance, but you could definitely save the extra overhead of a function call here. You even did it in saveEventList - if (f != NULL).

• do-While in inputRange: This part of your code:

  printf(prompt, min, max);
  
  fgets(temp, 21, stdin);
  input = atoi(temp);
  
  while (input > max || input < min) { // Data validation
      printf(prompt, min, max);
      fgets(temp, 21, stdin);
      input = atoi(temp);
  }
  

  can get written a little bit shorter by using a do-while-loop. Something like this:

  do {
      printf(prompt, min, max);
      fgets(temp, 21, stdin);
      input = atoi(temp);
  } while (input > max || input < min);
  

• No need for casting malloc: You do this for example in initEventand there is no need for explicit casting. Actually, using an explicit cast is discouraged, as described here.

• Check malloc for NULL: One thing you should definitely do is check the return value of malloc. When there is a allocation problem malloc will return NULL and you should be able to handle it. In such a case, this:

  event *e = (event*)malloc(sizeof(event));
  // e is NULL
  
  e->hour = 0;
  e->minute = 0;
  strcpy(e->description, "");
  

  would end in undefined behavior, since you try to dereference NULL. In such a case initEvent() could return NULL as well and the caller has to handle it (print warning, that event couldn't get created).

  event *e = (event*)malloc(sizeof(event));
  
  if (!e) {
      return NULL;
  }
  
  ...
  

• Minor: Print what is about to get read in: I think you should print an initial message before expecting input from the user (instead of printing just the range, print what is about to get read in - hour, minute).

  e->hour = inputRange(0, 23);
  e->minute = inputRange(0, 59);
  

• Why bother checking NULL on non-pointer?: I'm confident, that this part in addEventAtIndex() is unnecessary:

  if (isNull(&e)) { // if our event is NULL, return
      return;
  }
  

  since you passed the actual structure to the function. You would already get an error, when trying to dereference NULL (there is no way to accidentally pass a struct with the address NULL).

• Swap method: The swapping part of your sort function should be in an extra function for clarification and I also think that you can shorten it quite a bit:

  void swapEvents(event list[], int index1, int index2) {
     int tmpHour = list[index1].hour;
     int tmpMinute = list[index1].minute;
  
     char tmpDescription[_MAX_DESCRIPTION];
     strcpy(tmpDescription, list[index1].description);
  
     list[index1].hour = list[index2].hour;
     list[index1].minute = list[index2].minute;
     strcpy(list[index1].description, list[index2].description);
  
     list[index2].hour = tmpHour;
     list[index2].minute = tmpMinute;
     strcpy(list[index2].description, tmpDescription);
  }
  

  This would essentially replace your loop-body in the sorting algorithm.

• Good use of variables in printEvent: I think it's a good thing that you used extra variables in printEvent instead of putting all the arithmetic as printf-arguments. It strongly support readability.

Final thoughts: This was probably a really good exercise for a beginner. Nice job you did there. You could think about using an array of pointers to events instead of events. When doing so sorting will significantly improve, since you don't have to copy whole structs back and forth, but only swap pointers - lightning fast.

Answer 2

Self-documenting code

char h1 = { (e.hour / 10) + '0' }; // Extract the first digit and convert to char (if any, else 0)

As you considered the division by ten and the 0 too enigmatic you added a comment, this is much better than leaving the reader wondering so good job.

The even better option though is stating your intention in code:

char h1 = convert_to_char(first_digit(e.hour))

Where you can easily define:

char convert_to_char(int x) {
    return x + '0';
}

and

int first_digit(int x) {
    return x / 10;
}

Having to maintain / read code and comments is double the effort than only code.

Answer 3

Even though @Herickson already made most of the points but one thing he kindly left for me to note: strlen(3) returns the type size_t which can be anything (but mostly an unsigned long). You should have got a warning from the compiler.

C is a language that is quite dangerous to the permeability of your feet and you should not risk it by switching off compiler warnings. A wrong type can cause quite the unexpected behaviour, e.g.: if you mix up signed and unsigned integers, integers of different sizes or cast between different types. All possible and even useful at times, but still dangerous.

• please check the returns. All returns. Always. (even printf(3) can fail in some circumstances and with embedded processors and it is one of the exceptions to the rule above).

• you do not check the length of the string fgets(3) gets (stdin might also not be available but that's a stretch, I admit).

• neither do not check the return of getc(3). If the user does not enter a letter but enter only it "hangs" for the user because you do not check for EOF. It does not really hang, of course, the user can enter a letter and go on but it is bad UX if the user gets no immediate response in case of an error. (You also should use fgetc(3) if possible, because getc might have been implemented as a macro. Does not matter here but will matter in the future)

• your own functions, too, should return something to indicate failure/success. Yes, you let the user know that something failed, also what and why but you do not know it yourself.

• two different streams for errors and for normal output are better. The standard stream for errors is stderr and the one for output is stdout. If you do it that way the user can redirect the output of a programm to a file and see only the errors or vice versa.

• constants, especially if arbitrary, should have a name. A preprocessor macro suffices in most cases (e.g.: you have 99 set as a flag in deleteEvent, could be a macro, or the 21 for the size of a temporary character array in inputRange, it should be a macro)

• there is more than one loop that waits for user input besides the one in main(). You should keep it all together if possible.

• you do not delete the list entries, you just overwrite them. But I think a linked list is the next thing you'll learn, so I'll keep my mouth shut for now ;-)

All over: good, no reason to fear the next lesson (but you should really learn about the strto*() functions).

Answer 4

A minor point, though it has the potential to get you into trouble:

From the GCC manual section 1.3.3 - Reserved Names:

reserved names include all external identifiers (global functions and variables) that begin with an underscore (‘_’) and all identifiers regardless of use that begin with either two underscores or an underscore followed by a capital letter are reserved names

I may be misinterpreting this, but to me this seems to recommend avoiding any identifier names that begin with an underscore. So you might consider renaming the _MAX_EVENTS and _MAX_DESCRIPTION preprocessor macros.

Answer 5

 for (int i = 1; i < size; i++) {
        for (int j = i; j > 0 && (list[j - 1].hour > list[j].hour || (list[j - 1].hour == list[j].hour && list[j - 1].minute > list[j].minute)); j--) {

The condition in the second for is pretty hard to read. You should extract it to a function.