Event scheduler in C

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
#include
#include
#include
#include

#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 hour = inputRange(0, 23);
e->minute = i

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.

StackExchange Code Review Q#123738, answer score: 37